[PATCH 2/2] [media] m2m: fix bad unlock balance
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: GW
[ 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: GW
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: r4: r3:
[ 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:0001
[ 145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>]
(lock_release+0x1a4/0x250)
[ 145.103983] r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:
[ 145.109728] [<80077c10>] (lock_release) from [<808cc470>]
(__mutex_unlock_slowpath+0xc4/0x1b4)
[ 145.118344] r9:acb27a41 r8: r7:81553814 r6:808cc578 r5:60030013
r4:ac6cd01c
[ 145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>]
(mutex_unlock+0x18/0x1c)
[ 145.134720] r7: r6:aced7cd4 r5:0041 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: r4:ac6cc380 r3:805a985c
[ 145.161991] [<80595534>] (v4l2_poll) from [<80156edc>]
(do_sys_poll+0x230/0x4c0)
[ 145.169391] r5: r4:aced7be4
[ 145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>]
(SyS_ppoll+0x1d4/0x1fc)
[ 145.180414] r10: r9:aced6000 r8: r7: r6:75c04538
r5:0002
[ 145.188338] r4:
[ 145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>]
(ret_fast_syscall+0x0/0x54)
[ 145.198481] r8:80010aa4 r7:0150 r6:75c04538 r5:0002 r4:0008
Signed-off-by: Zahari Doychev
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index dc853e5..5392fb4 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -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,
--
2.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] m2m: fix bad unlock balance
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
On 07/08/2015 03:37 PM, Zahari Doychev wrote:
> 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: GW
> [ 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: GW
> 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: r4: r3:
> [ 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:0001
> [ 145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>]
> (lock_release+0x1a4/0x250)
> [ 145.103983] r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:
> [ 145.109728] [<80077c10>] (lock_release) from [<808cc470>]
> (__mutex_unlock_slowpath+0xc4/0x1b4)
> [ 145.118344] r9:acb27a41 r8: r7:81553814 r6:808cc578 r5:60030013
> r4:ac6cd01c
> [ 145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>]
> (mutex_unlock+0x18/0x1c)
> [ 145.134720] r7: r6:aced7cd4 r5:0041 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: r4:ac6cc380 r3:805a985c
> [ 145.161991] [<80595534>] (v4l2_poll) from [<80156edc>]
> (do_sys_poll+0x230/0x4c0)
> [ 145.169391] r5: r4:aced7be4
> [ 145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>]
> (SyS_ppoll+0x1d4/0x1fc)
> [ 145.180414] r10: r9:aced6000 r8: r7: r6:75c04538
> r5:0002
> [ 145.188338] r4:
> [ 145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>]
> (ret_fast_syscall+0x0/0x54)
> [ 145.198481] r8:80010aa4 r7:0150 r6:75c04538 r5:0002 r4:0008
>
> Signed-off-by: Zahari Doychev
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++-
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index dc853e5..5392fb4 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -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;
> + }
Re: [PATCH 2/2] [media] m2m: fix bad unlock balance
(sent again, this time with Kamil's new email)
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
On 07/08/2015 03:37 PM, Zahari Doychev wrote:
> 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: GW
> [ 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: GW
> 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: r4: r3:
> [ 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:0001
> [ 145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>]
> (lock_release+0x1a4/0x250)
> [ 145.103983] r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:
> [ 145.109728] [<80077c10>] (lock_release) from [<808cc470>]
> (__mutex_unlock_slowpath+0xc4/0x1b4)
> [ 145.118344] r9:acb27a41 r8: r7:81553814 r6:808cc578 r5:60030013
> r4:ac6cd01c
> [ 145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>]
> (mutex_unlock+0x18/0x1c)
> [ 145.134720] r7: r6:aced7cd4 r5:0041 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: r4:ac6cc380 r3:805a985c
> [ 145.161991] [<80595534>] (v4l2_poll) from [<80156edc>]
> (do_sys_poll+0x230/0x4c0)
> [ 145.169391] r5: r4:aced7be4
> [ 145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>]
> (SyS_ppoll+0x1d4/0x1fc)
> [ 145.180414] r10: r9:aced6000 r8: r7: r6:75c04538
> r5:0002
> [ 145.188338] r4:
> [ 145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>]
> (ret_fast_syscall+0x0/0x54)
> [ 145.198481] r8:80010aa4 r7:0150 r6:75c04538 r5:0002 r4:0008
>
> Signed-off-by: Zahari Doychev
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++-
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index dc853e5..5392fb4 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -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 |
Re: [PATCH 2/2] [media] m2m: fix bad unlock balance
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.
Regards,
Hans
On 07/08/2015 03:37 PM, Zahari Doychev wrote:
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: GW
[ 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: GW
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: r4: r3:
[ 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:0001
[ 145.095020] [<80072aac>] (print_unlock_imbalance_bug) from [<80077db4>]
(lock_release+0x1a4/0x250)
[ 145.103983] r6:808cc578 r5:ac6cd050 r4:ac38e400 r3:
[ 145.109728] [<80077c10>] (lock_release) from [<808cc470>]
(__mutex_unlock_slowpath+0xc4/0x1b4)
[ 145.118344] r9:acb27a41 r8: r7:81553814 r6:808cc578 r5:60030013
r4:ac6cd01c
[ 145.126190] [<808cc3ac>] (__mutex_unlock_slowpath) from [<808cc578>]
(mutex_unlock+0x18/0x1c)
[ 145.134720] r7: r6:aced7cd4 r5:0041 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: r4:ac6cc380 r3:805a985c
[ 145.161991] [<80595534>] (v4l2_poll) from [<80156edc>]
(do_sys_poll+0x230/0x4c0)
[ 145.169391] r5: r4:aced7be4
[ 145.173013] [<80156cac>] (do_sys_poll) from [<801574a8>]
(SyS_ppoll+0x1d4/0x1fc)
[ 145.180414] r10: r9:aced6000 r8: r7: r6:75c04538
r5:0002
[ 145.188338] r4:
[ 145.190906] [<801572d4>] (SyS_ppoll) from [<800108c0>]
(ret_fast_syscall+0x0/0x54)
[ 145.198481] r8:80010aa4 r7:0150 r6:75c04538 r5:0002 r4:0008
Signed-off-by: Zahari Doychev
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index dc853e5..5392fb4 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -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, &
Re: [PATCH 2/2] [media] m2m: fix bad unlock balance
Hi,
On 12 August 2015 at 13:42, Marek Szyprowski 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.
[snip]
Best wishes,
Kamil
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] m2m: fix bad unlock balance
On 08/12/2015 05:50 PM, Kamil Debski wrote:
> Hi,
>
> On 12 August 2015 at 13:42, Marek Szyprowski 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?
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] m2m: fix bad unlock balance
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
> > 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
