Re: em38xx locking question

[Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>]

On 03/10/2015 10:46 AM, Hans Verkuil wrote:
> On 03/10/2015 02:29 PM, Ezequiel Garcia wrote:
>>
>>
>> On 03/10/2015 10:26 AM, Hans Verkuil wrote:
>>> On 03/10/2015 02:18 PM, Ezequiel Garcia wrote:
>>>> 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.
>>>
>>
>> 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.

>> 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.
> 

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.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar