| Message ID | 20200825145556.637323-3-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: > > 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 >
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 > >
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; >
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 > > >
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 >>>>
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;
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(-)