[v4] ufs: core: fix lockdep warning of clk_scaling_lock

Commit Message

Peter Wang (王信友) July 27, 2022, 3:21 a.m. UTC

From: Peter Wang <[e-mail]>

There have a lockdep warning like below in current flow, and have deadlock issue.
kworker/u16:0:  Possible unsafe locking scenario:

kworker/u16:0:        CPU0                    CPU1
kworker/u16:0:        ----                    ----
kworker/u16:0:   lock(&hba->clk_scaling_lock);
kworker/u16:0:                                lock(&hba->dev_cmd.lock);
kworker/u16:0:                                lock(&hba->clk_scaling_lock);
kworker/u16:0:   lock(&hba->dev_cmd.lock);
kworker/u16:0:

Before this patch clk_scaling_lock was held in reader mode during the ufshcd_wb_toggle() call.
With this patch applied clk_scaling_lock is not held while ufshcd_wb_toggle() is called.

This is safe because ufshcd_wb_toggle will held clk_scaling_lock in reader mode "again" in flow
ufshcd_wb_toggle -> __ufshcd_wb_toggle -> ufshcd_query_flag_retry -> ufshcd_query_flag ->
ufshcd_exec_dev_cmd -> down_read(&hba->clk_scaling_lock);
The protect should enough and make sure clock is not change while send command.

ufshcd_wb_toggle can protected by hba->clk_scaling.is_allowed to make sure
ufshcd_devfreq_scale function not run concurrently.

Fixes: 0e9d4ca43ba8 ("scsi: ufs: Protect some contexts from unexpected clock scaling")
Cc: <[e-mail]> # 5.15.x
Signed-off-by: Peter Wang <[e-mail]>
---
 drivers/ufs/core/ufshcd.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Comments

Bart Van Assche July 27, 2022, 6:12 p.m. UTC | #1

On 7/26/22 20:21, [e-mail] wrote:
> -	/* Enable Write Booster if we have scaled up else disable it */
> -	downgrade_write(&hba->clk_scaling_lock);
> -	is_writelock = false;
> -	ufshcd_wb_toggle(hba, scale_up);
> +	/* Disable clk_scaling until ufshcd_wb_toggle finish */
> +	hba->clk_scaling.is_allowed = false;
> +	wb_toggle = true;
>   
>   out_unprepare:
> -	ufshcd_clock_scaling_unprepare(hba, is_writelock);
> +	ufshcd_clock_scaling_unprepare(hba);
> +
> +	/* Enable Write Booster if we have scaled up else disable it */
> +	if (wb_toggle) {
> +		ufshcd_wb_toggle(hba, scale_up);
> +		ufshcd_clk_scaling_allow(hba, true);
> +	}

I'm concerned that briefly disabling clock scaling may cause the clock 
to remain at a high frequency even if it shouldn't. Has the following 
approach been considered? Instead of moving the 
ufshcd_clk_scaling_allow() call, convert dev_cmd.lock into a semaphore, 
lock it near the start of ufshcd_devfreq_scale() and unlock it near the 
end of the same function.

Thanks,

Bart.

Peter Wang (王信友) July 28, 2022, 7:13 a.m. UTC | #2

On 7/28/22 2:12 AM, Bart Van Assche wrote:
> On 7/26/22 20:21, [e-mail] wrote:
>> -    /* Enable Write Booster if we have scaled up else disable it */
>> -    downgrade_write(&hba->clk_scaling_lock);
>> -    is_writelock = false;
>> -    ufshcd_wb_toggle(hba, scale_up);
>> +    /* Disable clk_scaling until ufshcd_wb_toggle finish */
>> +    hba->clk_scaling.is_allowed = false;
>> +    wb_toggle = true;
>>     out_unprepare:
>> -    ufshcd_clock_scaling_unprepare(hba, is_writelock);
>> +    ufshcd_clock_scaling_unprepare(hba);
>> +
>> +    /* Enable Write Booster if we have scaled up else disable it */
>> +    if (wb_toggle) {
>> +        ufshcd_wb_toggle(hba, scale_up);
>> +        ufshcd_clk_scaling_allow(hba, true);
>> +    }
>
> I'm concerned that briefly disabling clock scaling may cause the clock 
> to remain at a high frequency even if it shouldn't. Has the following 
> approach been considered? Instead of moving the 
> ufshcd_clk_scaling_allow() call, convert dev_cmd.lock into a 
> semaphore, lock it near the start of ufshcd_devfreq_scale() and unlock 
> it near the end of the same function.
>
> Thanks,
>
> Bart.


Hi Bart,

Clock scaling up/down have a polling_ms, so it shouldn't have this 
condition that scale up block scale down.
Convert dev_cmd.lock into a semaphore is more risky, and dev_cmd.lock 
should hold when send dev command only.
I think it is not suitable to hold this dev_cmd.lock in 
ufshcd_devfreq_scale.

Maybe we can have another choice, let vendor decide ufshcd_wb_toggle 
with clock scaling or not?

Thanks.
Peter

Bart Van Assche July 28, 2022, 6:27 p.m. UTC | #3

On 7/28/22 00:13, Peter Wang wrote:
> Maybe we can have another choice, let vendor decide ufshcd_wb_toggle 
> with clock scaling or not?

I prefer that this code behaves identical for all UFS host controllers 
and all UFS devices. Making this code behave different per vendor would 
make it harder to read and to verify this code.

Thanks,

Bart.

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c7b337480e3e..aa57126fdb49 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -272,6 +272,7 @@  static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
 static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
 static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
+static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow);
 
 static inline void ufshcd_enable_irq(struct ufs_hba *hba)
 {
@@ -1249,12 +1250,10 @@  static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 	return ret;
 }
 
-static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
+static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
 {
-	if (writelock)
-		up_write(&hba->clk_scaling_lock);
-	else
-		up_read(&hba->clk_scaling_lock);
+	up_write(&hba->clk_scaling_lock);
+
 	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_release(hba);
 }
@@ -1271,7 +1270,7 @@  static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
 static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 {
 	int ret = 0;
-	bool is_writelock = true;
+	bool wb_toggle = false;
 
 	ret = ufshcd_clock_scaling_prepare(hba);
 	if (ret)
@@ -1300,13 +1299,19 @@  static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 		}
 	}
 
-	/* Enable Write Booster if we have scaled up else disable it */
-	downgrade_write(&hba->clk_scaling_lock);
-	is_writelock = false;
-	ufshcd_wb_toggle(hba, scale_up);
+	/* Disable clk_scaling until ufshcd_wb_toggle finish */
+	hba->clk_scaling.is_allowed = false;
+	wb_toggle = true;
 
 out_unprepare:
-	ufshcd_clock_scaling_unprepare(hba, is_writelock);
+	ufshcd_clock_scaling_unprepare(hba);
+
+	/* Enable Write Booster if we have scaled up else disable it */
+	if (wb_toggle) {
+		ufshcd_wb_toggle(hba, scale_up);
+		ufshcd_clk_scaling_allow(hba, true);
+	}
+
 	return ret;
 }