Re: [PATCH v5] videobuf2: Add missing lock held on vb2_fop_relase

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/06/13 15:46, Sylwester Nawrocki wrote:
> Hello,
> 
> (dropping some unrelated e-mail addresses from Cc)
> 
> On 06/11/13 10:07, Hans Verkuil wrote:
>> On 11/06/13 09:24, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com>
>>>
>>> vb2_fop_relase does not held the lock although it is modifying the
>>
>> Small typo: _relase -> _release
>>
>>> queue->owner field.
>>>
> [...]
>>>
>>> Signed-off-by: Ricardo Ribalda <ricardo.ribalda@gmail.com>
>>> ---
>>>
>>> v2: Comments by Sylvester Nawrocki
>>>
>>> fimc-capture and fimc-lite where calling vb2_fop_release with the lock held.
>>> Therefore a new __vb2_fop_release function has been created to be used by
>>> drivers that overload the release function.
>>>
>>> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab
>>>
>>> Use vb2_fop_release_locked instead of __vb2_fop_release
>>>
>>> v4: Comments by Sylvester Nawrocki
>>>
>>> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>>>
>>> v5: Comments by Sylvester Nawrocki and Hans Verkuil
>>>
>>> Rename __vb2_fop_release to vb2_fop_release_unlock and rearrange
>>> arguments
>>
>> I know I suggested the vb2_fop_release_unlock name, but on second thoughts
>> that's not a good name. I suggest vb2_fop_release_no_lock instead.
>> '_unlock' suggests that there is a _lock version as well, which isn't the
>> case here.
> 
> Yes. Sorry, but to me vb2_fop_release_no_lock() looks s bit ugly.
> Couldn't we just use double underscore prefix instead of _no_lock postfix,
> as is commonly done in the kernel ?

I'm not keen on that since we then end up with a no-prefix version, a '_'
version and a '__' version. A bit obscure IMHO.

How about just exporting the _vb2_fop_release function and pass a NULL
pointer as lock?

> Grep reveals almost no use of "_no_lock" in function names.

Usually the version without prefix doesn't lock, and you have a _lock
version that does lock. Unfortunately, we couldn't use that here.

Regards,

	Hans