diff mbox series

[v5,01/21] gpu: host1x: Use different lock classes for each client

Message ID 20210111130019.3515669-2-mperttunen@nvidia.com
State Rejected
Headers show
Series [v5,01/21] gpu: host1x: Use different lock classes for each client | expand

Commit Message

Mikko Perttunen Jan. 11, 2021, 12:59 p.m. UTC
To avoid false lockdep warnings, give each client lock a different
lock class, passed from the initialization site by macro.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/host1x/bus.c | 7 ++++---
 include/linux/host1x.h   | 9 ++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Thierry Reding March 22, 2021, 2:46 p.m. UTC | #1
On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
> To avoid false lockdep warnings, give each client lock a different
> lock class, passed from the initialization site by macro.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/gpu/host1x/bus.c | 7 ++++---
>  include/linux/host1x.h   | 9 ++++++++-
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index 347fb962b6c9..8fc79e9cb652 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
>   * device and call host1x_device_init(), which will in turn call each client's
>   * &host1x_client_ops.init implementation.
>   */
> -int host1x_client_register(struct host1x_client *client)
> +int __host1x_client_register(struct host1x_client *client,
> +			   struct lock_class_key *key)

I've seen the kbuild robot warn about this because the kerneldoc is now
out of date.

>  {
>  	struct host1x *host1x;
>  	int err;
>  
>  	INIT_LIST_HEAD(&client->list);
> -	mutex_init(&client->lock);
> +	__mutex_init(&client->lock, "host1x client lock", key);

Should we maybe attempt to make this unique? Could we use something like
dev_name(client->dev) for this?

Thierry
Dmitry Osipenko March 22, 2021, 2:48 p.m. UTC | #2
22.03.2021 17:46, Thierry Reding пишет:
> On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
>> To avoid false lockdep warnings, give each client lock a different
>> lock class, passed from the initialization site by macro.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>  drivers/gpu/host1x/bus.c | 7 ++++---
>>  include/linux/host1x.h   | 9 ++++++++-
>>  2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>> index 347fb962b6c9..8fc79e9cb652 100644
>> --- a/drivers/gpu/host1x/bus.c
>> +++ b/drivers/gpu/host1x/bus.c
>> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
>>   * device and call host1x_device_init(), which will in turn call each client's
>>   * &host1x_client_ops.init implementation.
>>   */
>> -int host1x_client_register(struct host1x_client *client)
>> +int __host1x_client_register(struct host1x_client *client,
>> +			   struct lock_class_key *key)
> 
> I've seen the kbuild robot warn about this because the kerneldoc is now
> out of date.
> 
>>  {
>>  	struct host1x *host1x;
>>  	int err;
>>  
>>  	INIT_LIST_HEAD(&client->list);
>> -	mutex_init(&client->lock);
>> +	__mutex_init(&client->lock, "host1x client lock", key);
> 
> Should we maybe attempt to make this unique? Could we use something like
> dev_name(client->dev) for this?

I'm curious who the lockdep warning could be triggered at all, I don't
recall ever seeing it. Mikko, could you please clarify how to reproduce
the warning?
Mikko Perttunen March 22, 2021, 3:19 p.m. UTC | #3
On 22.3.2021 16.48, Dmitry Osipenko wrote:
> 22.03.2021 17:46, Thierry Reding пишет:
>> On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
>>> To avoid false lockdep warnings, give each client lock a different
>>> lock class, passed from the initialization site by macro.
>>>
>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>> ---
>>>   drivers/gpu/host1x/bus.c | 7 ++++---
>>>   include/linux/host1x.h   | 9 ++++++++-
>>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>>> index 347fb962b6c9..8fc79e9cb652 100644
>>> --- a/drivers/gpu/host1x/bus.c
>>> +++ b/drivers/gpu/host1x/bus.c
>>> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
>>>    * device and call host1x_device_init(), which will in turn call each client's
>>>    * &host1x_client_ops.init implementation.
>>>    */
>>> -int host1x_client_register(struct host1x_client *client)
>>> +int __host1x_client_register(struct host1x_client *client,
>>> +			   struct lock_class_key *key)
>>
>> I've seen the kbuild robot warn about this because the kerneldoc is now
>> out of date.
>>
>>>   {
>>>   	struct host1x *host1x;
>>>   	int err;
>>>   
>>>   	INIT_LIST_HEAD(&client->list);
>>> -	mutex_init(&client->lock);
>>> +	__mutex_init(&client->lock, "host1x client lock", key);
>>
>> Should we maybe attempt to make this unique? Could we use something like
>> dev_name(client->dev) for this?
> 
> I'm curious who the lockdep warning could be triggered at all, I don't
> recall ever seeing it. Mikko, could you please clarify how to reproduce
> the warning?
> 

This is pretty difficult to read but I guess it's some interaction 
related to the delayed initialization of host1x clients? In any case, I 
consistently get it at boot (though it may be triggered by vic probe 
instead of nvdec).

I'll fix the kbuild robot warnings and see if I can add a 
client-specific lock name for v6.

Mikko

[   38.128257] WARNING: possible recursive locking detected
[   38.133567] 5.11.0-rc2-next-20210108+ #102 Tainted: G S
[   38.140089] --------------------------------------------
[   38.145395] systemd-udevd/239 is trying to acquire lock:
[   38.150703] ffff0000997aa218 (&client->lock){+.+.}-{3:3}, at: 
host1x_client_resume+0x30/0x100 [host1x]
[   38.160142]
[   38.160142] but task is already holding lock:
[   38.165968] ffff000080c3b148 (&client->lock){+.+.}-{3:3}, at: 
host1x_client_resume+0x30/0x100 [host1x]
[   38.175398]
[   38.175398] other info that might help us debug this:
[   38.181918]  Possible unsafe locking scenario:
[   38.181918]
[   38.187830]        CPU0
[   38.190275]        ----
[   38.192719]   lock(&client->lock);
[   38.196129]   lock(&client->lock);
[   38.199537]
[   38.199537]  *** DEADLOCK ***
[   38.199537]
[   38.205449]  May be due to missing lock nesting notation
[   38.205449]
[   38.212228] 6 locks held by systemd-udevd/239:
[   38.216669]  #0: ffff00009261c188 (&dev->mutex){....}-{3:3}, at: 
device_driver_attach+0x60/0x130
[   38.225487]  #1: ffff800009a17168 (devices_lock){+.+.}-{3:3}, at: 
host1x_client_register+0x7c/0x220 [host1x]
[   38.235441]  #2: ffff000083f94bb8 (&host->devices_lock){+.+.}-{3:3}, 
at: host1x_client_register+0xac/0x220 [host1x]
[   38.245996]  #3: ffff0000a2267190 (&dev->mutex){....}-{3:3}, at: 
__device_attach+0x8c/0x230
[   38.254372]  #4: ffff000092c880f0 (&wgrp->lock){+.+.}-{3:3}, at: 
tegra_display_hub_prepare+0xd8/0x170 [tegra_drm]
[   38.264788]  #5: ffff000080c3b148 (&client->lock){+.+.}-{3:3}, at: 
host1x_client_resume+0x30/0x100 [host1x]
[   38.274658]
[   38.274658] stack backtrace:
[   38.279012] CPU: 0 PID: 239 Comm: systemd-udevd Tainted: G S 
       5.11.0-rc2-next-20210108+ #102
[   38.288660] Hardware name: NVIDIA Jetson TX2 Developer Kit (DT)
[   38.294577] Call trace:
[   38.297022]  dump_backtrace+0x0/0x2c0
[   38.300695]  show_stack+0x18/0x6c
[   38.304013]  dump_stack+0x120/0x19c
[   38.307507]  __lock_acquire+0x171c/0x2c34
[   38.311521]  lock_acquire.part.0+0x230/0x490
[   38.315793]  lock_acquire+0x70/0x90
[   38.319285]  __mutex_lock+0x11c/0x6d0
[   38.322952]  mutex_lock_nested+0x58/0x90
[   38.326877]  host1x_client_resume+0x30/0x100 [host1x]
[   38.332047]  host1x_client_resume+0x44/0x100 [host1x]
[   38.337200]  tegra_display_hub_prepare+0xf8/0x170 [tegra_drm]
[   38.343084]  host1x_drm_probe+0x1fc/0x4f0 [tegra_drm]
[   38.348256]  host1x_device_probe+0x3c/0x50 [host1x]
[   38.353240]  really_probe+0x148/0x6f0
[   38.356906]  driver_probe_device+0x78/0xe4
[   38.361005]  __device_attach_driver+0x10c/0x170
[   38.365536]  bus_for_each_drv+0xf0/0x160
[   38.369461]  __device_attach+0x168/0x230
[   38.373385]  device_initial_probe+0x14/0x20
[   38.377571]  bus_probe_device+0xec/0x100
[   38.381494]  device_add+0x580/0xbcc
[   38.384985]  host1x_subdev_register+0x178/0x1cc [host1x]
[   38.390397]  host1x_client_register+0x138/0x220 [host1x]
[   38.395808]  nvdec_probe+0x240/0x3ec [tegra_drm]
[   38.400549]  platform_probe+0x8c/0x110
[   38.404302]  really_probe+0x148/0x6f0
[   38.407966]  driver_probe_device+0x78/0xe4
[   38.412065]  device_driver_attach+0x120/0x130
[   38.416423]  __driver_attach+0xb4/0x190
[   38.420261]  bus_for_each_dev+0xe8/0x160
[   38.424185]  driver_attach+0x34/0x44
[   38.427761]  bus_add_driver+0x1a4/0x2b0
[   38.431598]  driver_register+0xe0/0x210
[   38.435437]  __platform_register_drivers+0x6c/0x104
[   38.440318]  host1x_drm_init+0x54/0x1000 [tegra_drm]
[   38.445405]  do_one_initcall+0xec/0x5e0
[   38.449244]  do_init_module+0xe0/0x384
[   38.453000]  load_module+0x32d8/0x3c60
Dmitry Osipenko March 22, 2021, 4:01 p.m. UTC | #4
22.03.2021 18:19, Mikko Perttunen пишет:
> On 22.3.2021 16.48, Dmitry Osipenko wrote:
>> 22.03.2021 17:46, Thierry Reding пишет:
>>> On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
>>>> To avoid false lockdep warnings, give each client lock a different
>>>> lock class, passed from the initialization site by macro.
>>>>
>>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>>> ---
>>>>   drivers/gpu/host1x/bus.c | 7 ++++---
>>>>   include/linux/host1x.h   | 9 ++++++++-
>>>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>>>> index 347fb962b6c9..8fc79e9cb652 100644
>>>> --- a/drivers/gpu/host1x/bus.c
>>>> +++ b/drivers/gpu/host1x/bus.c
>>>> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
>>>>    * device and call host1x_device_init(), which will in turn call
>>>> each client's
>>>>    * &host1x_client_ops.init implementation.
>>>>    */
>>>> -int host1x_client_register(struct host1x_client *client)
>>>> +int __host1x_client_register(struct host1x_client *client,
>>>> +               struct lock_class_key *key)
>>>
>>> I've seen the kbuild robot warn about this because the kerneldoc is now
>>> out of date.
>>>
>>>>   {
>>>>       struct host1x *host1x;
>>>>       int err;
>>>>         INIT_LIST_HEAD(&client->list);
>>>> -    mutex_init(&client->lock);
>>>> +    __mutex_init(&client->lock, "host1x client lock", key);
>>>
>>> Should we maybe attempt to make this unique? Could we use something like
>>> dev_name(client->dev) for this?
>>
>> I'm curious who the lockdep warning could be triggered at all, I don't
>> recall ever seeing it. Mikko, could you please clarify how to reproduce
>> the warning?
>>
> 
> This is pretty difficult to read but I guess it's some interaction
> related to the delayed initialization of host1x clients? In any case, I
> consistently get it at boot (though it may be triggered by vic probe
> instead of nvdec).
> 
> I'll fix the kbuild robot warnings and see if I can add a
> client-specific lock name for v6.

Thank you for the clarification! We now actually have a similar problem on Tegra20 after fixing the coupling of display controllers using the dc1_client->parent=dc0_client and I see the same warning when DC1 is enabled.

[    3.808338] ============================================
[    3.808355] WARNING: possible recursive locking detected
[    3.808376] 5.12.0-rc3-next-20210319-00176-g60867e51e180 #7219 Tainted: G        W        
[    3.808406] --------------------------------------------
[    3.808421] kworker/1:2/108 is trying to acquire lock:
[    3.808449] c36b70a4 (&client->lock){+.+.}-{3:3}, at: host1x_client_resume+0x17/0x58
[    3.808586] 
               but task is already holding lock:
[    3.808603] c34df8a4 (&client->lock){+.+.}-{3:3}, at: host1x_client_resume+0x17/0x58
[    3.808712] 
               other info that might help us debug this:
[    3.808729]  Possible unsafe locking scenario:

[    3.808744]        CPU0
[    3.808757]        ----
[    3.808771]   lock(&client->lock);
[    3.808810]   lock(&client->lock);
[    3.808821] 
                *** DEADLOCK ***

[    3.808825]  May be due to missing lock nesting notation

[    3.808829] 15 locks held by kworker/1:2/108:
[    3.808836]  #0: c20068a8 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x15a/0x608
[    3.808878]  #1: c2bbbf18 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x15a/0x608
[    3.808912]  #2: c366d4d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x29/0xdc
[    3.808953]  #3: c141a980 (devices_lock){+.+.}-{3:3}, at: host1x_client_register+0x35/0xfc
[    3.808986]  #4: c34df64c (&host1x->devices_lock){+.+.}-{3:3}, at: host1x_client_register+0x51/0xfc
[    3.809017]  #5: c34ed4d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x29/0xdc
[    3.809050]  #6: c13faf5c (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x2d/0x274
[    3.809092]  #7: c132566c (console_lock){+.+.}-{0:0}, at: register_framebuffer+0x219/0x274
[    3.809124]  #8: c36e7848 (&fb_info->lock){+.+.}-{3:3}, at: register_framebuffer+0x19f/0x274
[    3.809157]  #9: c36d2d6c (&helper->lock){+.+.}-{3:3}, at: __drm_fb_helper_restore_fbdev_mode_unlocked+0x41/0x8c
[    3.809199]  #10: c36f00e8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x17/0x28
[    3.809233]  #11: c36d2c50 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x1d/0x138
[    3.809272]  #12: c2bbba28 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x2f/0x1c4
[    3.809306]  #13: c36e6448 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_backoff+0x63/0x190
[    3.809337]  #14: c34df8a4 (&client->lock){+.+.}-{3:3}, at: host1x_client_resume+0x17/0x58
[    3.809369] 
               stack backtrace:
[    3.809375] CPU: 1 PID: 108 Comm: kworker/1:2 Tainted: G        W         5.12.0-rc3-next-20210319-00176-g60867e51e180 #7219
[    3.809387] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    3.809396] Workqueue: events deferred_probe_work_func
[    3.809417] [<c010d1ad>] (unwind_backtrace) from [<c010961d>] (show_stack+0x11/0x14)
[    3.809447] [<c010961d>] (show_stack) from [<c0b7d7c9>] (dump_stack+0x9f/0xb8)
[    3.809467] [<c0b7d7c9>] (dump_stack) from [<c0179eef>] (__lock_acquire+0x7fb/0x253c)
[    3.809495] [<c0179eef>] (__lock_acquire) from [<c017c403>] (lock_acquire+0xf3/0x420)
[    3.809516] [<c017c403>] (lock_acquire) from [<c0b87663>] (__mutex_lock+0x87/0x814)
[    3.809544] [<c0b87663>] (__mutex_lock) from [<c0b87e09>] (mutex_lock_nested+0x19/0x20)
[    3.809565] [<c0b87e09>] (mutex_lock_nested) from [<c05ccd2f>] (host1x_client_resume+0x17/0x58)
[    3.809587] [<c05ccd2f>] (host1x_client_resume) from [<c05ccd37>] (host1x_client_resume+0x1f/0x58)
[    3.809604] [<c05ccd37>] (host1x_client_resume) from [<c061d9a3>] (tegra_crtc_atomic_enable+0x33/0x21c4)
[    3.809634] [<c061d9a3>] (tegra_crtc_atomic_enable) from [<c05e0355>] (drm_atomic_helper_commit_modeset_enables+0x131/0x16c)
[    3.809667] [<c05e0355>] (drm_atomic_helper_commit_modeset_enables) from [<c05e0e89>] (drm_atomic_helper_commit_tail_rpm+0x1d/0x4c)
[    3.809691] [<c05e0e89>] (drm_atomic_helper_commit_tail_rpm) from [<c0610157>] (tegra_atomic_commit_tail+0x83/0x84)
[    3.809712] [<c0610157>] (tegra_atomic_commit_tail) from [<c05e1271>] (commit_tail+0x71/0x138)
[    3.809732] [<c05e1271>] (commit_tail) from [<c05e1b95>] (drm_atomic_helper_commit+0xf1/0x114)
[    3.809753] [<c05e1b95>] (drm_atomic_helper_commit) from [<c0607355>] (drm_client_modeset_commit_atomic+0x199/0x1c4)
[    3.809777] [<c0607355>] (drm_client_modeset_commit_atomic) from [<c0607401>] (drm_client_modeset_commit_locked+0x3d/0x138)
[    3.809798] [<c0607401>] (drm_client_modeset_commit_locked) from [<c0607517>] (drm_client_modeset_commit+0x1b/0x2c)
[    3.809818] [<c0607517>] (drm_client_modeset_commit) from [<c05e5c4f>] (__drm_fb_helper_restore_fbdev_mode_unlocked+0x73/0x8c)
[    3.809842] [<c05e5c4f>] (__drm_fb_helper_restore_fbdev_mode_unlocked) from [<c05e5cc9>] (drm_fb_helper_set_par+0x2d/0x4c)
[    3.809862] [<c05e5cc9>] (drm_fb_helper_set_par) from [<c056c763>] (fbcon_init+0x1cb/0x370)
[    3.809883] [<c056c763>] (fbcon_init) from [<c05af8c7>] (visual_init+0x8b/0xc8)
[    3.809902] [<c05af8c7>] (visual_init) from [<c05b07c5>] (do_bind_con_driver+0x13d/0x2b4)
[    3.809919] [<c05b07c5>] (do_bind_con_driver) from [<c05b0b93>] (do_take_over_console+0xdf/0x15c)
[    3.809937] [<c05b0b93>] (do_take_over_console) from [<c056b1df>] (do_fbcon_takeover+0x4f/0x90)
[    3.809955] [<c056b1df>] (do_fbcon_takeover) from [<c056545d>] (register_framebuffer+0x1a5/0x274)
[    3.809977] [<c056545d>] (register_framebuffer) from [<c05e57cf>] (__drm_fb_helper_initial_config_and_unlock+0x29f/0x438)
[    3.809999] [<c05e57cf>] (__drm_fb_helper_initial_config_and_unlock) from [<c06115e1>] (tegra_drm_fb_init+0x25/0x5c)
[    3.810022] [<c06115e1>] (tegra_drm_fb_init) from [<c060feff>] (host1x_drm_probe+0x247/0x404)
[    3.810041] [<c060feff>] (host1x_drm_probe) from [<c0647ad9>] (really_probe+0xb1/0x2a4)
[    3.810064] [<c0647ad9>] (really_probe) from [<c0647d0b>] (driver_probe_device+0x3f/0x78)
[    3.810086] [<c0647d0b>] (driver_probe_device) from [<c0646737>] (bus_for_each_drv+0x4f/0x78)
[    3.810107] [<c0646737>] (bus_for_each_drv) from [<c06479d5>] (__device_attach+0x95/0xdc)
[    3.810127] [<c06479d5>] (__device_attach) from [<c064702d>] (bus_probe_device+0x5d/0x64)
[    3.810147] [<c064702d>] (bus_probe_device) from [<c064581b>] (device_add+0x293/0x5c0)
[    3.810166] [<c064581b>] (device_add) from [<c05cd211>] (host1x_subdev_register+0x8d/0xac)
[    3.810186] [<c05cd211>] (host1x_subdev_register) from [<c05cd4d3>] (host1x_client_register+0x8f/0xfc)
[    3.810204] [<c05cd4d3>] (host1x_client_register) from [<c061870f>] (tegra_dc_probe+0x1bf/0x2b0)
[    3.810225] [<c061870f>] (tegra_dc_probe) from [<c064977b>] (platform_probe+0x43/0x80)
[    3.810247] [<c064977b>] (platform_probe) from [<c0647ad9>] (really_probe+0xb1/0x2a4)
[    3.810266] [<c0647ad9>] (really_probe) from [<c0647d0b>] (driver_probe_device+0x3f/0x78)
[    3.810286] [<c0647d0b>] (driver_probe_device) from [<c0646737>] (bus_for_each_drv+0x4f/0x78)
[    3.810307] [<c0646737>] (bus_for_each_drv) from [<c06479d5>] (__device_attach+0x95/0xdc)
[    3.810326] [<c06479d5>] (__device_attach) from [<c064702d>] (bus_probe_device+0x5d/0x64)
[    3.810346] [<c064702d>] (bus_probe_device) from [<c0647379>] (deferred_probe_work_func+0x4d/0x70)
[    3.810367] [<c0647379>] (deferred_probe_work_func) from [<c0139557>] (process_one_work+0x1eb/0x608)
[    3.810391] [<c0139557>] (process_one_work) from [<c0139a6d>] (worker_thread+0xf9/0x3bc)
[    3.810411] [<c0139a6d>] (worker_thread) from [<c013f3db>] (kthread+0xff/0x134)
[    3.810432] [<c013f3db>] (kthread) from [<c0100159>] (ret_from_fork+0x11/0x38)
[    3.810449] Exception stack(0xc2bbbfb0 to 0xc2bbbff8)
Thierry Reding March 23, 2021, 10:20 a.m. UTC | #5
On Mon, Mar 22, 2021 at 07:01:34PM +0300, Dmitry Osipenko wrote:
> 22.03.2021 18:19, Mikko Perttunen пишет:
> > On 22.3.2021 16.48, Dmitry Osipenko wrote:
> >> 22.03.2021 17:46, Thierry Reding пишет:
> >>> On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
> >>>> To avoid false lockdep warnings, give each client lock a different
> >>>> lock class, passed from the initialization site by macro.
> >>>>
> >>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> >>>> ---
> >>>>   drivers/gpu/host1x/bus.c | 7 ++++---
> >>>>   include/linux/host1x.h   | 9 ++++++++-
> >>>>   2 files changed, 12 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> >>>> index 347fb962b6c9..8fc79e9cb652 100644
> >>>> --- a/drivers/gpu/host1x/bus.c
> >>>> +++ b/drivers/gpu/host1x/bus.c
> >>>> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
> >>>>    * device and call host1x_device_init(), which will in turn call
> >>>> each client's
> >>>>    * &host1x_client_ops.init implementation.
> >>>>    */
> >>>> -int host1x_client_register(struct host1x_client *client)
> >>>> +int __host1x_client_register(struct host1x_client *client,
> >>>> +               struct lock_class_key *key)
> >>>
> >>> I've seen the kbuild robot warn about this because the kerneldoc is now
> >>> out of date.
> >>>
> >>>>   {
> >>>>       struct host1x *host1x;
> >>>>       int err;
> >>>>         INIT_LIST_HEAD(&client->list);
> >>>> -    mutex_init(&client->lock);
> >>>> +    __mutex_init(&client->lock, "host1x client lock", key);
> >>>
> >>> Should we maybe attempt to make this unique? Could we use something like
> >>> dev_name(client->dev) for this?
> >>
> >> I'm curious who the lockdep warning could be triggered at all, I don't
> >> recall ever seeing it. Mikko, could you please clarify how to reproduce
> >> the warning?
> >>
> > 
> > This is pretty difficult to read but I guess it's some interaction
> > related to the delayed initialization of host1x clients? In any case, I
> > consistently get it at boot (though it may be triggered by vic probe
> > instead of nvdec).
> > 
> > I'll fix the kbuild robot warnings and see if I can add a
> > client-specific lock name for v6.
> 
> Thank you for the clarification! We now actually have a similar problem on Tegra20 after fixing the coupling of display controllers using the dc1_client->parent=dc0_client and I see the same warning when DC1 is enabled.
> 
> [    3.808338] ============================================
> [    3.808355] WARNING: possible recursive locking detected
> [    3.808376] 5.12.0-rc3-next-20210319-00176-g60867e51e180 #7219 Tainted: G        W        
> [    3.808406] --------------------------------------------
> [    3.808421] kworker/1:2/108 is trying to acquire lock:
> [    3.808449] c36b70a4 (&client->lock){+.+.}-{3:3}, at: host1x_client_resume+0x17/0x58
> [    3.808586] 
>                but task is already holding lock:
> [    3.808603] c34df8a4 (&client->lock){+.+.}-{3:3}, at: host1x_client_resume+0x17/0x58
> [    3.808712] 
>                other info that might help us debug this:
> [    3.808729]  Possible unsafe locking scenario:
> 
> [    3.808744]        CPU0
> [    3.808757]        ----
> [    3.808771]   lock(&client->lock);
> [    3.808810]   lock(&client->lock);
> [    3.808821] 
>                 *** DEADLOCK ***
> 
> [    3.808825]  May be due to missing lock nesting notation
> 
> [    3.808829] 15 locks held by kworker/1:2/108:
> [    3.808836]  #0: c20068a8 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x15a/0x608
> [    3.808878]  #1: c2bbbf18 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x15a/0x608
> [    3.808912]  #2: c366d4d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x29/0xdc
> [    3.808953]  #3: c141a980 (devices_lock){+.+.}-{3:3}, at: host1x_client_register+0x35/0xfc
> [    3.808986]  #4: c34df64c (&host1x->devices_lock){+.+.}-{3:3}, at: host1x_client_register+0x51/0xfc
> [    3.809017]  #5: c34ed4d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x29/0xdc
> [    3.809050]  #6: c13faf5c (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x2d/0x274
> [    3.809092]  #7: c132566c (console_lock){+.+.}-{0:0}, at: register_framebuffer+0x219/0x274
> [    3.809124]  #8: c36e7848 (&fb_info->lock){+.+.}-{3:3}, at: register_framebuffer+0x19f/0x274
> [    3.809157]  #9: c36d2d6c (&helper->lock){+.+.}-{3:3}, at: __drm_fb_helper_restore_fbdev_mode_unlocked+0x41/0x8c
> [    3.809199]  #10: c36f00e8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x17/0x28
> [    3.809233]  #11: c36d2c50 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x1d/0x138
> [    3.809272]  #12: c2bbba28 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x2f/0x1c4
> [    3.809306]  #13: c36e6448 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_backoff+0x63/0x190
> [    3.809337]  #14: c34df8a4 (&client->lock){+.+.}-{3:3}, at: host1x_client_resume+0x17/0x58
> [    3.809369] 
>                stack backtrace:
> [    3.809375] CPU: 1 PID: 108 Comm: kworker/1:2 Tainted: G        W         5.12.0-rc3-next-20210319-00176-g60867e51e180 #7219
> [    3.809387] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    3.809396] Workqueue: events deferred_probe_work_func
> [    3.809417] [<c010d1ad>] (unwind_backtrace) from [<c010961d>] (show_stack+0x11/0x14)
> [    3.809447] [<c010961d>] (show_stack) from [<c0b7d7c9>] (dump_stack+0x9f/0xb8)
> [    3.809467] [<c0b7d7c9>] (dump_stack) from [<c0179eef>] (__lock_acquire+0x7fb/0x253c)
> [    3.809495] [<c0179eef>] (__lock_acquire) from [<c017c403>] (lock_acquire+0xf3/0x420)
> [    3.809516] [<c017c403>] (lock_acquire) from [<c0b87663>] (__mutex_lock+0x87/0x814)
> [    3.809544] [<c0b87663>] (__mutex_lock) from [<c0b87e09>] (mutex_lock_nested+0x19/0x20)
> [    3.809565] [<c0b87e09>] (mutex_lock_nested) from [<c05ccd2f>] (host1x_client_resume+0x17/0x58)
> [    3.809587] [<c05ccd2f>] (host1x_client_resume) from [<c05ccd37>] (host1x_client_resume+0x1f/0x58)
> [    3.809604] [<c05ccd37>] (host1x_client_resume) from [<c061d9a3>] (tegra_crtc_atomic_enable+0x33/0x21c4)
> [    3.809634] [<c061d9a3>] (tegra_crtc_atomic_enable) from [<c05e0355>] (drm_atomic_helper_commit_modeset_enables+0x131/0x16c)
> [    3.809667] [<c05e0355>] (drm_atomic_helper_commit_modeset_enables) from [<c05e0e89>] (drm_atomic_helper_commit_tail_rpm+0x1d/0x4c)
> [    3.809691] [<c05e0e89>] (drm_atomic_helper_commit_tail_rpm) from [<c0610157>] (tegra_atomic_commit_tail+0x83/0x84)
> [    3.809712] [<c0610157>] (tegra_atomic_commit_tail) from [<c05e1271>] (commit_tail+0x71/0x138)
> [    3.809732] [<c05e1271>] (commit_tail) from [<c05e1b95>] (drm_atomic_helper_commit+0xf1/0x114)
> [    3.809753] [<c05e1b95>] (drm_atomic_helper_commit) from [<c0607355>] (drm_client_modeset_commit_atomic+0x199/0x1c4)
> [    3.809777] [<c0607355>] (drm_client_modeset_commit_atomic) from [<c0607401>] (drm_client_modeset_commit_locked+0x3d/0x138)
> [    3.809798] [<c0607401>] (drm_client_modeset_commit_locked) from [<c0607517>] (drm_client_modeset_commit+0x1b/0x2c)
> [    3.809818] [<c0607517>] (drm_client_modeset_commit) from [<c05e5c4f>] (__drm_fb_helper_restore_fbdev_mode_unlocked+0x73/0x8c)
> [    3.809842] [<c05e5c4f>] (__drm_fb_helper_restore_fbdev_mode_unlocked) from [<c05e5cc9>] (drm_fb_helper_set_par+0x2d/0x4c)
> [    3.809862] [<c05e5cc9>] (drm_fb_helper_set_par) from [<c056c763>] (fbcon_init+0x1cb/0x370)
> [    3.809883] [<c056c763>] (fbcon_init) from [<c05af8c7>] (visual_init+0x8b/0xc8)
> [    3.809902] [<c05af8c7>] (visual_init) from [<c05b07c5>] (do_bind_con_driver+0x13d/0x2b4)
> [    3.809919] [<c05b07c5>] (do_bind_con_driver) from [<c05b0b93>] (do_take_over_console+0xdf/0x15c)
> [    3.809937] [<c05b0b93>] (do_take_over_console) from [<c056b1df>] (do_fbcon_takeover+0x4f/0x90)
> [    3.809955] [<c056b1df>] (do_fbcon_takeover) from [<c056545d>] (register_framebuffer+0x1a5/0x274)
> [    3.809977] [<c056545d>] (register_framebuffer) from [<c05e57cf>] (__drm_fb_helper_initial_config_and_unlock+0x29f/0x438)
> [    3.809999] [<c05e57cf>] (__drm_fb_helper_initial_config_and_unlock) from [<c06115e1>] (tegra_drm_fb_init+0x25/0x5c)
> [    3.810022] [<c06115e1>] (tegra_drm_fb_init) from [<c060feff>] (host1x_drm_probe+0x247/0x404)
> [    3.810041] [<c060feff>] (host1x_drm_probe) from [<c0647ad9>] (really_probe+0xb1/0x2a4)
> [    3.810064] [<c0647ad9>] (really_probe) from [<c0647d0b>] (driver_probe_device+0x3f/0x78)
> [    3.810086] [<c0647d0b>] (driver_probe_device) from [<c0646737>] (bus_for_each_drv+0x4f/0x78)
> [    3.810107] [<c0646737>] (bus_for_each_drv) from [<c06479d5>] (__device_attach+0x95/0xdc)
> [    3.810127] [<c06479d5>] (__device_attach) from [<c064702d>] (bus_probe_device+0x5d/0x64)
> [    3.810147] [<c064702d>] (bus_probe_device) from [<c064581b>] (device_add+0x293/0x5c0)
> [    3.810166] [<c064581b>] (device_add) from [<c05cd211>] (host1x_subdev_register+0x8d/0xac)
> [    3.810186] [<c05cd211>] (host1x_subdev_register) from [<c05cd4d3>] (host1x_client_register+0x8f/0xfc)
> [    3.810204] [<c05cd4d3>] (host1x_client_register) from [<c061870f>] (tegra_dc_probe+0x1bf/0x2b0)
> [    3.810225] [<c061870f>] (tegra_dc_probe) from [<c064977b>] (platform_probe+0x43/0x80)
> [    3.810247] [<c064977b>] (platform_probe) from [<c0647ad9>] (really_probe+0xb1/0x2a4)
> [    3.810266] [<c0647ad9>] (really_probe) from [<c0647d0b>] (driver_probe_device+0x3f/0x78)
> [    3.810286] [<c0647d0b>] (driver_probe_device) from [<c0646737>] (bus_for_each_drv+0x4f/0x78)
> [    3.810307] [<c0646737>] (bus_for_each_drv) from [<c06479d5>] (__device_attach+0x95/0xdc)
> [    3.810326] [<c06479d5>] (__device_attach) from [<c064702d>] (bus_probe_device+0x5d/0x64)
> [    3.810346] [<c064702d>] (bus_probe_device) from [<c0647379>] (deferred_probe_work_func+0x4d/0x70)
> [    3.810367] [<c0647379>] (deferred_probe_work_func) from [<c0139557>] (process_one_work+0x1eb/0x608)
> [    3.810391] [<c0139557>] (process_one_work) from [<c0139a6d>] (worker_thread+0xf9/0x3bc)
> [    3.810411] [<c0139a6d>] (worker_thread) from [<c013f3db>] (kthread+0xff/0x134)
> [    3.810432] [<c013f3db>] (kthread) from [<c0100159>] (ret_from_fork+0x11/0x38)
> [    3.810449] Exception stack(0xc2bbbfb0 to 0xc2bbbff8)

Sounds like we should decouple this from the series and fast-track this
for v5.13, or perhaps even v5.12 along with the DC coupling fix?

Thierry
Dmitry Osipenko March 23, 2021, 1:25 p.m. UTC | #6
23.03.2021 13:20, Thierry Reding пишет:
> On Mon, Mar 22, 2021 at 07:01:34PM +0300, Dmitry Osipenko wrote:
>> 22.03.2021 18:19, Mikko Perttunen пишет:
>>> On 22.3.2021 16.48, Dmitry Osipenko wrote:
>>>> 22.03.2021 17:46, Thierry Reding пишет:
>>>>> On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
>>>>>> To avoid false lockdep warnings, give each client lock a different
>>>>>> lock class, passed from the initialization site by macro.
>>>>>>
>>>>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>>>>> ---
>>>>>>   drivers/gpu/host1x/bus.c | 7 ++++---
>>>>>>   include/linux/host1x.h   | 9 ++++++++-
>>>>>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>>>>>> index 347fb962b6c9..8fc79e9cb652 100644
>>>>>> --- a/drivers/gpu/host1x/bus.c
>>>>>> +++ b/drivers/gpu/host1x/bus.c
>>>>>> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
>>>>>>    * device and call host1x_device_init(), which will in turn call
>>>>>> each client's
>>>>>>    * &host1x_client_ops.init implementation.
>>>>>>    */
>>>>>> -int host1x_client_register(struct host1x_client *client)
>>>>>> +int __host1x_client_register(struct host1x_client *client,
>>>>>> +               struct lock_class_key *key)
>>>>>
>>>>> I've seen the kbuild robot warn about this because the kerneldoc is now
>>>>> out of date.
>>>>>
>>>>>>   {
>>>>>>       struct host1x *host1x;
>>>>>>       int err;
>>>>>>         INIT_LIST_HEAD(&client->list);
>>>>>> -    mutex_init(&client->lock);
>>>>>> +    __mutex_init(&client->lock, "host1x client lock", key);
>>>>>
>>>>> Should we maybe attempt to make this unique? Could we use something like
>>>>> dev_name(client->dev) for this?
>>>>
>>>> I'm curious who the lockdep warning could be triggered at all, I don't
>>>> recall ever seeing it. Mikko, could you please clarify how to reproduce
>>>> the warning?
>>>>
>>>
>>> This is pretty difficult to read but I guess it's some interaction
>>> related to the delayed initialization of host1x clients? In any case, I
>>> consistently get it at boot (though it may be triggered by vic probe
>>> instead of nvdec).
>>>
>>> I'll fix the kbuild robot warnings and see if I can add a
>>> client-specific lock name for v6.
>>
>> Thank you for the clarification! We now actually have a similar problem on Tegra20 after fixing the coupling of display controllers using the dc1_client->parent=dc0_client and I see the same warning when DC1 is enabled.
>>
...
> Sounds like we should decouple this from the series and fast-track this
> for v5.13, or perhaps even v5.12 along with the DC coupling fix?

Agree that the patch should be decoupled since it fixes a standalone
problem that isn't related to the rest of the patches.

It also should be good to have it backported, although this is optional.
If there are no merge conflicts with stable kernels for this patch, then
I'd add a stable tag to it.

Mikko, please update this patch and send it separately.
Mikko Perttunen March 26, 2021, 2:54 p.m. UTC | #7
On 3/22/21 5:19 PM, Mikko Perttunen wrote:
> On 22.3.2021 16.48, Dmitry Osipenko wrote:
>> 22.03.2021 17:46, Thierry Reding пишет:
>>> On Mon, Jan 11, 2021 at 02:59:59PM +0200, Mikko Perttunen wrote:
>>>> To avoid false lockdep warnings, give each client lock a different
>>>> lock class, passed from the initialization site by macro.
>>>>
>>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>>> ---
>>>>   drivers/gpu/host1x/bus.c | 7 ++++---
>>>>   include/linux/host1x.h   | 9 ++++++++-
>>>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>>>> index 347fb962b6c9..8fc79e9cb652 100644
>>>> --- a/drivers/gpu/host1x/bus.c
>>>> +++ b/drivers/gpu/host1x/bus.c
>>>> @@ -715,13 +715,14 @@ EXPORT_SYMBOL(host1x_driver_unregister);
>>>>    * device and call host1x_device_init(), which will in turn call 
>>>> each client's
>>>>    * &host1x_client_ops.init implementation.
>>>>    */
>>>> -int host1x_client_register(struct host1x_client *client)
>>>> +int __host1x_client_register(struct host1x_client *client,
>>>> +               struct lock_class_key *key)
>>>
>>> I've seen the kbuild robot warn about this because the kerneldoc is now
>>> out of date.
>>>
>>>>   {
>>>>       struct host1x *host1x;
>>>>       int err;
>>>>       INIT_LIST_HEAD(&client->list);
>>>> -    mutex_init(&client->lock);
>>>> +    __mutex_init(&client->lock, "host1x client lock", key);
>>>
>>> Should we maybe attempt to make this unique? Could we use something like
>>> dev_name(client->dev) for this?
>>
>> I'm curious who the lockdep warning could be triggered at all, I don't
>> recall ever seeing it. Mikko, could you please clarify how to reproduce
>> the warning?
>>
> 
> This is pretty difficult to read but I guess it's some interaction 
> related to the delayed initialization of host1x clients? In any case, I 
> consistently get it at boot (though it may be triggered by vic probe 
> instead of nvdec).
> 
> I'll fix the kbuild robot warnings and see if I can add a 
> client-specific lock name for v6.

Lockdep doesn't seem to be liking dev_name() for the name, and I think 
allocating a string for this purpose seems a bit overkill, so I'll keep 
the lock name as is if there are no objections.

Mikko

> 
> Mikko
> 
> [   38.128257] WARNING: possible recursive locking detected
> [   38.133567] 5.11.0-rc2-next-20210108+ #102 Tainted: G S
> [   38.140089] --------------------------------------------
> [   38.145395] systemd-udevd/239 is trying to acquire lock:
> [   38.150703] ffff0000997aa218 (&client->lock){+.+.}-{3:3}, at: 
> host1x_client_resume+0x30/0x100 [host1x]
> [   38.160142]
> [   38.160142] but task is already holding lock:
> [   38.165968] ffff000080c3b148 (&client->lock){+.+.}-{3:3}, at: 
> host1x_client_resume+0x30/0x100 [host1x]
> [   38.175398]
> [   38.175398] other info that might help us debug this:
> [   38.181918]  Possible unsafe locking scenario:
> [   38.181918]
> [   38.187830]        CPU0
> [   38.190275]        ----
> [   38.192719]   lock(&client->lock);
> [   38.196129]   lock(&client->lock);
> [   38.199537]
> [   38.199537]  *** DEADLOCK ***
> [   38.199537]
> [   38.205449]  May be due to missing lock nesting notation
> [   38.205449]
> [   38.212228] 6 locks held by systemd-udevd/239:
> [   38.216669]  #0: ffff00009261c188 (&dev->mutex){....}-{3:3}, at: 
> device_driver_attach+0x60/0x130
> [   38.225487]  #1: ffff800009a17168 (devices_lock){+.+.}-{3:3}, at: 
> host1x_client_register+0x7c/0x220 [host1x]
> [   38.235441]  #2: ffff000083f94bb8 (&host->devices_lock){+.+.}-{3:3}, 
> at: host1x_client_register+0xac/0x220 [host1x]
> [   38.245996]  #3: ffff0000a2267190 (&dev->mutex){....}-{3:3}, at: 
> __device_attach+0x8c/0x230
> [   38.254372]  #4: ffff000092c880f0 (&wgrp->lock){+.+.}-{3:3}, at: 
> tegra_display_hub_prepare+0xd8/0x170 [tegra_drm]
> [   38.264788]  #5: ffff000080c3b148 (&client->lock){+.+.}-{3:3}, at: 
> host1x_client_resume+0x30/0x100 [host1x]
> [   38.274658]
> [   38.274658] stack backtrace:
> [   38.279012] CPU: 0 PID: 239 Comm: systemd-udevd Tainted: G S       
> 5.11.0-rc2-next-20210108+ #102
> [   38.288660] Hardware name: NVIDIA Jetson TX2 Developer Kit (DT)
> [   38.294577] Call trace:
> [   38.297022]  dump_backtrace+0x0/0x2c0
> [   38.300695]  show_stack+0x18/0x6c
> [   38.304013]  dump_stack+0x120/0x19c
> [   38.307507]  __lock_acquire+0x171c/0x2c34
> [   38.311521]  lock_acquire.part.0+0x230/0x490
> [   38.315793]  lock_acquire+0x70/0x90
> [   38.319285]  __mutex_lock+0x11c/0x6d0
> [   38.322952]  mutex_lock_nested+0x58/0x90
> [   38.326877]  host1x_client_resume+0x30/0x100 [host1x]
> [   38.332047]  host1x_client_resume+0x44/0x100 [host1x]
> [   38.337200]  tegra_display_hub_prepare+0xf8/0x170 [tegra_drm]
> [   38.343084]  host1x_drm_probe+0x1fc/0x4f0 [tegra_drm]
> [   38.348256]  host1x_device_probe+0x3c/0x50 [host1x]
> [   38.353240]  really_probe+0x148/0x6f0
> [   38.356906]  driver_probe_device+0x78/0xe4
> [   38.361005]  __device_attach_driver+0x10c/0x170
> [   38.365536]  bus_for_each_drv+0xf0/0x160
> [   38.369461]  __device_attach+0x168/0x230
> [   38.373385]  device_initial_probe+0x14/0x20
> [   38.377571]  bus_probe_device+0xec/0x100
> [   38.381494]  device_add+0x580/0xbcc
> [   38.384985]  host1x_subdev_register+0x178/0x1cc [host1x]
> [   38.390397]  host1x_client_register+0x138/0x220 [host1x]
> [   38.395808]  nvdec_probe+0x240/0x3ec [tegra_drm]
> [   38.400549]  platform_probe+0x8c/0x110
> [   38.404302]  really_probe+0x148/0x6f0
> [   38.407966]  driver_probe_device+0x78/0xe4
> [   38.412065]  device_driver_attach+0x120/0x130
> [   38.416423]  __driver_attach+0xb4/0x190
> [   38.420261]  bus_for_each_dev+0xe8/0x160
> [   38.424185]  driver_attach+0x34/0x44
> [   38.427761]  bus_add_driver+0x1a4/0x2b0
> [   38.431598]  driver_register+0xe0/0x210
> [   38.435437]  __platform_register_drivers+0x6c/0x104
> [   38.440318]  host1x_drm_init+0x54/0x1000 [tegra_drm]
> [   38.445405]  do_one_initcall+0xec/0x5e0
> [   38.449244]  do_init_module+0xe0/0x384
> [   38.453000]  load_module+0x32d8/0x3c60
Dmitry Osipenko March 26, 2021, 6:31 p.m. UTC | #8
26.03.2021 17:54, Mikko Perttunen пишет:
> 
> Lockdep doesn't seem to be liking dev_name() for the name, and I think
> allocating a string for this purpose seems a bit overkill, so I'll keep
> the lock name as is if there are no objections.

What does "liking" mean?
Mikko Perttunen March 26, 2021, 7:10 p.m. UTC | #9
On 3/26/21 8:31 PM, Dmitry Osipenko wrote:
> 26.03.2021 17:54, Mikko Perttunen пишет:
>>
>> Lockdep doesn't seem to be liking dev_name() for the name, and I think
>> allocating a string for this purpose seems a bit overkill, so I'll keep
>> the lock name as is if there are no objections.
> 
> What does "liking" mean?
> 

kernel/locking/lockdep.c:894 (on my local tree):

                        WARN_ON_ONCE(class->name != lock->name &&
                                      lock->key != 
&__lockdep_no_validate__);

Mikko
Dmitry Osipenko March 26, 2021, 10:47 p.m. UTC | #10
26.03.2021 22:10, Mikko Perttunen пишет:
> On 3/26/21 8:31 PM, Dmitry Osipenko wrote:
>> 26.03.2021 17:54, Mikko Perttunen пишет:
>>>
>>> Lockdep doesn't seem to be liking dev_name() for the name, and I think
>>> allocating a string for this purpose seems a bit overkill, so I'll keep
>>> the lock name as is if there are no objections.
>>
>> What does "liking" mean?
>>
> 
> kernel/locking/lockdep.c:894 (on my local tree):
> 
>                        WARN_ON_ONCE(class->name != lock->name &&
>                                      lock->key !=
> &__lockdep_no_validate__);

Alright, I see now that lockdep requires to use the same name.
diff mbox series

Patch

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 347fb962b6c9..8fc79e9cb652 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -715,13 +715,14 @@  EXPORT_SYMBOL(host1x_driver_unregister);
  * device and call host1x_device_init(), which will in turn call each client's
  * &host1x_client_ops.init implementation.
  */
-int host1x_client_register(struct host1x_client *client)
+int __host1x_client_register(struct host1x_client *client,
+			   struct lock_class_key *key)
 {
 	struct host1x *host1x;
 	int err;
 
 	INIT_LIST_HEAD(&client->list);
-	mutex_init(&client->lock);
+	__mutex_init(&client->lock, "host1x client lock", key);
 	client->usecount = 0;
 
 	mutex_lock(&devices_lock);
@@ -742,7 +743,7 @@  int host1x_client_register(struct host1x_client *client)
 
 	return 0;
 }
-EXPORT_SYMBOL(host1x_client_register);
+EXPORT_SYMBOL(__host1x_client_register);
 
 /**
  * host1x_client_unregister() - unregister a host1x client
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index ce59a6a6a008..9eb77c87a83b 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -320,7 +320,14 @@  static inline struct host1x_device *to_host1x_device(struct device *dev)
 int host1x_device_init(struct host1x_device *device);
 int host1x_device_exit(struct host1x_device *device);
 
-int host1x_client_register(struct host1x_client *client);
+int __host1x_client_register(struct host1x_client *client,
+			     struct lock_class_key *key);
+#define host1x_client_register(class) \
+	({ \
+		static struct lock_class_key __key; \
+		__host1x_client_register(class, &__key); \
+	})
+
 int host1x_client_unregister(struct host1x_client *client);
 
 int host1x_client_suspend(struct host1x_client *client);