RFC: m2m device fixes

The coda context queue sequence counter is incremented only if the vb2 source buffer payload is non zero. This makes possible to signal EOS otherwise the condition in coda_buf_is_end_of_stream is never met or more precisely buf->v4l2_buf.sequence == (ctx->qsequence - 1) never happens.

Signed-off-by Zahari Doychev <[email protected]>

Hi Zahari,

Am Mittwoch, den 08.07.2015, 15:37 +0200 schrieb Zahari Doychev:

Quoted 5 lines

The coda context queue sequence counter is incremented only if the vb2 source buffer payload is non zero. This makes possible to signal EOS otherwise the condition in coda_buf_is_end_of_stream is never met or more precisely buf->v4l2_buf.sequence == (ctx->qsequence - 1) never happens.

Quoted 1 line

Signed-off-by Zahari Doychev <[email protected]>

I think we should instead avoid calling coda_bitstream_queue with zero payload buffers altogether and dump them in coda_fill_bitstream already.

Quoted 10 lines

Hi Zahari,

Am Mittwoch, den 08.07.2015, 15:37 +0200 schrieb Zahari Doychev:

Quoted 6 lines

The coda context queue sequence counter is incremented only if the vb2 source buffer payload is non zero. This makes possible to signal EOS otherwise the condition in coda_buf_is_end_of_stream is never met or more precisely buf->v4l2_buf.sequence == (ctx->qsequence - 1) never happens.

Signed-off-by Zahari Doychev <[email protected]>

Quoted 2 lines

I think we should instead avoid calling coda_bitstream_queue with zero payload buffers altogether and dump them in coda_fill_bitstream already.

Philipp, is this still outstanding or did you fix this already according to the suggestion you made above?

Just wondering whether to set this bug report to 'Rejected' or 'Changes Requested'.

Am Dienstag, den 28.07.2015, 10:54 +0200 schrieb Hans Verkuil:

Quoted 14 lines · Philipp Zabel

Quoted 12 lines

Hi Zahari,

Am Mittwoch, den 08.07.2015, 15:37 +0200 schrieb Zahari Doychev:

Quoted 6 lines

The coda context queue sequence counter is incremented only if the vb2 source buffer payload is non zero. This makes possible to signal EOS otherwise the condition in coda_buf_is_end_of_stream is never met or more precisely buf->v4l2_buf.sequence == (ctx->qsequence - 1) never happens.

Signed-off-by Zahari Doychev <[email protected]>

I think we should instead avoid calling coda_bitstream_queue with zero payload buffers altogether and dump them in coda_fill_bitstream already.

Quoted 5 lines

Philipp, is this still outstanding or did you fix this already according to the suggestion you made above?

Just wondering whether to set this bug report to 'Rejected' or 'Changes Requested'.

Changes requested.

Is this something I should do myself for coda patches in the future?

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This commit fixes bad unlock balance when polling. v4l2_m2m_poll is called with mutex hold but the function releases the mutex and returns. This leads to the bad unlock because after the call of v4l2_m2m_poll in v4l2_m2m_fop_poll the mutex is again unlocked. This patch makes sure that the v4l2_m2m_poll returns always with balanced locks.

[  144.990873] =====================================
[  144.995584] [ BUG: bad unlock balance detected! ]
[  145.000301] 4.1.0-00137-ga105070 #98 Tainted: G        W
[  145.006140] -------------------------------------
[  145.010851] demux:sink/487 is trying to release lock (&dev->dev_mutex) at:
[  145.017785] [<808cc578>] mutex_unlock+0x18/0x1c
[  145.022322] but there are no more locks to release!
[  145.027205]
[  145.027205] other info that might help us debug this:
[  145.033741] no locks held by demux:sink/487.
[  145.038015]
[  145.038015] stack backtrace:
[  145.042385] CPU: 2 PID: 487 Comm: demux:sink Tainted: G        W       4.1.0-00137-ga105070 #98
[  145.051089] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  145.057622] Backtrace:
[  145.060102] [<80014a4c>] (dump_backtrace) from [<80014cc4>] (show_stack+0x20/0x24)
[  145.067679]  r6:80cedf78 r5:00000000 r4:00000000 r3:00000000
[  145.073421] [<80014ca4>] (show_stack) from [<808c61e0>] (dump_stack+0x8c/0xa4)
[  145.080661] [<808c6154>] (dump_stack) from [<80072b64>] (print_unlock_imbalance_bug+0xb8/0xe8)
[  145.089277]  r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:00000001
[  145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>] (lock_release+0x1a4/0x250)
[  145.103983]  r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:00000000
[  145.109728] [<80077c10>] (lock_release) from [<808cc470>] (__mutex_unlock_slowpath+0xc4/0x1b4)
[  145.118344]  r9:acb27a41 r8:00000000 r7:81553814 r6:808cc578 r5:60030013 r4:ac6cd01c
[  145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>] (mutex_unlock+0x18/0x1c)
[  145.134720]  r7:00000000 r6:aced7cd4 r5:00000041 r4:acb87800
[  145.140468] [<808cc560>] (mutex_unlock) from [<805a98b8>] (v4l2_m2m_fop_poll+0x5c/0x64)
[  145.148494] [<805a985c>] (v4l2_m2m_fop_poll) from [<805955a0>] (v4l2_poll+0x6c/0xa0)
[  145.156243]  r6:aced7bec r5:00000000 r4:ac6cc380 r3:805a985c
[  145.161991] [<80595534>] (v4l2_poll) from [<80156edc>] (do_sys_poll+0x230/0x4c0)
[  145.169391]  r5:00000000 r4:aced7be4
[  145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>] (SyS_ppoll+0x1d4/0x1fc)
[  145.180414]  r10:00000000 r9:aced6000 r8:00000000 r7:00000000 r6:75c04538 r5:00000002
[  145.188338]  r4:00000000
[  145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>] (ret_fast_syscall+0x0/0x54)
[  145.198481]  r8:80010aa4 r7:00000150 r6:75c04538 r5:00000002 r4:00000008

Signed-off-by Zahari Doychev <[email protected]>

---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/media/v4l2-core/v4l2-mem2mem.c +10 -9

index dc853e5..5392fb4 100644
@@ -583,16 +583,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
 	if (list_empty(&src_q->done_list))
 		poll_wait(file, &src_q->done_wq, wait);
-	if (list_empty(&dst_q->done_list)) {
-		/*
-		 * If the last buffer was dequeued from the capture queue,
-		 * return immediately. DQBUF will return -EPIPE.
-		 */
-		if (dst_q->last_buffer_dequeued)
-			return rc | POLLIN | POLLRDNORM;
-
+	if (list_empty(&dst_q->done_list) && !dst_q->last_buffer_dequeued)
 		poll_wait(file, &dst_q->done_wq, wait);
-	}
 
 	if (m2m_ctx->m2m_dev->m2m_ops->lock)
 		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);

@@ -603,6 +595,15 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 		}
 	}
 
+	if (list_empty(&dst_q->done_list) && dst_q->last_buffer_dequeued) {
+		/*
+		 * If the last buffer was dequeued from the capture queue,
+		 * return immediately. DQBUF will return -EPIPE.
+		 */
+		rc |= POLLIN | POLLRDNORM;
+		goto end;
+	}
+
 	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,

Kamil, Marek,

Why does v4l2_m2m_poll unlock and lock in that function?

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.

Regards,

Hans

Quoted 74 lines · Zahari Doychev

This commit fixes bad unlock balance when polling. v4l2_m2m_poll is called with mutex hold but the function releases the mutex and returns. This leads to the bad unlock because after the call of v4l2_m2m_poll in v4l2_m2m_fop_poll the mutex is again unlocked. This patch makes sure that the v4l2_m2m_poll returns always with balanced locks.

[  144.990873] =====================================
[  144.995584] [ BUG: bad unlock balance detected! ]
[  145.000301] 4.1.0-00137-ga105070 #98 Tainted: G        W
[  145.006140] -------------------------------------
[  145.010851] demux:sink/487 is trying to release lock (&dev->dev_mutex) at:
[  145.017785] [<808cc578>] mutex_unlock+0x18/0x1c
[  145.022322] but there are no more locks to release!
[  145.027205]
[  145.027205] other info that might help us debug this:
[  145.033741] no locks held by demux:sink/487.
[  145.038015]
[  145.038015] stack backtrace:
[  145.042385] CPU: 2 PID: 487 Comm: demux:sink Tainted: G        W       4.1.0-00137-ga105070 #98
[  145.051089] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  145.057622] Backtrace:
[  145.060102] [<80014a4c>] (dump_backtrace) from [<80014cc4>] (show_stack+0x20/0x24)
[  145.067679]  r6:80cedf78 r5:00000000 r4:00000000 r3:00000000
[  145.073421] [<80014ca4>] (show_stack) from [<808c61e0>] (dump_stack+0x8c/0xa4)
[  145.080661] [<808c6154>] (dump_stack) from [<80072b64>] (print_unlock_imbalance_bug+0xb8/0xe8)
[  145.089277]  r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:00000001
[  145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>] (lock_release+0x1a4/0x250)
[  145.103983]  r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:00000000
[  145.109728] [<80077c10>] (lock_release) from [<808cc470>] (__mutex_unlock_slowpath+0xc4/0x1b4)
[  145.118344]  r9:acb27a41 r8:00000000 r7:81553814 r6:808cc578 r5:60030013 r4:ac6cd01c
[  145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>] (mutex_unlock+0x18/0x1c)
[  145.134720]  r7:00000000 r6:aced7cd4 r5:00000041 r4:acb87800
[  145.140468] [<808cc560>] (mutex_unlock) from [<805a98b8>] (v4l2_m2m_fop_poll+0x5c/0x64)
[  145.148494] [<805a985c>] (v4l2_m2m_fop_poll) from [<805955a0>] (v4l2_poll+0x6c/0xa0)
[  145.156243]  r6:aced7bec r5:00000000 r4:ac6cc380 r3:805a985c
[  145.161991] [<80595534>] (v4l2_poll) from [<80156edc>] (do_sys_poll+0x230/0x4c0)
[  145.169391]  r5:00000000 r4:aced7be4
[  145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>] (SyS_ppoll+0x1d4/0x1fc)
[  145.180414]  r10:00000000 r9:aced6000 r8:00000000 r7:00000000 r6:75c04538 r5:00000002
[  145.188338]  r4:00000000
[  145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>] (ret_fast_syscall+0x0/0x54)
[  145.198481]  r8:80010aa4 r7:00000150 r6:75c04538 r5:00000002 r4:00000008

Signed-off-by Zahari Doychev <[email protected]>

---
  drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++++++++++---------
  1 file changed, 10 insertions(+), 9 deletions(-)

drivers/media/v4l2-core/v4l2-mem2mem.c +1 -9

index dc853e5..5392fb4 100644
@@ -583,16 +583,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
  	if (list_empty(&src_q->done_list))
  		poll_wait(file, &src_q->done_wq, wait);
-	if (list_empty(&dst_q->done_list)) {
-		/*
-		 * If the last buffer was dequeued from the capture queue,
-		 * return immediately. DQBUF will return -EPIPE.
-		 */
-		if (dst_q->last_buffer_dequeued)
-			return rc | POLLIN | POLLRDNORM;
-
+	if (list_empty(&dst_q->done_list) && !dst_q->last_buffer_dequeued)
  		poll_wait(file, &dst_q->done_wq, wait);
-	}
 
  	if (m2m_ctx->m2m_dev->m2m_ops->lock)
  		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);

@@ -603,6 +595,15 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
  		}
  	}
 

drivers/media/v4l2-core/v4l2-mem2mem.c unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx, · line 609

Quoted 13 lines

drivers/media/v4l2-core/v4l2-mem2mem.c +9 -0

+	if (list_empty(&dst_q->done_list) && dst_q->last_buffer_dequeued) {
+		/*
+		 * If the last buffer was dequeued from the capture queue,
+		 * return immediately. DQBUF will return -EPIPE.
+		 */
+		rc |= POLLIN | POLLRDNORM;
+		goto end;
+	}
+
  	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,

Hello Hans,

I'm sorry for a delay. Once again I've been busy with some other internal stuff.

Quoted 4 lines · Hans Verkuil

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.

Quoted 9 lines

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.

Hi,

Quoted 29 lines · Marek Szyprowski

Hello Hans,

I'm sorry for a delay. Once again I've been busy with some other internal stuff.

Quoted 4 lines · Hans Verkuil

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.

Quoted 9 lines

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.

Quoted 10 lines

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.

Quoted 43 lines · Kamil Debski

Hi,

Quoted 39 lines · Marek Szyprowski

Hello Hans,

I'm sorry for a delay. Once again I've been busy with some other internal stuff.

Quoted 4 lines · Hans Verkuil

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.

Quoted 9 lines

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.

Quoted 3 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This commit fixes bad unlock balance when polling. v4l2_m2m_poll is called with mutex hold but the function releases the mutex and returns. This leads to the bad unlock because after the call of v4l2_m2m_poll in v4l2_m2m_fop_poll the mutex is again unlocked. This patch makes sure that the v4l2_m2m_poll returns always with balanced locks.

[  144.990873] =====================================
[  144.995584] [ BUG: bad unlock balance detected! ]
[  145.000301] 4.1.0-00137-ga105070 #98 Tainted: G        W
[  145.006140] -------------------------------------
[  145.010851] demux:sink/487 is trying to release lock (&dev->dev_mutex) at:
[  145.017785] [<808cc578>] mutex_unlock+0x18/0x1c
[  145.022322] but there are no more locks to release!
[  145.027205]
[  145.027205] other info that might help us debug this:
[  145.033741] no locks held by demux:sink/487.
[  145.038015]
[  145.038015] stack backtrace:
[  145.042385] CPU: 2 PID: 487 Comm: demux:sink Tainted: G        W       4.1.0-00137-ga105070 #98
[  145.051089] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  145.057622] Backtrace:
[  145.060102] [<80014a4c>] (dump_backtrace) from [<80014cc4>] (show_stack+0x20/0x24)
[  145.067679]  r6:80cedf78 r5:00000000 r4:00000000 r3:00000000
[  145.073421] [<80014ca4>] (show_stack) from [<808c61e0>] (dump_stack+0x8c/0xa4)
[  145.080661] [<808c6154>] (dump_stack) from [<80072b64>] (print_unlock_imbalance_bug+0xb8/0xe8)
[  145.089277]  r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:00000001
[  145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>] (lock_release+0x1a4/0x250)
[  145.103983]  r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:00000000
[  145.109728] [<80077c10>] (lock_release) from [<808cc470>] (__mutex_unlock_slowpath+0xc4/0x1b4)
[  145.118344]  r9:acb27a41 r8:00000000 r7:81553814 r6:808cc578 r5:60030013 r4:ac6cd01c
[  145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>] (mutex_unlock+0x18/0x1c)
[  145.134720]  r7:00000000 r6:aced7cd4 r5:00000041 r4:acb87800
[  145.140468] [<808cc560>] (mutex_unlock) from [<805a98b8>] (v4l2_m2m_fop_poll+0x5c/0x64)
[  145.148494] [<805a985c>] (v4l2_m2m_fop_poll) from [<805955a0>] (v4l2_poll+0x6c/0xa0)
[  145.156243]  r6:aced7bec r5:00000000 r4:ac6cc380 r3:805a985c
[  145.161991] [<80595534>] (v4l2_poll) from [<80156edc>] (do_sys_poll+0x230/0x4c0)
[  145.169391]  r5:00000000 r4:aced7be4
[  145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>] (SyS_ppoll+0x1d4/0x1fc)
[  145.180414]  r10:00000000 r9:aced6000 r8:00000000 r7:00000000 r6:75c04538 r5:00000002
[  145.188338]  r4:00000000
[  145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>] (ret_fast_syscall+0x0/0x54)
[  145.198481]  r8:80010aa4 r7:00000150 r6:75c04538 r5:00000002 r4:00000008

Signed-off-by Zahari Doychev <[email protected]>

---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/media/v4l2-core/v4l2-mem2mem.c +3 -9

index dc853e5..5392fb4 100644
@@ -583,16 +583,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
 	if (list_empty(&src_q->done_list))
 		poll_wait(file, &src_q->done_wq, wait);
-	if (list_empty(&dst_q->done_list)) {
-		/*
-		 * If the last buffer was dequeued from the capture queue,
-		 * return immediately. DQBUF will return -EPIPE.
-		 */
-		if (dst_q->last_buffer_dequeued)
-			return rc | POLLIN | POLLRDNORM;
-
+	if (list_empty(&dst_q->done_list) && !dst_q->last_buffer_dequeued)
 		poll_wait(file, &dst_q->done_wq, wait);
-	}
 
 	if (m2m_ctx->m2m_dev->m2m_ops->lock)
 		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);

@@ -603,6 +595,15 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 		}
 	}
 
+	if (list_empty(&dst_q->done_list) && dst_q->last_buffer_dequeued) {
+		/*