em38xx locking question

Mauro,

Function drivers/media/usb/em28xx/em28xx-video.c:get_next_buf (copy pasted below for reference) does not take the list spinlock, yet it modifies the list. Is that correct?

static inline struct em28xx_buffer *get_next_buf(struct em28xx *dev,
struct em28xx_dmaqueue *dma_q)

{
        struct em28xx_buffer *buf;

        if (list_empty(&dma_q->active)) {
                em28xx_isocdbg("No active queue to serve\n");
                return NULL;
        }
 
        /* Get the next buffer */
        buf = list_entry(dma_q->active.next, struct em28xx_buffer, list);
        /* Cleans up buffer - Useful for testing for frame/URB loss */
        list_del(&buf->list);
        buf->pos = 0; 
        buf->vb_buf = buf->mem;
 
        return buf;
}

Thanks!

Quoted 5 lines · Ezequiel Garcia

Mauro,

Function drivers/media/usb/em28xx/em28xx-video.c:get_next_buf (copy pasted below for reference) does not take the list spinlock, yet it modifies the list. Is that correct?

That looks wrong to me. You really need spinlocks here.

Quoted 7 lines · Hans Verkuil

Quoted 5 lines · Ezequiel Garcia

Mauro,

Function drivers/media/usb/em28xx/em28xx-video.c:get_next_buf (copy pasted below for reference) does not take the list spinlock, yet it modifies the list. Is that correct?

Quoted 2 lines

That looks wrong to me. You really need spinlocks here.

OK, second question then. Is there any way to guarantee the URBs irq handler is *not* running, when vb2_ops are called (e.g. stop_streaming)?

Otherwise, given stop_streaming will return the current buffer to vb2 (dev->usb_ctl.vid_buf), I believe that will race against the irq handler, which is processing it.

It seems that's currently racy as well.

Quoted 13 lines · Ezequiel Garcia

Quoted 9 lines · Hans Verkuil

Quoted 5 lines · Ezequiel Garcia

Mauro,

Function drivers/media/usb/em28xx/em28xx-video.c:get_next_buf (copy pasted below for reference) does not take the list spinlock, yet it modifies the list. Is that correct?

That looks wrong to me. You really need spinlocks here.

Quoted 2 lines

OK, second question then. Is there any way to guarantee the URBs irq handler is *not* running, when vb2_ops are called (e.g. stop_streaming)?

That depends on the op. But stop_streaming is the op that is supposed to turn off the streaming (and thus the irq), so it depends on the order of how things are done in that function.

Quoted 6 lines

Otherwise, given stop_streaming will return the current buffer to vb2 (dev->usb_ctl.vid_buf), I believe that will race against the irq handler, which is processing it.

It seems that's currently racy as well.

Hmm, the stop_streaming code looks fine at first sight, but I think there is a race if you start streaming both video and vbi, and then stop streaming one of the two. I think the code might keep calling get_next_buf() in that case, even if for that stream the streaming was stopped.

This is a problem anyway: get_next_buf() should do this check at the beginning:

	if (!vb2_start_streaming_called(vb2_queue))
		return NULL;

to prevent it from using buffer before start_streaming was actually called.

Quoted 14 lines · Hans Verkuil

Quoted 13 lines · Ezequiel Garcia

Quoted 9 lines · Hans Verkuil

Quoted 5 lines · Ezequiel Garcia

Mauro,

Function drivers/media/usb/em28xx/em28xx-video.c:get_next_buf (copy pasted below for reference) does not take the list spinlock, yet it modifies the list. Is that correct?

That looks wrong to me. You really need spinlocks here.

Quoted 7 lines

Quoted 2 lines

OK, second question then. Is there any way to guarantee the URBs irq handler is *not* running, when vb2_ops are called (e.g. stop_streaming)?

That depends on the op. But stop_streaming is the op that is supposed to turn off the streaming (and thus the irq), so it depends on the order of how things are done in that function.

Ah, right. As long as you kill the urbs before you try to access the current buffer, everything is OK.

Quoted 14 lines

Quoted 6 lines

Otherwise, given stop_streaming will return the current buffer to vb2 (dev->usb_ctl.vid_buf), I believe that will race against the irq handler, which is processing it.

It seems that's currently racy as well.

Hmm, the stop_streaming code looks fine at first sight, but I think there is a race if you start streaming both video and vbi, and then stop streaming one of the two. I think the code might keep calling get_next_buf() in that case, even if for that stream the streaming was stopped.

This is a problem anyway: get_next_buf() should do this check at the beginning:

Quoted 5 lines

	if (!vb2_start_streaming_called(vb2_queue))
		return NULL;

to prevent it from using buffer before start_streaming was actually called.

I'd say get_next_buf() is called only in the URB complete handler path, and hence only after start_streaming.

However, maybe there's a subtle
issue here: URB complete handler can be called _while_ start_streaming is still running.