Re: [PATCH 2/2] [media] m2m: fix bad unlock balance

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/12/2015 05:50 PM, Kamil Debski wrote:
> Hi,
> 
> On 12 August 2015 at 13:42, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Hello Hans,
>>
>> I'm sorry for a delay. Once again I've been busy with some other internal
>> stuff.
>>
>> On 2015-07-28 11:02, Hans Verkuil wrote:
>>>
>>> Kamil, Marek,
>>>
>>> Why does v4l2_m2m_poll unlock and lock in that function?
>>
>>
>> I've checked the code and indeed the poll_wait() function doesn't do
>> anything that
>> should not be done with queue mutex being taken. I don't remember if it was
>> always
>> like that. You are right that the unlock&lock code should be removed.
>>
>>> Zahari is right that the locking is unbalanced, but I don't see the reason
>>> for the unlock/lock sequence in the first place. I'm wondering if that
>>> shouldn't just be removed.
>>>
>>> Am I missing something?
>>>
>>> Instead, I would expect to see a spin_lock_irqsave(&src/dst_q->done_lock,
>>> flags)
>>> around the list_empty(&src/dst_q->done_list) calls.
>>
>>
>> Indeed, that's another thing that should be fixed in this function. I looks
>> that
>> commit c16218402a000bb25c1277c43ae98c11bcb59bd1 ("[media] videobuf2: return
>> -EPIPE
>> from DQBUF after the last buffer") is the root cause of both issues
>> (unballanced
>> locking and lack of spinlock protection), while the unnecessary queue
>> unlock/lock
>> sequence was there from the beginning.
>>
> 
> I am all with Marek on this. Unlock/lock was there from the beginning,
> it is not necessary. I agree also that spin_lock/unlock should be
> added for the list_empty call.

Zahari, will you make a new version of this patch with the suggested changes?

Regards,

	Hans