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

[Zahari Doychev <zax@zaxnet.org>]

On Fri, Aug 14, 2015 at 01:57:51PM +0200, Hans Verkuil wrote:
> 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?

yes I will do it.

Regards

Zahari

> 
> Regards,
> 
> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html