[PATCH 0/2] media: v4l2-mem2mem: fix poll() bug

[PATCH 0/2] media: v4l2-mem2mem: fix poll() bug

[Alexandre Courbot]

This addresses a very corner case that probably nobody ever encounters,
but I have hit it when playing with vicoded so here is a tentative fix.

Patch 1/2 addresses the issue that when the last buffer of a m2m device
has been dequeued, any attempt to poll with EPOLLOUT will result in only
EPOLLIN being returned, even if OUTPUT buffers are still pending. The
issue stems from the fact that the last buffer check if done first, and
returns immediately if true.

Patch 2/2 builds on the first one to (hopefully) clean up the code a bit
and make the function flow easier to follow. Functionally speaking it is
supposed to be a no-op and it can safely be dropped if the former code
is preferred - the actual fix is in 1/2.

Alexandre Courbot (2):
  media: v4l2-mem2mem: consider OUTPUT queue first when polling
  media: v4l2-mem2mem: simplify poll logic a bit

 drivers/media/v4l2-core/v4l2-mem2mem.c | 42 +++++++++++---------------
 1 file changed, 18 insertions(+), 24 deletions(-)

-- 
2.28.0

[PATCH 1/2] media: v4l2-mem2mem: consider OUTPUT queue first when polling

[Alexandre Courbot]

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(-)

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

[PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

[Alexandre Courbot]

Factorize redundant checks into a single code block, remove the early
return, and declare variables in their innermost block. Hopefully this
makes this code a little bit easier to follow.

Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0d0192119af20..aeac9707123d0 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
 				       struct poll_table_struct *wait)
 {
 	struct vb2_queue *src_q, *dst_q;
-	struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
 	__poll_t rc = 0;
 	unsigned long flags;
 
@@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
 		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;
+	if (!list_empty(&src_q->done_list)) {
+		struct vb2_buffer *src_vb = list_first_entry(
+			&src_q->done_list, struct vb2_buffer, done_entry);
+		if (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)) {
+	if (!list_empty(&dst_q->done_list)) {
+		struct vb2_buffer *dst_vb = list_first_entry(
+			&dst_q->done_list, struct vb2_buffer, done_entry);
+		if (dst_vb->state == VB2_BUF_STATE_DONE ||
+		    dst_vb->state == VB2_BUF_STATE_ERROR)
+			rc |= EPOLLIN | EPOLLRDNORM;
+	} else if (dst_q->last_buffer_dequeued) {
 		/*
 		 * If the last buffer was dequeued from the capture queue,
 		 * return immediately. DQBUF will return -EPIPE.
 		 */
-		if (dst_q->last_buffer_dequeued) {
-			spin_unlock_irqrestore(&dst_q->done_lock, flags);
-			rc |= EPOLLIN | EPOLLRDNORM;
-			return rc;
-		}
-	}
-
-	if (!list_empty(&dst_q->done_list))
-		dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer,
-						done_entry);
-	if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE
-			|| dst_vb->state == VB2_BUF_STATE_ERROR))
 		rc |= EPOLLIN | EPOLLRDNORM;
+	}
 	spin_unlock_irqrestore(&dst_q->done_lock, flags);
 
 	return rc;
-- 
2.28.0

Re: [PATCH 0/2] media: v4l2-mem2mem: fix poll() bug

[Ezequiel Garcia]

Hello Alex,

Thanks for the patch.

On Tue, Aug 25, 2020, 11:56 AM Alexandre Courbot <gnurou@gmail.com> wrote:
>
> This addresses a very corner case that probably nobody ever encounters,
> but I have hit it when playing with vicoded so here is a tentative fix.
>

I'll try to make a more complete review soon, but meanwhile
I was thinking if it was possible to include a little kselftest program
for this issue, something CIs can pick-up and test corner cases like this,
making sure we don't regress on the issue.

(Or alternatively, v4l2-compliance?)

Thanks!
Ezequiel

> Patch 1/2 addresses the issue that when the last buffer of a m2m device
> has been dequeued, any attempt to poll with EPOLLOUT will result in only
> EPOLLIN being returned, even if OUTPUT buffers are still pending. The
> issue stems from the fact that the last buffer check if done first, and
> returns immediately if true.
>
> Patch 2/2 builds on the first one to (hopefully) clean up the code a bit
> and make the function flow easier to follow. Functionally speaking it is
> supposed to be a no-op and it can safely be dropped if the former code
> is preferred - the actual fix is in 1/2.
>
> Alexandre Courbot (2):
>   media: v4l2-mem2mem: consider OUTPUT queue first when polling
>   media: v4l2-mem2mem: simplify poll logic a bit
>
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 42 +++++++++++---------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
>
> --
> 2.28.0
>

Re: [PATCH 1/2] media: v4l2-mem2mem: consider OUTPUT queue first when polling

[Ezequiel Garcia]

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
>

Re: [PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

[Ezequiel Garcia]

Hi Alexandre,

On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@gmail.com> wrote:
>
> Factorize redundant checks into a single code block, remove the early
> return, and declare variables in their innermost block. Hopefully this
> makes this code a little bit easier to follow.
>

This _definitely_ makes the poll handling more readable.

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

See below a nitpick.

> Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 0d0192119af20..aeac9707123d0 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>                                        struct poll_table_struct *wait)
>  {
>         struct vb2_queue *src_q, *dst_q;
> -       struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
>         __poll_t rc = 0;
>         unsigned long flags;
>
> @@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>                 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;
> +       if (!list_empty(&src_q->done_list)) {
> +               struct vb2_buffer *src_vb = list_first_entry(
> +                       &src_q->done_list, struct vb2_buffer, done_entry);
> +               if (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)) {
> +       if (!list_empty(&dst_q->done_list)) {
> +               struct vb2_buffer *dst_vb = list_first_entry(
> +                       &dst_q->done_list, struct vb2_buffer, done_entry);
> +               if (dst_vb->state == VB2_BUF_STATE_DONE ||
> +                   dst_vb->state == VB2_BUF_STATE_ERROR)
> +                       rc |= EPOLLIN | EPOLLRDNORM;
> +       } else if (dst_q->last_buffer_dequeued) {
>                 /*
>                  * If the last buffer was dequeued from the capture queue,
>                  * return immediately. DQBUF will return -EPIPE.
>                  */

The part about "returning immediately" doesn't make
much sense now. Could we rephrase this, keeping the -EPIPE
comment?

Thanks,
Ezequiel

> -               if (dst_q->last_buffer_dequeued) {
> -                       spin_unlock_irqrestore(&dst_q->done_lock, flags);
> -                       rc |= EPOLLIN | EPOLLRDNORM;
> -                       return rc;
> -               }
> -       }
> -
> -       if (!list_empty(&dst_q->done_list))
> -               dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer,
> -                                               done_entry);
> -       if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE
> -                       || dst_vb->state == VB2_BUF_STATE_ERROR))
>                 rc |= EPOLLIN | EPOLLRDNORM;
> +       }
>         spin_unlock_irqrestore(&dst_q->done_lock, flags);
>
>         return rc;
> --
> 2.28.0
>

Re: [PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

[Alexandre Courbot]

Hi Ezequiel, thanks for the review!

On Wed, Aug 26, 2020 at 1:15 PM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Hi Alexandre,
>
> On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@gmail.com> wrote:
> >
> > Factorize redundant checks into a single code block, remove the early
> > return, and declare variables in their innermost block. Hopefully this
> > makes this code a little bit easier to follow.
> >
>
> This _definitely_ makes the poll handling more readable.
>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>
> See below a nitpick.
>
> > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
> >  1 file changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > index 0d0192119af20..aeac9707123d0 100644
> > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
> >                                        struct poll_table_struct *wait)
> >  {
> >         struct vb2_queue *src_q, *dst_q;
> > -       struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
> >         __poll_t rc = 0;
> >         unsigned long flags;
> >
> > @@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
> >                 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;
> > +       if (!list_empty(&src_q->done_list)) {
> > +               struct vb2_buffer *src_vb = list_first_entry(
> > +                       &src_q->done_list, struct vb2_buffer, done_entry);
> > +               if (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)) {
> > +       if (!list_empty(&dst_q->done_list)) {
> > +               struct vb2_buffer *dst_vb = list_first_entry(
> > +                       &dst_q->done_list, struct vb2_buffer, done_entry);
> > +               if (dst_vb->state == VB2_BUF_STATE_DONE ||
> > +                   dst_vb->state == VB2_BUF_STATE_ERROR)
> > +                       rc |= EPOLLIN | EPOLLRDNORM;
> > +       } else if (dst_q->last_buffer_dequeued) {
> >                 /*
> >                  * If the last buffer was dequeued from the capture queue,
> >                  * return immediately. DQBUF will return -EPIPE.
> >                  */
>
> The part about "returning immediately" doesn't make
> much sense now. Could we rephrase this, keeping the -EPIPE
> comment?

I understood this sentence as referring to the system call and not
just this function, but maybe we can rephrase this as "... make
user-space wake up immediately"?

>
> Thanks,
> Ezequiel
>
> > -               if (dst_q->last_buffer_dequeued) {
> > -                       spin_unlock_irqrestore(&dst_q->done_lock, flags);
> > -                       rc |= EPOLLIN | EPOLLRDNORM;
> > -                       return rc;
> > -               }
> > -       }
> > -
> > -       if (!list_empty(&dst_q->done_list))
> > -               dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer,
> > -                                               done_entry);
> > -       if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE
> > -                       || dst_vb->state == VB2_BUF_STATE_ERROR))
> >                 rc |= EPOLLIN | EPOLLRDNORM;
> > +       }
> >         spin_unlock_irqrestore(&dst_q->done_lock, flags);
> >
> >         return rc;
> > --
> > 2.28.0
> >

Re: [PATCH 1/2] media: v4l2-mem2mem: consider OUTPUT queue first when polling

[Alexandre Courbot]

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

Re: [PATCH 0/2] media: v4l2-mem2mem: fix poll() bug

[Alexandre Courbot]

On Wed, Aug 26, 2020 at 7:10 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Hello Alex,
>
> Thanks for the patch.
>
> On Tue, Aug 25, 2020, 11:56 AM Alexandre Courbot <gnurou@gmail.com> wrote:
> >
> > This addresses a very corner case that probably nobody ever encounters,
> > but I have hit it when playing with vicoded so here is a tentative fix.
> >
>
> I'll try to make a more complete review soon, but meanwhile
> I was thinking if it was possible to include a little kselftest program
> for this issue, something CIs can pick-up and test corner cases like this,
> making sure we don't regress on the issue.
>
> (Or alternatively, v4l2-compliance?)

I am not very familiar with kselftest, but IIUC the conditions that
lead to this issue are easier to reproduce using v4l2-compliance,
where we can have a user-space driving the queues. It would also have
the benefit to exercise all drivers. I'll think about adding such a
test, thanks for the suggestion!

>
> Thanks!
> Ezequiel
>
> > Patch 1/2 addresses the issue that when the last buffer of a m2m device
> > has been dequeued, any attempt to poll with EPOLLOUT will result in only
> > EPOLLIN being returned, even if OUTPUT buffers are still pending. The
> > issue stems from the fact that the last buffer check if done first, and
> > returns immediately if true.
> >
> > Patch 2/2 builds on the first one to (hopefully) clean up the code a bit
> > and make the function flow easier to follow. Functionally speaking it is
> > supposed to be a no-op and it can safely be dropped if the former code
> > is preferred - the actual fix is in 1/2.
> >
> > Alexandre Courbot (2):
> >   media: v4l2-mem2mem: consider OUTPUT queue first when polling
> >   media: v4l2-mem2mem: simplify poll logic a bit
> >
> >  drivers/media/v4l2-core/v4l2-mem2mem.c | 42 +++++++++++---------------
> >  1 file changed, 18 insertions(+), 24 deletions(-)
> >
> > --
> > 2.28.0
> >

Re: [PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

[Hans Verkuil]

On 25/08/2020 16:55, Alexandre Courbot wrote:
> Factorize redundant checks into a single code block, remove the early
> return, and declare variables in their innermost block. Hopefully this
> makes this code a little bit easier to follow.
> 
> Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 0d0192119af20..aeac9707123d0 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>  				       struct poll_table_struct *wait)
>  {
>  	struct vb2_queue *src_q, *dst_q;
> -	struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
>  	__poll_t rc = 0;
>  	unsigned long flags;
>  
> @@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>  		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;
> +	if (!list_empty(&src_q->done_list)) {
> +		struct vb2_buffer *src_vb = list_first_entry(
> +			&src_q->done_list, struct vb2_buffer, done_entry);
> +		if (src_vb->state == VB2_BUF_STATE_DONE ||
> +		    src_vb->state == VB2_BUF_STATE_ERROR)

This test is unnecessary: only buffers in state DONE or ERROR can be on the done_list.

> +			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)) {
> +	if (!list_empty(&dst_q->done_list)) {
> +		struct vb2_buffer *dst_vb = list_first_entry(
> +			&dst_q->done_list, struct vb2_buffer, done_entry);
> +		if (dst_vb->state == VB2_BUF_STATE_DONE ||
> +		    dst_vb->state == VB2_BUF_STATE_ERROR)

Ditto.

Regards,

	Hans

> +			rc |= EPOLLIN | EPOLLRDNORM;
> +	} else if (dst_q->last_buffer_dequeued) {
>  		/*
>  		 * If the last buffer was dequeued from the capture queue,
>  		 * return immediately. DQBUF will return -EPIPE.
>  		 */
> -		if (dst_q->last_buffer_dequeued) {
> -			spin_unlock_irqrestore(&dst_q->done_lock, flags);
> -			rc |= EPOLLIN | EPOLLRDNORM;
> -			return rc;
> -		}
> -	}
> -
> -	if (!list_empty(&dst_q->done_list))
> -		dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer,
> -						done_entry);
> -	if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE
> -			|| dst_vb->state == VB2_BUF_STATE_ERROR))
>  		rc |= EPOLLIN | EPOLLRDNORM;
> +	}
>  	spin_unlock_irqrestore(&dst_q->done_lock, flags);
>  
>  	return rc;
> 

Re: [PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

[Ezequiel Garcia]

On Wed, 26 Aug 2020 at 08:19, Alexandre Courbot <gnurou@gmail.com> wrote:
>
> Hi Ezequiel, thanks for the review!
>
> On Wed, Aug 26, 2020 at 1:15 PM Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> >
> > Hi Alexandre,
> >
> > On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@gmail.com> wrote:
> > >
> > > Factorize redundant checks into a single code block, remove the early
> > > return, and declare variables in their innermost block. Hopefully this
> > > makes this code a little bit easier to follow.
> > >
> >
> > This _definitely_ makes the poll handling more readable.
> >
> > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> >
> > See below a nitpick.
> >
> > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
> > >  1 file changed, 15 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > index 0d0192119af20..aeac9707123d0 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
> > >                                        struct poll_table_struct *wait)
> > >  {
> > >         struct vb2_queue *src_q, *dst_q;
> > > -       struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
> > >         __poll_t rc = 0;
> > >         unsigned long flags;
> > >
> > > @@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
> > >                 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;
> > > +       if (!list_empty(&src_q->done_list)) {
> > > +               struct vb2_buffer *src_vb = list_first_entry(
> > > +                       &src_q->done_list, struct vb2_buffer, done_entry);
> > > +               if (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)) {
> > > +       if (!list_empty(&dst_q->done_list)) {
> > > +               struct vb2_buffer *dst_vb = list_first_entry(
> > > +                       &dst_q->done_list, struct vb2_buffer, done_entry);
> > > +               if (dst_vb->state == VB2_BUF_STATE_DONE ||
> > > +                   dst_vb->state == VB2_BUF_STATE_ERROR)
> > > +                       rc |= EPOLLIN | EPOLLRDNORM;
> > > +       } else if (dst_q->last_buffer_dequeued) {
> > >                 /*
> > >                  * If the last buffer was dequeued from the capture queue,
> > >                  * return immediately. DQBUF will return -EPIPE.
> > >                  */
> >
> > The part about "returning immediately" doesn't make
> > much sense now. Could we rephrase this, keeping the -EPIPE
> > comment?
>
> I understood this sentence as referring to the system call and not
> just this function, but maybe we can rephrase this as "... make
> user-space wake up immediately"?
>

But is this really about user-space wakeup? I am under the impression
that past poll_wait on both queues, we are already about to return
(and wakeup).

The way I see it, the original commit intention was to skip any
done_list handling, returning immediately on the last buffer condition.

How about just

"""
If the last buffer was dequeued from the capture queue,
signal userspace. DQBUF will return -EPIPE.
"""

?

> >
> > Thanks,
> > Ezequiel
> >
> > > -               if (dst_q->last_buffer_dequeued) {
> > > -                       spin_unlock_irqrestore(&dst_q->done_lock, flags);
> > > -                       rc |= EPOLLIN | EPOLLRDNORM;
> > > -                       return rc;
> > > -               }
> > > -       }
> > > -
> > > -       if (!list_empty(&dst_q->done_list))
> > > -               dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer,
> > > -                                               done_entry);
> > > -       if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE
> > > -                       || dst_vb->state == VB2_BUF_STATE_ERROR))
> > >                 rc |= EPOLLIN | EPOLLRDNORM;
> > > +       }
> > >         spin_unlock_irqrestore(&dst_q->done_lock, flags);
> > >
> > >         return rc;
> > > --
> > > 2.28.0
> > >

Re: [PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

[Hans Verkuil]

On 26/08/2020 16:32, Ezequiel Garcia wrote:
> On Wed, 26 Aug 2020 at 08:19, Alexandre Courbot <gnurou@gmail.com> wrote:
>>
>> Hi Ezequiel, thanks for the review!
>>
>> On Wed, Aug 26, 2020 at 1:15 PM Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>
>>> Hi Alexandre,
>>>
>>> On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@gmail.com> wrote:
>>>>
>>>> Factorize redundant checks into a single code block, remove the early
>>>> return, and declare variables in their innermost block. Hopefully this
>>>> makes this code a little bit easier to follow.
>>>>
>>>
>>> This _definitely_ makes the poll handling more readable.
>>>
>>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>>>
>>> See below a nitpick.
>>>
>>>> Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
>>>>  1 file changed, 15 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> index 0d0192119af20..aeac9707123d0 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>>>>                                        struct poll_table_struct *wait)
>>>>  {
>>>>         struct vb2_queue *src_q, *dst_q;
>>>> -       struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
>>>>         __poll_t rc = 0;
>>>>         unsigned long flags;
>>>>
>>>> @@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>>>>                 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;
>>>> +       if (!list_empty(&src_q->done_list)) {
>>>> +               struct vb2_buffer *src_vb = list_first_entry(
>>>> +                       &src_q->done_list, struct vb2_buffer, done_entry);
>>>> +               if (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)) {
>>>> +       if (!list_empty(&dst_q->done_list)) {
>>>> +               struct vb2_buffer *dst_vb = list_first_entry(
>>>> +                       &dst_q->done_list, struct vb2_buffer, done_entry);
>>>> +               if (dst_vb->state == VB2_BUF_STATE_DONE ||
>>>> +                   dst_vb->state == VB2_BUF_STATE_ERROR)
>>>> +                       rc |= EPOLLIN | EPOLLRDNORM;
>>>> +       } else if (dst_q->last_buffer_dequeued) {
>>>>                 /*
>>>>                  * If the last buffer was dequeued from the capture queue,
>>>>                  * return immediately. DQBUF will return -EPIPE.
>>>>                  */
>>>
>>> The part about "returning immediately" doesn't make
>>> much sense now. Could we rephrase this, keeping the -EPIPE
>>> comment?
>>
>> I understood this sentence as referring to the system call and not
>> just this function, but maybe we can rephrase this as "... make
>> user-space wake up immediately"?
>>
> 
> But is this really about user-space wakeup? I am under the impression
> that past poll_wait on both queues, we are already about to return
> (and wakeup).
> 
> The way I see it, the original commit intention was to skip any
> done_list handling, returning immediately on the last buffer condition.
> 
> How about just
> 
> """
> If the last buffer was dequeued from the capture queue,
> signal userspace. DQBUF will return -EPIPE.

I'd write 'DQBUF(CAPTURE)' here to emphasize that only the capture
queue will return -EPIPE when you try to dequeue from it.

Also note that the original text was a copy-and-paste from vb2_core_poll().
The phrase 'return immediately' makes sense in that context since that
poll code deals with a single queue. In this case you have two queues,
and 'return immediately' no longer applies (in fact, that effectively is
the bug that being fixed here!).

Regards,

	Hans

> """
> 
> ?
> 
>>>
>>> Thanks,
>>> Ezequiel
>>>
>>>> -               if (dst_q->last_buffer_dequeued) {
>>>> -                       spin_unlock_irqrestore(&dst_q->done_lock, flags);
>>>> -                       rc |= EPOLLIN | EPOLLRDNORM;
>>>> -                       return rc;
>>>> -               }
>>>> -       }
>>>> -
>>>> -       if (!list_empty(&dst_q->done_list))
>>>> -               dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer,
>>>> -                                               done_entry);
>>>> -       if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE
>>>> -                       || dst_vb->state == VB2_BUF_STATE_ERROR))
>>>>                 rc |= EPOLLIN | EPOLLRDNORM;
>>>> +       }
>>>>         spin_unlock_irqrestore(&dst_q->done_lock, flags);
>>>>
>>>>         return rc;
>>>> --
>>>> 2.28.0
>>>>