| Message ID | 20200825145556.637323-2-gnurou@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-mem2mem: fix poll() bug | expand |
Hi Alexandre, On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@gmail.com> wrote: > > If poll() is called on a m2m device with the EPOLLOUT event after the > last buffer of the CAPTURE queue is dequeued, any buffer available on > OUTPUT queue will never be signaled because v4l2_m2m_poll_for_data() > starts by checking whether dst_q->last_buffer_dequeued is set and > returns EPOLLIN in this case, without looking at the state of the OUTPUT > queue. > > Fix this by checking the state of the OUTPUT queue before considering > that early-return case. > > This also has the side-effect of bringing the two blocks of code dealing > with the CAPTURE queue next to one another, and saves us one spin > lock/unlock cycle, for what it's worth. > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com> Change looks good to me. Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> Do you think it qualifies for -stable? The issue has been here since the dawn of time. Thanks, Ezequiel > --- > drivers/media/v4l2-core/v4l2-mem2mem.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c > index 95a8f2dc5341d..0d0192119af20 100644 > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c > @@ -862,6 +862,15 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > list_empty(&dst_q->queued_list))) > return EPOLLERR; > > + spin_lock_irqsave(&src_q->done_lock, flags); > + if (!list_empty(&src_q->done_list)) > + src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, > + done_entry); > + if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE > + || src_vb->state == VB2_BUF_STATE_ERROR)) > + rc |= EPOLLOUT | EPOLLWRNORM; > + spin_unlock_irqrestore(&src_q->done_lock, flags); > + > spin_lock_irqsave(&dst_q->done_lock, flags); > if (list_empty(&dst_q->done_list)) { > /* > @@ -870,21 +879,11 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > */ > if (dst_q->last_buffer_dequeued) { > spin_unlock_irqrestore(&dst_q->done_lock, flags); > - return EPOLLIN | EPOLLRDNORM; > + rc |= EPOLLIN | EPOLLRDNORM; > + return rc; > } > } > - spin_unlock_irqrestore(&dst_q->done_lock, flags); > > - spin_lock_irqsave(&src_q->done_lock, flags); > - if (!list_empty(&src_q->done_list)) > - src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, > - done_entry); > - if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE > - || src_vb->state == VB2_BUF_STATE_ERROR)) > - rc |= EPOLLOUT | EPOLLWRNORM; > - spin_unlock_irqrestore(&src_q->done_lock, flags); > - > - spin_lock_irqsave(&dst_q->done_lock, flags); > if (!list_empty(&dst_q->done_list)) > dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer, > done_entry); > -- > 2.28.0 >
On Wed, Aug 26, 2020 at 1:08 PM Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > > Hi Alexandre, > > On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@gmail.com> wrote: > > > > If poll() is called on a m2m device with the EPOLLOUT event after the > > last buffer of the CAPTURE queue is dequeued, any buffer available on > > OUTPUT queue will never be signaled because v4l2_m2m_poll_for_data() > > starts by checking whether dst_q->last_buffer_dequeued is set and > > returns EPOLLIN in this case, without looking at the state of the OUTPUT > > queue. > > > > Fix this by checking the state of the OUTPUT queue before considering > > that early-return case. > > > > This also has the side-effect of bringing the two blocks of code dealing > > with the CAPTURE queue next to one another, and saves us one spin > > lock/unlock cycle, for what it's worth. > > > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com> > > Change looks good to me. > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > Do you think it qualifies for -stable? The issue has been > here since the dawn of time. Indeed, and this should be quite a rare corner case. I will leave that call to the maintainers. > > Thanks, > Ezequiel > > > --- > > drivers/media/v4l2-core/v4l2-mem2mem.c | 23 +++++++++++------------ > > 1 file changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c > > index 95a8f2dc5341d..0d0192119af20 100644 > > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c > > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c > > @@ -862,6 +862,15 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > > list_empty(&dst_q->queued_list))) > > return EPOLLERR; > > > > + spin_lock_irqsave(&src_q->done_lock, flags); > > + if (!list_empty(&src_q->done_list)) > > + src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, > > + done_entry); > > + if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE > > + || src_vb->state == VB2_BUF_STATE_ERROR)) > > + rc |= EPOLLOUT | EPOLLWRNORM; > > + spin_unlock_irqrestore(&src_q->done_lock, flags); > > + > > spin_lock_irqsave(&dst_q->done_lock, flags); > > if (list_empty(&dst_q->done_list)) { > > /* > > @@ -870,21 +879,11 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > > */ > > if (dst_q->last_buffer_dequeued) { > > spin_unlock_irqrestore(&dst_q->done_lock, flags); > > - return EPOLLIN | EPOLLRDNORM; > > + rc |= EPOLLIN | EPOLLRDNORM; > > + return rc; > > } > > } > > - spin_unlock_irqrestore(&dst_q->done_lock, flags); > > > > - spin_lock_irqsave(&src_q->done_lock, flags); > > - if (!list_empty(&src_q->done_list)) > > - src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, > > - done_entry); > > - if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE > > - || src_vb->state == VB2_BUF_STATE_ERROR)) > > - rc |= EPOLLOUT | EPOLLWRNORM; > > - spin_unlock_irqrestore(&src_q->done_lock, flags); > > - > > - spin_lock_irqsave(&dst_q->done_lock, flags); > > if (!list_empty(&dst_q->done_list)) > > dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer, > > done_entry); > > -- > > 2.28.0 > >
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c index 95a8f2dc5341d..0d0192119af20 100644 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c @@ -862,6 +862,15 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, list_empty(&dst_q->queued_list))) return EPOLLERR; + spin_lock_irqsave(&src_q->done_lock, flags); + if (!list_empty(&src_q->done_list)) + src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, + done_entry); + if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE + || src_vb->state == VB2_BUF_STATE_ERROR)) + rc |= EPOLLOUT | EPOLLWRNORM; + spin_unlock_irqrestore(&src_q->done_lock, flags); + spin_lock_irqsave(&dst_q->done_lock, flags); if (list_empty(&dst_q->done_list)) { /* @@ -870,21 +879,11 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, */ if (dst_q->last_buffer_dequeued) { spin_unlock_irqrestore(&dst_q->done_lock, flags); - return EPOLLIN | EPOLLRDNORM; + rc |= EPOLLIN | EPOLLRDNORM; + return rc; } } - spin_unlock_irqrestore(&dst_q->done_lock, flags); - spin_lock_irqsave(&src_q->done_lock, flags); - if (!list_empty(&src_q->done_list)) - src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, - done_entry); - if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE - || src_vb->state == VB2_BUF_STATE_ERROR)) - rc |= EPOLLOUT | EPOLLWRNORM; - spin_unlock_irqrestore(&src_q->done_lock, flags); - - spin_lock_irqsave(&dst_q->done_lock, flags); if (!list_empty(&dst_q->done_list)) dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer, done_entry);
If poll() is called on a m2m device with the EPOLLOUT event after the last buffer of the CAPTURE queue is dequeued, any buffer available on OUTPUT queue will never be signaled because v4l2_m2m_poll_for_data() starts by checking whether dst_q->last_buffer_dequeued is set and returns EPOLLIN in this case, without looking at the state of the OUTPUT queue. Fix this by checking the state of the OUTPUT queue before considering that early-return case. This also has the side-effect of bringing the two blocks of code dealing with the CAPTURE queue next to one another, and saves us one spin lock/unlock cycle, for what it's worth. Signed-off-by: Alexandre Courbot <gnurou@gmail.com> --- drivers/media/v4l2-core/v4l2-mem2mem.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)