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

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 06/11/13 15:51, Hans Verkuil wrote:
>>>> 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?

Sounds OK to me.

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

Perhaps I'm misunderstanding something, but isn't it the opposite,
e.g. foo() which does lock, and __foo() that doesn't ?

So having a set of functions like:

/* locks */
int vb2_fop_release(struct file *file)

/* locks conditionally */
int _vb2_fop_release(struct file *file, struct mutex *lock)

/* doesn't lock */
int __vb2_fop_release(struct file *file)

would make sense ?

(I hope we end that thread soon :))

--
Regards,
Sylwester