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

From: Peter Wang <peter.wang@mediatek.com>

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: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

On 7/28/22 2:12 AM, Bart Van Assche wrote:
> On 7/26/22 20:21, peter.wang@mediatek.com 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

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.