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

[Bart Van Assche <bvanassche@acm.org>]

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.