[PATCH 0/2] fix lockdep warnings in s5p_mfc and exynos-gsc vb2 drivers

[PATCH 0/2] fix lockdep warnings in s5p_mfc and exynos-gsc vb2 drivers

[Shuah Khan]

Driver mmap functions shouldn't hold lock when calling vb2_mmap(). The
vb2_mmap() function has its own lock that it uses to protect the critical
section.

Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3

Shuah Khan (2):
  media: exynos-gsc: fix lockdep warning
  media: s5p-mfc: fix lockdep warning

 drivers/media/platform/exynos-gsc/gsc-m2m.c | 5 -----
 drivers/media/platform/s5p-mfc/s5p_mfc.c    | 3 ---
 2 files changed, 8 deletions(-)

-- 
2.7.4

[PATCH 1/2] media: exynos-gsc: fix lockdep warning

[Shuah Khan]

The driver mmap functions shouldn't take lock when calling vb2_mmap().
Fix it to not take the lock. The following lockdep warning is fixed
with this change.

[ 1990.972058] ======================================================
[ 1990.978172] WARNING: possible circular locking dependency detected
[ 1990.984327] 4.14.0-rc2-00002-gfab205f-dirty #4 Not tainted
[ 1990.989783] ------------------------------------------------------
[ 1990.995937] qtdemux0:sink/2765 is trying to acquire lock:
[ 1991.001309]  (&gsc->lock){+.+.}, at: [<bf1729f0>] gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
[ 1991.009108]
               but task is already holding lock:
[ 1991.014913]  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
[ 1991.021932]
               which lock already depends on the new lock.

[ 1991.030078]
               the existing dependency chain (in reverse order) is:
[ 1991.037530]
               -> #1 (&mm->mmap_sem){++++}:
[ 1991.042913]        __might_fault+0x80/0xb0
[ 1991.047096]        video_usercopy+0x1cc/0x510 [videodev]
[ 1991.052297]        v4l2_ioctl+0xa4/0xdc [videodev]
[ 1991.057036]        do_vfs_ioctl+0xa0/0xa18
[ 1991.061102]        SyS_ioctl+0x34/0x5c
[ 1991.064834]        ret_fast_syscall+0x0/0x28
[ 1991.069072]
               -> #0 (&gsc->lock){+.+.}:
[ 1991.074193]        lock_acquire+0x6c/0x88
[ 1991.078179]        __mutex_lock+0x68/0xa34
[ 1991.082247]        mutex_lock_interruptible_nested+0x1c/0x24
[ 1991.087888]        gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
[ 1991.093029]        v4l2_mmap+0x54/0x88 [videodev]
[ 1991.097673]        mmap_region+0x3a8/0x638
[ 1991.101743]        do_mmap+0x330/0x3a4
[ 1991.105470]        vm_mmap_pgoff+0x90/0xb8
[ 1991.109542]        SyS_mmap_pgoff+0x90/0xc0
[ 1991.113702]        ret_fast_syscall+0x0/0x28
[ 1991.117945]
               other info that might help us debug this:

[ 1991.125918]  Possible unsafe locking scenario:

[ 1991.131810]        CPU0                    CPU1
[ 1991.136315]        ----                    ----
[ 1991.140821]   lock(&mm->mmap_sem);
[ 1991.144201]                                lock(&gsc->lock);
[ 1991.149833]                                lock(&mm->mmap_sem);
[ 1991.155725]   lock(&gsc->lock);
[ 1991.158845]
                *** DEADLOCK ***

[ 1991.164740] 1 lock held by qtdemux0:sink/2765:
[ 1991.169157]  #0:  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
[ 1991.176609]
               stack backtrace:
[ 1991.180946] CPU: 2 PID: 2765 Comm: qtdemux0:sink Not tainted 4.14.0-rc2-00002-gfab205f-dirty #4
[ 1991.189608] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 1991.195686] [<c01102c8>] (unwind_backtrace) from [<c010cabc>] (show_stack+0x10/0x14)
[ 1991.203393] [<c010cabc>] (show_stack) from [<c08543a4>] (dump_stack+0x98/0xc4)
[ 1991.210586] [<c08543a4>] (dump_stack) from [<c016b2fc>] (print_circular_bug+0x254/0x410)
[ 1991.218644] [<c016b2fc>] (print_circular_bug) from [<c016c580>] (check_prev_add+0x468/0x938)
[ 1991.227049] [<c016c580>] (check_prev_add) from [<c016f4dc>] (__lock_acquire+0x1314/0x14fc)
[ 1991.235281] [<c016f4dc>] (__lock_acquire) from [<c016fefc>] (lock_acquire+0x6c/0x88)
[ 1991.242993] [<c016fefc>] (lock_acquire) from [<c0869fb4>] (__mutex_lock+0x68/0xa34)
[ 1991.250620] [<c0869fb4>] (__mutex_lock) from [<c086aa08>] (mutex_lock_interruptible_nested+0x1c/0x24)
[ 1991.259812] [<c086aa08>] (mutex_lock_interruptible_nested) from [<bf1729f0>] (gsc_m2m_mmap+0x24/0x5c [exynos_gsc])
[ 1991.270159] [<bf1729f0>] (gsc_m2m_mmap [exynos_gsc]) from [<bf037120>] (v4l2_mmap+0x54/0x88 [videodev])
[ 1991.279510] [<bf037120>] (v4l2_mmap [videodev]) from [<c01f4798>] (mmap_region+0x3a8/0x638)
[ 1991.287792] [<c01f4798>] (mmap_region) from [<c01f4d58>] (do_mmap+0x330/0x3a4)
[ 1991.294986] [<c01f4d58>] (do_mmap) from [<c01df330>] (vm_mmap_pgoff+0x90/0xb8)
[ 1991.302178] [<c01df330>] (vm_mmap_pgoff) from [<c01f28cc>] (SyS_mmap_pgoff+0x90/0xc0)
[ 1991.309977] [<c01f28cc>] (SyS_mmap_pgoff) from [<c0108820>] (ret_fast_syscall+0x0/0x28)

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
Suggested-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/platform/exynos-gsc/gsc-m2m.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 2a2994e..722d7c4 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -726,14 +726,9 @@ static unsigned int gsc_m2m_poll(struct file *file,
 static int gsc_m2m_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct gsc_ctx *ctx = fh_to_ctx(file->private_data);
-	struct gsc_dev *gsc = ctx->gsc_dev;
 	int ret;
 
-	if (mutex_lock_interruptible(&gsc->lock))
-		return -ERESTARTSYS;
-
 	ret = v4l2_m2m_mmap(file, ctx->m2m_ctx, vma);
-	mutex_unlock(&gsc->lock);
 
 	return ret;
 }
-- 
2.7.4

[PATCH 2/2] media: s5p-mfc: fix lockdep warning

[Shuah Khan]

The driver mmap functions shouldn't take lock when calling vb2_mmap().
Fix it to not take the lock. The following lockdep warning is fixed
with this change.

[ 2106.181412] ======================================================
[ 2106.187563] WARNING: possible circular locking dependency detected
[ 2106.193718] 4.14.0-rc2-00002-gfab205f-dirty #4 Not tainted
[ 2106.199175] ------------------------------------------------------
[ 2106.205328] qtdemux0:sink/2614 is trying to acquire lock:
[ 2106.210701]  (&dev->mfc_mutex){+.+.}, at: [<bf175544>] s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
[ 2106.218672]
[ 2106.218672] but task is already holding lock:
[ 2106.224477]  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
[ 2106.231497]
[ 2106.231497] which lock already depends on the new lock.
[ 2106.231497]
[ 2106.239642]
[ 2106.239642] the existing dependency chain (in reverse order) is:
[ 2106.247095]
[ 2106.247095] -> #1 (&mm->mmap_sem){++++}:
[ 2106.252473]        __might_fault+0x80/0xb0
[ 2106.256567]        video_usercopy+0x1cc/0x510 [videodev]
[ 2106.261845]        v4l2_ioctl+0xa4/0xdc [videodev]
[ 2106.266596]        do_vfs_ioctl+0xa0/0xa18
[ 2106.270667]        SyS_ioctl+0x34/0x5c
[ 2106.274395]        ret_fast_syscall+0x0/0x28
[ 2106.278637]
[ 2106.278637] -> #0 (&dev->mfc_mutex){+.+.}:
[ 2106.284186]        lock_acquire+0x6c/0x88
[ 2106.288173]        __mutex_lock+0x68/0xa34
[ 2106.292244]        mutex_lock_interruptible_nested+0x1c/0x24
[ 2106.297893]        s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
[ 2106.302747]        v4l2_mmap+0x54/0x88 [videodev]
[ 2106.307409]        mmap_region+0x3a8/0x638
[ 2106.311480]        do_mmap+0x330/0x3a4
[ 2106.315207]        vm_mmap_pgoff+0x90/0xb8
[ 2106.319279]        SyS_mmap_pgoff+0x90/0xc0
[ 2106.323439]        ret_fast_syscall+0x0/0x28
[ 2106.327683]
[ 2106.327683] other info that might help us debug this:
[ 2106.327683]
[ 2106.335656]  Possible unsafe locking scenario:
[ 2106.335656]
[ 2106.341548]        CPU0                    CPU1
[ 2106.346053]        ----                    ----
[ 2106.350559]   lock(&mm->mmap_sem);
[ 2106.353939]                                lock(&dev->mfc_mutex);
[ 2106.353939]                                lock(&dev->mfc_mutex);
[ 2106.365897]   lock(&dev->mfc_mutex);
[ 2106.369450]
[ 2106.369450]  *** DEADLOCK ***
[ 2106.369450]
[ 2106.375344] 1 lock held by qtdemux0:sink/2614:
[ 2106.379762]  #0:  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
[ 2106.387214]
[ 2106.387214] stack backtrace:
[ 2106.391550] CPU: 7 PID: 2614 Comm: qtdemux0:sink Not tainted 4.14.0-rc2-00002-gfab205f-dirty #4
[ 2106.400213] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 2106.406285] [<c01102c8>] (unwind_backtrace) from [<c010cabc>] (show_stack+0x10/0x14)
[ 2106.413995] [<c010cabc>] (show_stack) from [<c08543a4>] (dump_stack+0x98/0xc4)
[ 2106.421187] [<c08543a4>] (dump_stack) from [<c016b2fc>] (print_circular_bug+0x254/0x410)
[ 2106.429245] [<c016b2fc>] (print_circular_bug) from [<c016c580>] (check_prev_add+0x468/0x938)
[ 2106.437651] [<c016c580>] (check_prev_add) from [<c016f4dc>] (__lock_acquire+0x1314/0x14fc)
[ 2106.445883] [<c016f4dc>] (__lock_acquire) from [<c016fefc>] (lock_acquire+0x6c/0x88)
[ 2106.453596] [<c016fefc>] (lock_acquire) from [<c0869fb4>] (__mutex_lock+0x68/0xa34)
[ 2106.461221] [<c0869fb4>] (__mutex_lock) from [<c086aa08>] (mutex_lock_interruptible_nested+0x1c/0x24)
[ 2106.470425] [<c086aa08>] (mutex_lock_interruptible_nested) from [<bf175544>] (s5p_mfc_mmap+0x28/0xd4 [s5p_mfc])
[ 2106.480494] [<bf175544>] (s5p_mfc_mmap [s5p_mfc]) from [<bf037120>] (v4l2_mmap+0x54/0x88 [videodev])
[ 2106.489575] [<bf037120>] (v4l2_mmap [videodev]) from [<c01f4798>] (mmap_region+0x3a8/0x638)
[ 2106.497875] [<c01f4798>] (mmap_region) from [<c01f4d58>] (do_mmap+0x330/0x3a4)
[ 2106.505068] [<c01f4d58>] (do_mmap) from [<c01df330>] (vm_mmap_pgoff+0x90/0xb8)
[ 2106.512260] [<c01df330>] (vm_mmap_pgoff) from [<c01f28cc>] (SyS_mmap_pgoff+0x90/0xc0)
[ 2106.520059] [<c01f28cc>] (SyS_mmap_pgoff) from [<c0108820>] (ret_fast_syscall+0x0/0x28)

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
Suggested-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 4c253fb..40c18b4 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1047,8 +1047,6 @@ static int s5p_mfc_mmap(struct file *file, struct vm_area_struct *vma)
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
 	int ret;
 
-	if (mutex_lock_interruptible(&dev->mfc_mutex))
-		return -ERESTARTSYS;
 	if (offset < DST_QUEUE_OFF_BASE) {
 		mfc_debug(2, "mmaping source\n");
 		ret = vb2_mmap(&ctx->vq_src, vma);
@@ -1057,7 +1055,6 @@ static int s5p_mfc_mmap(struct file *file, struct vm_area_struct *vma)
 		vma->vm_pgoff -= (DST_QUEUE_OFF_BASE >> PAGE_SHIFT);
 		ret = vb2_mmap(&ctx->vq_dst, vma);
 	}
-	mutex_unlock(&dev->mfc_mutex);
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH 0/2] fix lockdep warnings in s5p_mfc and exynos-gsc vb2 drivers

[Marek Szyprowski]

Hi Shuah,

On 2017-10-14 01:13, Shuah Khan wrote:
> Driver mmap functions shouldn't hold lock when calling vb2_mmap(). The
> vb2_mmap() function has its own lock that it uses to protect the critical
> section.
>
> Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3

It would make sense to add the information about the reference commit to 
each
commit message and also point to commit 
e752577ed7bf55c81e10343fced8b378cda2b63b,
as it is exactly the same case here. Anyway:

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

I wonder if makes sense to send those patches also to stable@vget.kernel.org
(maybe v4.3+, like the mentioned above commit, if they really apply?).

> Shuah Khan (2):
>    media: exynos-gsc: fix lockdep warning
>    media: s5p-mfc: fix lockdep warning
>
>   drivers/media/platform/exynos-gsc/gsc-m2m.c | 5 -----
>   drivers/media/platform/s5p-mfc/s5p_mfc.c    | 3 ---
>   2 files changed, 8 deletions(-)
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 0/2] fix lockdep warnings in s5p_mfc and exynos-gsc vb2 drivers

[Shuah Khan]

Hi Marek,

On 10/16/2017 06:48 AM, Marek Szyprowski wrote:
> Hi Shuah,
> 
> On 2017-10-14 01:13, Shuah Khan wrote:
>> Driver mmap functions shouldn't hold lock when calling vb2_mmap(). The
>> vb2_mmap() function has its own lock that it uses to protect the critical
>> section.
>>
>> Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3
> 
> It would make sense to add the information about the reference commit to each
> commit message and also point to commit e752577ed7bf55c81e10343fced8b378cda2b63b,
> as it is exactly the same case here. Anyway:

I think It does make sense to add the commit information to each commit message.
I can do that send v2.

> 
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks.

> 
> I wonder if makes sense to send those patches also to stable@vget.kernel.org
> (maybe v4.3+, like the mentioned above commit, if they really apply?).

I don't believe they will apply as is. I can work back-porting once these get into
the mainline.

> 
>> Shuah Khan (2):
>>    media: exynos-gsc: fix lockdep warning
>>    media: s5p-mfc: fix lockdep warning
>>
>>   drivers/media/platform/exynos-gsc/gsc-m2m.c | 5 -----
>>   drivers/media/platform/s5p-mfc/s5p_mfc.c    | 3 ---
>>   2 files changed, 8 deletions(-)
>>
> 
> Best regards

thanks,
-- Shuah

Re: [PATCH v2 1/2] media: exynos-gsc: fix lockdep warning

[Hans Verkuil]

On 10/16/2017 05:16 PM, Shuah Khan wrote:
> The driver mmap functions shouldn't take lock when calling vb2_mmap().
> Fix it to not take the lock.
> 
> Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3
> and e752577ed7bf55c81e10343fced8b378cda2b63b
> 
> The following lockdep warning is fixed with this change.
> 
> [ 1990.972058] ======================================================
> [ 1990.978172] WARNING: possible circular locking dependency detected
> [ 1990.984327] 4.14.0-rc2-00002-gfab205f-dirty #4 Not tainted
> [ 1990.989783] ------------------------------------------------------
> [ 1990.995937] qtdemux0:sink/2765 is trying to acquire lock:
> [ 1991.001309]  (&gsc->lock){+.+.}, at: [<bf1729f0>] gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
> [ 1991.009108]
>                but task is already holding lock:
> [ 1991.014913]  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
> [ 1991.021932]
>                which lock already depends on the new lock.
> 
> [ 1991.030078]
>                the existing dependency chain (in reverse order) is:
> [ 1991.037530]
>                -> #1 (&mm->mmap_sem){++++}:
> [ 1991.042913]        __might_fault+0x80/0xb0
> [ 1991.047096]        video_usercopy+0x1cc/0x510 [videodev]
> [ 1991.052297]        v4l2_ioctl+0xa4/0xdc [videodev]
> [ 1991.057036]        do_vfs_ioctl+0xa0/0xa18
> [ 1991.061102]        SyS_ioctl+0x34/0x5c
> [ 1991.064834]        ret_fast_syscall+0x0/0x28
> [ 1991.069072]
>                -> #0 (&gsc->lock){+.+.}:
> [ 1991.074193]        lock_acquire+0x6c/0x88
> [ 1991.078179]        __mutex_lock+0x68/0xa34
> [ 1991.082247]        mutex_lock_interruptible_nested+0x1c/0x24
> [ 1991.087888]        gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
> [ 1991.093029]        v4l2_mmap+0x54/0x88 [videodev]
> [ 1991.097673]        mmap_region+0x3a8/0x638
> [ 1991.101743]        do_mmap+0x330/0x3a4
> [ 1991.105470]        vm_mmap_pgoff+0x90/0xb8
> [ 1991.109542]        SyS_mmap_pgoff+0x90/0xc0
> [ 1991.113702]        ret_fast_syscall+0x0/0x28
> [ 1991.117945]
>                other info that might help us debug this:
> 
> [ 1991.125918]  Possible unsafe locking scenario:
> 
> [ 1991.131810]        CPU0                    CPU1
> [ 1991.136315]        ----                    ----
> [ 1991.140821]   lock(&mm->mmap_sem);
> [ 1991.144201]                                lock(&gsc->lock);
> [ 1991.149833]                                lock(&mm->mmap_sem);
> [ 1991.155725]   lock(&gsc->lock);
> [ 1991.158845]
>                 *** DEADLOCK ***
> 
> [ 1991.164740] 1 lock held by qtdemux0:sink/2765:
> [ 1991.169157]  #0:  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
> [ 1991.176609]
>                stack backtrace:
> [ 1991.180946] CPU: 2 PID: 2765 Comm: qtdemux0:sink Not tainted 4.14.0-rc2-00002-gfab205f-dirty #4
> [ 1991.189608] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [ 1991.195686] [<c01102c8>] (unwind_backtrace) from [<c010cabc>] (show_stack+0x10/0x14)
> [ 1991.203393] [<c010cabc>] (show_stack) from [<c08543a4>] (dump_stack+0x98/0xc4)
> [ 1991.210586] [<c08543a4>] (dump_stack) from [<c016b2fc>] (print_circular_bug+0x254/0x410)
> [ 1991.218644] [<c016b2fc>] (print_circular_bug) from [<c016c580>] (check_prev_add+0x468/0x938)
> [ 1991.227049] [<c016c580>] (check_prev_add) from [<c016f4dc>] (__lock_acquire+0x1314/0x14fc)
> [ 1991.235281] [<c016f4dc>] (__lock_acquire) from [<c016fefc>] (lock_acquire+0x6c/0x88)
> [ 1991.242993] [<c016fefc>] (lock_acquire) from [<c0869fb4>] (__mutex_lock+0x68/0xa34)
> [ 1991.250620] [<c0869fb4>] (__mutex_lock) from [<c086aa08>] (mutex_lock_interruptible_nested+0x1c/0x24)
> [ 1991.259812] [<c086aa08>] (mutex_lock_interruptible_nested) from [<bf1729f0>] (gsc_m2m_mmap+0x24/0x5c [exynos_gsc])
> [ 1991.270159] [<bf1729f0>] (gsc_m2m_mmap [exynos_gsc]) from [<bf037120>] (v4l2_mmap+0x54/0x88 [videodev])
> [ 1991.279510] [<bf037120>] (v4l2_mmap [videodev]) from [<c01f4798>] (mmap_region+0x3a8/0x638)
> [ 1991.287792] [<c01f4798>] (mmap_region) from [<c01f4d58>] (do_mmap+0x330/0x3a4)
> [ 1991.294986] [<c01f4d58>] (do_mmap) from [<c01df330>] (vm_mmap_pgoff+0x90/0xb8)
> [ 1991.302178] [<c01df330>] (vm_mmap_pgoff) from [<c01f28cc>] (SyS_mmap_pgoff+0x90/0xc0)
> [ 1991.309977] [<c01f28cc>] (SyS_mmap_pgoff) from [<c0108820>] (ret_fast_syscall+0x0/0x28)
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> Suggested-by: Hans Verkuil <hansverk@cisco.com>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

Acked-by: Hans Verkuil <hansverk@cisco.com>

Regards,

	Hans

> ---
>  drivers/media/platform/exynos-gsc/gsc-m2m.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 2a2994e..722d7c4 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -726,14 +726,9 @@ static unsigned int gsc_m2m_poll(struct file *file,
>  static int gsc_m2m_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	struct gsc_ctx *ctx = fh_to_ctx(file->private_data);
> -	struct gsc_dev *gsc = ctx->gsc_dev;
>  	int ret;
>  
> -	if (mutex_lock_interruptible(&gsc->lock))
> -		return -ERESTARTSYS;
> -
>  	ret = v4l2_m2m_mmap(file, ctx->m2m_ctx, vma);
> -	mutex_unlock(&gsc->lock);
>  
>  	return ret;
>  }
> 

Re: [PATCH v2 2/2] media: s5p-mfc: fix lockdep warning

[Hans Verkuil]

On 10/16/2017 05:16 PM, Shuah Khan wrote:
> The driver mmap functions shouldn't take lock when calling vb2_mmap().
> Fix it to not take the lock.
> 
> Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3
> and e752577ed7bf55c81e10343fced8b378cda2b63b
> 
> The following lockdep warning is fixed with this change.
> 
> [ 2106.181412] ======================================================
> [ 2106.187563] WARNING: possible circular locking dependency detected
> [ 2106.193718] 4.14.0-rc2-00002-gfab205f-dirty #4 Not tainted
> [ 2106.199175] ------------------------------------------------------
> [ 2106.205328] qtdemux0:sink/2614 is trying to acquire lock:
> [ 2106.210701]  (&dev->mfc_mutex){+.+.}, at: [<bf175544>] s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
> [ 2106.218672]
> [ 2106.218672] but task is already holding lock:
> [ 2106.224477]  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
> [ 2106.231497]
> [ 2106.231497] which lock already depends on the new lock.
> [ 2106.231497]
> [ 2106.239642]
> [ 2106.239642] the existing dependency chain (in reverse order) is:
> [ 2106.247095]
> [ 2106.247095] -> #1 (&mm->mmap_sem){++++}:
> [ 2106.252473]        __might_fault+0x80/0xb0
> [ 2106.256567]        video_usercopy+0x1cc/0x510 [videodev]
> [ 2106.261845]        v4l2_ioctl+0xa4/0xdc [videodev]
> [ 2106.266596]        do_vfs_ioctl+0xa0/0xa18
> [ 2106.270667]        SyS_ioctl+0x34/0x5c
> [ 2106.274395]        ret_fast_syscall+0x0/0x28
> [ 2106.278637]
> [ 2106.278637] -> #0 (&dev->mfc_mutex){+.+.}:
> [ 2106.284186]        lock_acquire+0x6c/0x88
> [ 2106.288173]        __mutex_lock+0x68/0xa34
> [ 2106.292244]        mutex_lock_interruptible_nested+0x1c/0x24
> [ 2106.297893]        s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
> [ 2106.302747]        v4l2_mmap+0x54/0x88 [videodev]
> [ 2106.307409]        mmap_region+0x3a8/0x638
> [ 2106.311480]        do_mmap+0x330/0x3a4
> [ 2106.315207]        vm_mmap_pgoff+0x90/0xb8
> [ 2106.319279]        SyS_mmap_pgoff+0x90/0xc0
> [ 2106.323439]        ret_fast_syscall+0x0/0x28
> [ 2106.327683]
> [ 2106.327683] other info that might help us debug this:
> [ 2106.327683]
> [ 2106.335656]  Possible unsafe locking scenario:
> [ 2106.335656]
> [ 2106.341548]        CPU0                    CPU1
> [ 2106.346053]        ----                    ----
> [ 2106.350559]   lock(&mm->mmap_sem);
> [ 2106.353939]                                lock(&dev->mfc_mutex);
> [ 2106.353939]                                lock(&dev->mfc_mutex);
> [ 2106.365897]   lock(&dev->mfc_mutex);
> [ 2106.369450]
> [ 2106.369450]  *** DEADLOCK ***
> [ 2106.369450]
> [ 2106.375344] 1 lock held by qtdemux0:sink/2614:
> [ 2106.379762]  #0:  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
> [ 2106.387214]
> [ 2106.387214] stack backtrace:
> [ 2106.391550] CPU: 7 PID: 2614 Comm: qtdemux0:sink Not tainted 4.14.0-rc2-00002-gfab205f-dirty #4
> [ 2106.400213] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [ 2106.406285] [<c01102c8>] (unwind_backtrace) from [<c010cabc>] (show_stack+0x10/0x14)
> [ 2106.413995] [<c010cabc>] (show_stack) from [<c08543a4>] (dump_stack+0x98/0xc4)
> [ 2106.421187] [<c08543a4>] (dump_stack) from [<c016b2fc>] (print_circular_bug+0x254/0x410)
> [ 2106.429245] [<c016b2fc>] (print_circular_bug) from [<c016c580>] (check_prev_add+0x468/0x938)
> [ 2106.437651] [<c016c580>] (check_prev_add) from [<c016f4dc>] (__lock_acquire+0x1314/0x14fc)
> [ 2106.445883] [<c016f4dc>] (__lock_acquire) from [<c016fefc>] (lock_acquire+0x6c/0x88)
> [ 2106.453596] [<c016fefc>] (lock_acquire) from [<c0869fb4>] (__mutex_lock+0x68/0xa34)
> [ 2106.461221] [<c0869fb4>] (__mutex_lock) from [<c086aa08>] (mutex_lock_interruptible_nested+0x1c/0x24)
> [ 2106.470425] [<c086aa08>] (mutex_lock_interruptible_nested) from [<bf175544>] (s5p_mfc_mmap+0x28/0xd4 [s5p_mfc])
> [ 2106.480494] [<bf175544>] (s5p_mfc_mmap [s5p_mfc]) from [<bf037120>] (v4l2_mmap+0x54/0x88 [videodev])
> [ 2106.489575] [<bf037120>] (v4l2_mmap [videodev]) from [<c01f4798>] (mmap_region+0x3a8/0x638)
> [ 2106.497875] [<c01f4798>] (mmap_region) from [<c01f4d58>] (do_mmap+0x330/0x3a4)
> [ 2106.505068] [<c01f4d58>] (do_mmap) from [<c01df330>] (vm_mmap_pgoff+0x90/0xb8)
> [ 2106.512260] [<c01df330>] (vm_mmap_pgoff) from [<c01f28cc>] (SyS_mmap_pgoff+0x90/0xc0)
> [ 2106.520059] [<c01f28cc>] (SyS_mmap_pgoff) from [<c0108820>] (ret_fast_syscall+0x0/0x28)
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> Suggested-by: Hans Verkuil <hansverk@cisco.com>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

Acked-by: Hans Verkuil <hansverk@cisco.com>

Regards,

	Hans

> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 4c253fb..40c18b4 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1047,8 +1047,6 @@ static int s5p_mfc_mmap(struct file *file, struct vm_area_struct *vma)
>  	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
>  	int ret;
>  
> -	if (mutex_lock_interruptible(&dev->mfc_mutex))
> -		return -ERESTARTSYS;
>  	if (offset < DST_QUEUE_OFF_BASE) {
>  		mfc_debug(2, "mmaping source\n");
>  		ret = vb2_mmap(&ctx->vq_src, vma);
> @@ -1057,7 +1055,6 @@ static int s5p_mfc_mmap(struct file *file, struct vm_area_struct *vma)
>  		vma->vm_pgoff -= (DST_QUEUE_OFF_BASE >> PAGE_SHIFT);
>  		ret = vb2_mmap(&ctx->vq_dst, vma);
>  	}
> -	mutex_unlock(&dev->mfc_mutex);
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 1/2] media: exynos-gsc: fix lockdep warning

[Shuah Khan]

On 10/16/2017 09:18 AM, Hans Verkuil wrote:
> On 10/16/2017 05:16 PM, Shuah Khan wrote:
>> The driver mmap functions shouldn't take lock when calling vb2_mmap().
>> Fix it to not take the lock.
>>
>> Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3
>> and e752577ed7bf55c81e10343fced8b378cda2b63b
>>
>> The following lockdep warning is fixed with this change.
>>
>> [ 1990.972058] ======================================================
>> [ 1990.978172] WARNING: possible circular locking dependency detected
>> [ 1990.984327] 4.14.0-rc2-00002-gfab205f-dirty #4 Not tainted
>> [ 1990.989783] ------------------------------------------------------
>> [ 1990.995937] qtdemux0:sink/2765 is trying to acquire lock:
>> [ 1991.001309]  (&gsc->lock){+.+.}, at: [<bf1729f0>] gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
>> [ 1991.009108]
>>                but task is already holding lock:
>> [ 1991.014913]  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
>> [ 1991.021932]
>>                which lock already depends on the new lock.
>>
>> [ 1991.030078]
>>                the existing dependency chain (in reverse order) is:
>> [ 1991.037530]
>>                -> #1 (&mm->mmap_sem){++++}:
>> [ 1991.042913]        __might_fault+0x80/0xb0
>> [ 1991.047096]        video_usercopy+0x1cc/0x510 [videodev]
>> [ 1991.052297]        v4l2_ioctl+0xa4/0xdc [videodev]
>> [ 1991.057036]        do_vfs_ioctl+0xa0/0xa18
>> [ 1991.061102]        SyS_ioctl+0x34/0x5c
>> [ 1991.064834]        ret_fast_syscall+0x0/0x28
>> [ 1991.069072]
>>                -> #0 (&gsc->lock){+.+.}:
>> [ 1991.074193]        lock_acquire+0x6c/0x88
>> [ 1991.078179]        __mutex_lock+0x68/0xa34
>> [ 1991.082247]        mutex_lock_interruptible_nested+0x1c/0x24
>> [ 1991.087888]        gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
>> [ 1991.093029]        v4l2_mmap+0x54/0x88 [videodev]
>> [ 1991.097673]        mmap_region+0x3a8/0x638
>> [ 1991.101743]        do_mmap+0x330/0x3a4
>> [ 1991.105470]        vm_mmap_pgoff+0x90/0xb8
>> [ 1991.109542]        SyS_mmap_pgoff+0x90/0xc0
>> [ 1991.113702]        ret_fast_syscall+0x0/0x28
>> [ 1991.117945]
>>                other info that might help us debug this:
>>
>> [ 1991.125918]  Possible unsafe locking scenario:
>>
>> [ 1991.131810]        CPU0                    CPU1
>> [ 1991.136315]        ----                    ----
>> [ 1991.140821]   lock(&mm->mmap_sem);
>> [ 1991.144201]                                lock(&gsc->lock);
>> [ 1991.149833]                                lock(&mm->mmap_sem);
>> [ 1991.155725]   lock(&gsc->lock);
>> [ 1991.158845]
>>                 *** DEADLOCK ***
>>
>> [ 1991.164740] 1 lock held by qtdemux0:sink/2765:
>> [ 1991.169157]  #0:  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
>> [ 1991.176609]
>>                stack backtrace:
>> [ 1991.180946] CPU: 2 PID: 2765 Comm: qtdemux0:sink Not tainted 4.14.0-rc2-00002-gfab205f-dirty #4
>> [ 1991.189608] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> [ 1991.195686] [<c01102c8>] (unwind_backtrace) from [<c010cabc>] (show_stack+0x10/0x14)
>> [ 1991.203393] [<c010cabc>] (show_stack) from [<c08543a4>] (dump_stack+0x98/0xc4)
>> [ 1991.210586] [<c08543a4>] (dump_stack) from [<c016b2fc>] (print_circular_bug+0x254/0x410)
>> [ 1991.218644] [<c016b2fc>] (print_circular_bug) from [<c016c580>] (check_prev_add+0x468/0x938)
>> [ 1991.227049] [<c016c580>] (check_prev_add) from [<c016f4dc>] (__lock_acquire+0x1314/0x14fc)
>> [ 1991.235281] [<c016f4dc>] (__lock_acquire) from [<c016fefc>] (lock_acquire+0x6c/0x88)
>> [ 1991.242993] [<c016fefc>] (lock_acquire) from [<c0869fb4>] (__mutex_lock+0x68/0xa34)
>> [ 1991.250620] [<c0869fb4>] (__mutex_lock) from [<c086aa08>] (mutex_lock_interruptible_nested+0x1c/0x24)
>> [ 1991.259812] [<c086aa08>] (mutex_lock_interruptible_nested) from [<bf1729f0>] (gsc_m2m_mmap+0x24/0x5c [exynos_gsc])
>> [ 1991.270159] [<bf1729f0>] (gsc_m2m_mmap [exynos_gsc]) from [<bf037120>] (v4l2_mmap+0x54/0x88 [videodev])
>> [ 1991.279510] [<bf037120>] (v4l2_mmap [videodev]) from [<c01f4798>] (mmap_region+0x3a8/0x638)
>> [ 1991.287792] [<c01f4798>] (mmap_region) from [<c01f4d58>] (do_mmap+0x330/0x3a4)
>> [ 1991.294986] [<c01f4d58>] (do_mmap) from [<c01df330>] (vm_mmap_pgoff+0x90/0xb8)
>> [ 1991.302178] [<c01df330>] (vm_mmap_pgoff) from [<c01f28cc>] (SyS_mmap_pgoff+0x90/0xc0)
>> [ 1991.309977] [<c01f28cc>] (SyS_mmap_pgoff) from [<c0108820>] (ret_fast_syscall+0x0/0x28)
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> Suggested-by: Hans Verkuil <hansverk@cisco.com>
>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Acked-by: Hans Verkuil <hansverk@cisco.com>
> 
> Regards,
> 
> 	Hans

Hi Mauro,

Are you planning to take this in for 4.15-rc1? This patch is applicable
to stable as well.

thanks,
-- Shuah

Re: [PATCH v2 1/2] media: exynos-gsc: fix lockdep warning

[Shuah Khan]

On 11/07/2017 09:53 AM, Shuah Khan wrote:
> On 10/16/2017 09:18 AM, Hans Verkuil wrote:
>> On 10/16/2017 05:16 PM, Shuah Khan wrote:
>>> The driver mmap functions shouldn't take lock when calling vb2_mmap().
>>> Fix it to not take the lock.
>>>
>>> Reference: commit log for f035eb4e976ef5a059e30bc91cfd310ff030a7d3
>>> and e752577ed7bf55c81e10343fced8b378cda2b63b
>>>
>>> The following lockdep warning is fixed with this change.
>>>
>>> [ 1990.972058] ======================================================
>>> [ 1990.978172] WARNING: possible circular locking dependency detected
>>> [ 1990.984327] 4.14.0-rc2-00002-gfab205f-dirty #4 Not tainted
>>> [ 1990.989783] ------------------------------------------------------
>>> [ 1990.995937] qtdemux0:sink/2765 is trying to acquire lock:
>>> [ 1991.001309]  (&gsc->lock){+.+.}, at: [<bf1729f0>] gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
>>> [ 1991.009108]
>>>                but task is already holding lock:
>>> [ 1991.014913]  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
>>> [ 1991.021932]
>>>                which lock already depends on the new lock.
>>>
>>> [ 1991.030078]
>>>                the existing dependency chain (in reverse order) is:
>>> [ 1991.037530]
>>>                -> #1 (&mm->mmap_sem){++++}:
>>> [ 1991.042913]        __might_fault+0x80/0xb0
>>> [ 1991.047096]        video_usercopy+0x1cc/0x510 [videodev]
>>> [ 1991.052297]        v4l2_ioctl+0xa4/0xdc [videodev]
>>> [ 1991.057036]        do_vfs_ioctl+0xa0/0xa18
>>> [ 1991.061102]        SyS_ioctl+0x34/0x5c
>>> [ 1991.064834]        ret_fast_syscall+0x0/0x28
>>> [ 1991.069072]
>>>                -> #0 (&gsc->lock){+.+.}:
>>> [ 1991.074193]        lock_acquire+0x6c/0x88
>>> [ 1991.078179]        __mutex_lock+0x68/0xa34
>>> [ 1991.082247]        mutex_lock_interruptible_nested+0x1c/0x24
>>> [ 1991.087888]        gsc_m2m_mmap+0x24/0x5c [exynos_gsc]
>>> [ 1991.093029]        v4l2_mmap+0x54/0x88 [videodev]
>>> [ 1991.097673]        mmap_region+0x3a8/0x638
>>> [ 1991.101743]        do_mmap+0x330/0x3a4
>>> [ 1991.105470]        vm_mmap_pgoff+0x90/0xb8
>>> [ 1991.109542]        SyS_mmap_pgoff+0x90/0xc0
>>> [ 1991.113702]        ret_fast_syscall+0x0/0x28
>>> [ 1991.117945]
>>>                other info that might help us debug this:
>>>
>>> [ 1991.125918]  Possible unsafe locking scenario:
>>>
>>> [ 1991.131810]        CPU0                    CPU1
>>> [ 1991.136315]        ----                    ----
>>> [ 1991.140821]   lock(&mm->mmap_sem);
>>> [ 1991.144201]                                lock(&gsc->lock);
>>> [ 1991.149833]                                lock(&mm->mmap_sem);
>>> [ 1991.155725]   lock(&gsc->lock);
>>> [ 1991.158845]
>>>                 *** DEADLOCK ***
>>>
>>> [ 1991.164740] 1 lock held by qtdemux0:sink/2765:
>>> [ 1991.169157]  #0:  (&mm->mmap_sem){++++}, at: [<c01df2e4>] vm_mmap_pgoff+0x44/0xb8
>>> [ 1991.176609]
>>>                stack backtrace:
>>> [ 1991.180946] CPU: 2 PID: 2765 Comm: qtdemux0:sink Not tainted 4.14.0-rc2-00002-gfab205f-dirty #4
>>> [ 1991.189608] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>> [ 1991.195686] [<c01102c8>] (unwind_backtrace) from [<c010cabc>] (show_stack+0x10/0x14)
>>> [ 1991.203393] [<c010cabc>] (show_stack) from [<c08543a4>] (dump_stack+0x98/0xc4)
>>> [ 1991.210586] [<c08543a4>] (dump_stack) from [<c016b2fc>] (print_circular_bug+0x254/0x410)
>>> [ 1991.218644] [<c016b2fc>] (print_circular_bug) from [<c016c580>] (check_prev_add+0x468/0x938)
>>> [ 1991.227049] [<c016c580>] (check_prev_add) from [<c016f4dc>] (__lock_acquire+0x1314/0x14fc)
>>> [ 1991.235281] [<c016f4dc>] (__lock_acquire) from [<c016fefc>] (lock_acquire+0x6c/0x88)
>>> [ 1991.242993] [<c016fefc>] (lock_acquire) from [<c0869fb4>] (__mutex_lock+0x68/0xa34)
>>> [ 1991.250620] [<c0869fb4>] (__mutex_lock) from [<c086aa08>] (mutex_lock_interruptible_nested+0x1c/0x24)
>>> [ 1991.259812] [<c086aa08>] (mutex_lock_interruptible_nested) from [<bf1729f0>] (gsc_m2m_mmap+0x24/0x5c [exynos_gsc])
>>> [ 1991.270159] [<bf1729f0>] (gsc_m2m_mmap [exynos_gsc]) from [<bf037120>] (v4l2_mmap+0x54/0x88 [videodev])
>>> [ 1991.279510] [<bf037120>] (v4l2_mmap [videodev]) from [<c01f4798>] (mmap_region+0x3a8/0x638)
>>> [ 1991.287792] [<c01f4798>] (mmap_region) from [<c01f4d58>] (do_mmap+0x330/0x3a4)
>>> [ 1991.294986] [<c01f4d58>] (do_mmap) from [<c01df330>] (vm_mmap_pgoff+0x90/0xb8)
>>> [ 1991.302178] [<c01df330>] (vm_mmap_pgoff) from [<c01f28cc>] (SyS_mmap_pgoff+0x90/0xc0)
>>> [ 1991.309977] [<c01f28cc>] (SyS_mmap_pgoff) from [<c0108820>] (ret_fast_syscall+0x0/0x28)
>>>
>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>> Suggested-by: Hans Verkuil <hansverk@cisco.com>
>>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Acked-by: Hans Verkuil <hansverk@cisco.com>
>>
>> Regards,
>>
>> 	Hans
> 
> Hi Mauro,
> 
> Are you planning to take this in for 4.15-rc1? This patch is applicable
> to stable as well.
> 
> thanks,
> -- Shuah
> 

Hi Mauro,

Doesn't look like this fox made it into 4.15-rc2 - do you plan to get this
into 4.15 at some point?

I am seeing this lockdep warning in 4.15-rc2

thanks,
-- Shuah