Re: [PATCH v5 3/4] scsi: ufs: core: Introduce a new clock_gating lock

[Manivannan Sadhasivam <manisadhasivam.linux@gmail.com>]

+ Dmitry

On Sun, Nov 24, 2024 at 09:08:07AM +0200, Avri Altman wrote:

[...]

> @@ -9162,7 +9159,6 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on)
>  	int ret = 0;
>  	struct ufs_clk_info *clki;
>  	struct list_head *head = &hba->clk_list_head;
> -	unsigned long flags;
>  	ktime_t start = ktime_get();
>  	bool clk_state_changed = false;
>  
> @@ -9213,11 +9209,10 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on)
>  				clk_disable_unprepare(clki->clk);
>  		}
>  	} else if (!ret && on) {
> -		spin_lock_irqsave(hba->host->host_lock, flags);
> -		hba->clk_gating.state = CLKS_ON;
> +		scoped_guard(spinlock_irqsave, &hba->clk_gating.lock)

This triggers the following lockdep warning on Qualcomm boards as reported by
Dmitry offline:

[    4.388838] INFO: trying to register non-static key.
[    4.395673] The code is fine but needs lockdep annotation, or maybe
[    4.402118] you didn't initialize this object before use?
[    4.407673] turning off the locking correctness validator.
[    4.413334] CPU: 5 UID: 0 PID: 58 Comm: kworker/u32:1 Not tainted 6.12-rc1 #185
[    4.413343] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[    4.413362] Call trace:
[    4.413364]  show_stack+0x18/0x24 (C)
[    4.413374]  dump_stack_lvl+0x90/0xd0
[    4.413384]  dump_stack+0x18/0x24
[    4.413392]  register_lock_class+0x498/0x4a8
[    4.413400]  __lock_acquire+0xb4/0x1b90
[    4.413406]  lock_acquire+0x114/0x310
[    4.413413]  _raw_spin_lock_irqsave+0x60/0x88
[    4.413423]  ufshcd_setup_clocks+0x2c0/0x490
[    4.413433]  ufshcd_init+0x198/0x10ec
[    4.413437]  ufshcd_pltfrm_init+0x600/0x7c0
[    4.413444]  ufs_qcom_probe+0x20/0x58
[    4.413449]  platform_probe+0x68/0xd8
[    4.413459]  really_probe+0xbc/0x268
[    4.413466]  __driver_probe_device+0x78/0x12c
[    4.413473]  driver_probe_device+0x40/0x11c
[    4.413481]  __device_attach_driver+0xb8/0xf8
[    4.413489]  bus_for_each_drv+0x84/0xe4
[    4.413495]  __device_attach+0xfc/0x18c
[    4.413502]  device_initial_probe+0x14/0x20
[    4.413510]  bus_probe_device+0xb0/0xb4
[    4.413517]  deferred_probe_work_func+0x8c/0xc8
[    4.413524]  process_scheduled_works+0x250/0x658
[    4.413534]  worker_thread+0x15c/0x2c8
[    4.413542]  kthread+0x134/0x200
[    4.413550]  ret_from_fork+0x10/0x20

As lockdep found, clk_gating.lock is uninitialized when ufshcd_setup_clocks() is
called for the first time. I looked into fixing it for a moment, but the overall
locking for 'clk_gating.state' looks fragile i.e., there are instances where the
code is not locked at all. So I'm just reporting to you here hoping that you'd
have some idea to fix it.

While submitting the fix, please add the following reported by tag:

Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

- Mani

-- 
மணிவண்ணன் சதாசிவம்