diff mbox series

[v3] scsi: ufs: core: skip UFS clkscale if host asynchronous scan in progress

Message ID 20250508093854.3281475-1-quic_ziqichen@quicinc.com
State Superseded
Headers show
Series [v3] scsi: ufs: core: skip UFS clkscale if host asynchronous scan in progress | expand

Commit Message

Ziqi Chen May 8, 2025, 9:38 a.m. UTC 
When preparing for UFS clock scaling, the UFS driver will quiesce all sdevs
queues on the UFS SCSI host tagset list and then unquiesce them when UFS
clock scaling unpreparing. If the UFS SCSI host async scan is in progress
at this time, some LUs may be added to the tagset list between UFS clkscale
prepare and unprepare. This can cause two issues:

1. During clock scaling, there may be IO requests issued through new added
queues that have not been quiesced, leading to task abort issue.

2. These new added queues that have not been quiesced will be unquiesced as
well when UFS clkscale is unprepared, resulting in warning prints.

Therefore, use the flag host->async_scan to check whether the host async
scan is in progress or not. Additionally, move ufshcd_devfreq_init() to
after ufshcd_add_lus() to ensure this flag already be set before starting
devfreq monitor.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
---

v1 -> v2:
Move whole clkscale Initialize process out of ufshcd_add_lus().

v2 -> v3:
Add check for the return value of ufshcd_add_lus().
---
 drivers/ufs/core/ufshcd.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

Comments

Bart Van Assche May 8, 2025, 4:06 p.m. UTC | #1 
On 5/8/25 2:38 AM, Ziqi Chen wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 1c53ccf5a616..04f40677e76a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1207,6 +1207,9 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>   	if (list_empty(head))
>   		return false;
>   
> +	if (hba->host->async_scan)
> +		return false;

Testing a boolean is never a proper way to synchronize code sections.
As an example, the SCSI core could set hba->host->async_scan after this
check completed and before the code below is executed. I think we need a
better solution.

Bart.
Ziqi Chen May 9, 2025, 5:02 a.m. UTC | #2 
On 5/9/2025 12:06 AM, Bart Van Assche wrote:
> On 5/8/25 2:38 AM, Ziqi Chen wrote:
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 1c53ccf5a616..04f40677e76a 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1207,6 +1207,9 @@ static bool 
>> ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>>       if (list_empty(head))
>>           return false;
>> +    if (hba->host->async_scan)
>> +        return false;
> 
> Testing a boolean is never a proper way to synchronize code sections.
> As an example, the SCSI core could set hba->host->async_scan after this
> check completed and before the code below is executed. I think we need a
> better solution.

Hi Bart,

I get your point, we have also taken this into consideration. That's why
we move ufshcd_devfreq_init() out of ufshd_add_lus().

Old sequence:

| ufshcd_async_scan()
   |ufshcd_add_lus()
     |ufshcd_devfreq_init()
     |  | enable UFS clock scaling
     |scsi_scan_host()
        |scsi_prep_async_scan()
        |    | set host->async_scan to '1'
        |async_schedule(do_scan_async, data)

With this old sequence , The ufs devfreq monitor started before the
scsi_prep_async_scan(),  the SCSI core could set hba->host->async_scan
after this check.

New sequence:

| ufshcd_async_scan()
   |ufshcd_add_lus()
   | |scsi_scan_host()
   |    |scsi_prep_async_scan()
   |    |    | set host->async_scan to '1'
   |    |async_schedule(do_scan_async, data)
   |ufshcd_devfreq_init()
   |    |enable UFS clock scaling

With the new sequence , it is guaranteed that host->async_scan
is set before the UFS clock scaling enabling.

I guess you might be worried about out-of-order execution will
cause this flag not be set before clock scaling enabling with
extremely low probability?
If yes, do you have any suggestion on this ?


BRs,
Ziqi


> 
> Bart.
Bart Van Assche May 12, 2025, 10:31 p.m. UTC | #3 
On 5/8/25 10:02 PM, Ziqi Chen wrote:
> 
> 
> On 5/9/2025 12:06 AM, Bart Van Assche wrote:
>> On 5/8/25 2:38 AM, Ziqi Chen wrote:
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index 1c53ccf5a616..04f40677e76a 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -1207,6 +1207,9 @@ static bool 
>>> ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>>>       if (list_empty(head))
>>>           return false;
>>> +    if (hba->host->async_scan)
>>> +        return false;
>>
>> Testing a boolean is never a proper way to synchronize code sections.
>> As an example, the SCSI core could set hba->host->async_scan after this
>> check completed and before the code below is executed. I think we need a
>> better solution.
> 
> Hi Bart,
> 
> I get your point, we have also taken this into consideration. That's why
> we move ufshcd_devfreq_init() out of ufshd_add_lus().
> 
> Old sequence:
> 
> | ufshcd_async_scan()
>    |ufshcd_add_lus()
>      |ufshcd_devfreq_init()
>      |  | enable UFS clock scaling
>      |scsi_scan_host()
>         |scsi_prep_async_scan()
>         |    | set host->async_scan to '1'
>         |async_schedule(do_scan_async, data)
> 
> With this old sequence , The ufs devfreq monitor started before the
> scsi_prep_async_scan(),  the SCSI core could set hba->host->async_scan
> after this check.
> 
> New sequence:
> 
> | ufshcd_async_scan()
>    |ufshcd_add_lus()
>    | |scsi_scan_host()
>    |    |scsi_prep_async_scan()
>    |    |    | set host->async_scan to '1'
>    |    |async_schedule(do_scan_async, data)
>    |ufshcd_devfreq_init()
>    |    |enable UFS clock scaling
> 
> With the new sequence , it is guaranteed that host->async_scan
> is set before the UFS clock scaling enabling.
> 
> I guess you might be worried about out-of-order execution will
> cause this flag not be set before clock scaling enabling with
> extremely low probability?
> If yes, do you have any suggestion on this ?

The new sequence depends on SCSI core internals that may change at
any time. SCSI drivers like the UFS drivers shouldn't depend on this
behavior since there are no guarantees that this behavior won't change.

Can host->scan_mutex be used to serialize clock scaling and LUN
scanning? I think this mutex is already used by a SCSI driver to
serialize against LUN addition and removal (storvsc).

Thanks,

Bart.
Ziqi Chen May 14, 2025, 7:25 a.m. UTC | #4 
On 5/13/2025 6:31 AM, Bart Van Assche wrote:
> On 5/8/25 10:02 PM, Ziqi Chen wrote:
>>
>>
>> On 5/9/2025 12:06 AM, Bart Van Assche wrote:
>>> On 5/8/25 2:38 AM, Ziqi Chen wrote:
>>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>>> index 1c53ccf5a616..04f40677e76a 100644
>>>> --- a/drivers/ufs/core/ufshcd.c
>>>> +++ b/drivers/ufs/core/ufshcd.c
>>>> @@ -1207,6 +1207,9 @@ static bool 
>>>> ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>>>>       if (list_empty(head))
>>>>           return false;
>>>> +    if (hba->host->async_scan)
>>>> +        return false;
>>>
>>> Testing a boolean is never a proper way to synchronize code sections.
>>> As an example, the SCSI core could set hba->host->async_scan after this
>>> check completed and before the code below is executed. I think we need a
>>> better solution.
>>
>> Hi Bart,
>>
>> I get your point, we have also taken this into consideration. That's why
>> we move ufshcd_devfreq_init() out of ufshd_add_lus().
>>
>> Old sequence:
>>
>> | ufshcd_async_scan()
>>    |ufshcd_add_lus()
>>      |ufshcd_devfreq_init()
>>      |  | enable UFS clock scaling
>>      |scsi_scan_host()
>>         |scsi_prep_async_scan()
>>         |    | set host->async_scan to '1'
>>         |async_schedule(do_scan_async, data)
>>
>> With this old sequence , The ufs devfreq monitor started before the
>> scsi_prep_async_scan(),  the SCSI core could set hba->host->async_scan
>> after this check.
>>
>> New sequence:
>>
>> | ufshcd_async_scan()
>>    |ufshcd_add_lus()
>>    | |scsi_scan_host()
>>    |    |scsi_prep_async_scan()
>>    |    |    | set host->async_scan to '1'
>>    |    |async_schedule(do_scan_async, data)
>>    |ufshcd_devfreq_init()
>>    |    |enable UFS clock scaling
>>
>> With the new sequence , it is guaranteed that host->async_scan
>> is set before the UFS clock scaling enabling.
>>
>> I guess you might be worried about out-of-order execution will
>> cause this flag not be set before clock scaling enabling with
>> extremely low probability?
>> If yes, do you have any suggestion on this ?
> 
> The new sequence depends on SCSI core internals that may change at
> any time. SCSI drivers like the UFS drivers shouldn't depend on this
> behavior since there are no guarantees that this behavior won't change.
> 
> Can host->scan_mutex be used to serialize clock scaling and LUN
> scanning? I think this mutex is already used by a SCSI driver to
> serialize against LUN addition and removal (storvsc).
> 
Hi Bart,

I tried the scan_mutex, from debugging logs, it seems okay for now.
I will provide to our internal test team for stability test.
And I will try to collect the extra time spent on clock scaling
path with applying scan_mutex.
If everything is fine, I will update a new version.

BRs,
Ziqi

> Thanks,
> 
> Bart.
Peter Wang (王信友) May 14, 2025, 9:05 a.m. UTC | #5 
On Wed, 2025-05-14 at 15:25 +0800, Ziqi Chen wrote:
> 
> Hi Bart,
> 
> I tried the scan_mutex, from debugging logs, it seems okay for now.
> I will provide to our internal test team for stability test.
> And I will try to collect the extra time spent on clock scaling
> path with applying scan_mutex.
> If everything is fine, I will update a new version.
> 
> BRs,
> Ziqi
> 
> 
> 

Hi Ziqi,

Could we enable devfreq when checking if hba->luns_avail equals 1 
in ufshcd_device_configure?
I think we can use flow to ensure correctness; it doesn't 
necessarily need to be protected by a mutex.

Thanks
Peter
Ziqi Chen May 21, 2025, 8:56 a.m. UTC | #6 
On 5/14/2025 5:05 PM, Peter Wang (王信友) wrote:
> On Wed, 2025-05-14 at 15:25 +0800, Ziqi Chen wrote:
>>
>> Hi Bart,
>>
>> I tried the scan_mutex, from debugging logs, it seems okay for now.
>> I will provide to our internal test team for stability test.
>> And I will try to collect the extra time spent on clock scaling
>> path with applying scan_mutex.
>> If everything is fine, I will update a new version.
>>
>> BRs,
>> Ziqi
>>
>>
>>
> 
> Hi Ziqi,
> 
> Could we enable devfreq when checking if hba->luns_avail equals 1
> in ufshcd_device_configure?
> I think we can use flow to ensure correctness; it doesn't
> necessarily need to be protected by a mutex.
> 
> Thanks
> Peter

Hi Peter,

Thanks for your suggestion, and sorry for the late response—I was out of
office last week.

It looks like checking hba->luns_avail == 1 in ufshcd_device_configure
before enabling devfreq can ensure correctness. But I think the
function name 'ufshcd_device_configure' suggests that enabling devfreq
inside it is not reasonable. That’s another same reason why we moved
ufshcd_devfreq_init() out of ufshcd_add_lus() in this patch.

BRs,
Ziqi

> 
>
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1c53ccf5a616..04f40677e76a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1207,6 +1207,9 @@  static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
 	if (list_empty(head))
 		return false;
 
+	if (hba->host->async_scan)
+		return false;
+
 	if (hba->use_pm_opp)
 		return freq != hba->clk_scaling.target_freq;
 
@@ -8740,21 +8743,6 @@  static int ufshcd_add_lus(struct ufs_hba *hba)
 	if (ret)
 		goto out;
 
-	/* Initialize devfreq after UFS device is detected */
-	if (ufshcd_is_clkscaling_supported(hba)) {
-		memcpy(&hba->clk_scaling.saved_pwr_info,
-			&hba->pwr_info,
-			sizeof(struct ufs_pa_layer_attr));
-		hba->clk_scaling.is_allowed = true;
-
-		ret = ufshcd_devfreq_init(hba);
-		if (ret)
-			goto out;
-
-		hba->clk_scaling.is_enabled = true;
-		ufshcd_init_clk_scaling_sysfs(hba);
-	}
-
 	/*
 	 * The RTC update code accesses the hba->ufs_device_wlun->sdev_gendev
 	 * pointer and hence must only be started after the WLUN pointer has
@@ -9009,6 +8997,23 @@  static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 
 	/* Probe and add UFS logical units  */
 	ret = ufshcd_add_lus(hba);
+	if (ret)
+		goto out;
+
+	/* Initialize devfreq and start devfreq monitor */
+	if (ufshcd_is_clkscaling_supported(hba)) {
+		memcpy(&hba->clk_scaling.saved_pwr_info,
+			&hba->pwr_info,
+			sizeof(struct ufs_pa_layer_attr));
+		hba->clk_scaling.is_allowed = true;
+
+		ret = ufshcd_devfreq_init(hba);
+		if (ret)
+			goto out;
+
+		hba->clk_scaling.is_enabled = true;
+		ufshcd_init_clk_scaling_sysfs(hba);
+	}
 
 out:
 	pm_runtime_put_sync(hba->dev);