On 08/12/2015 05:50 PM, Kamil Debski wrote:
> Hi,
>
> On 12 August 2015 at 13:42, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
