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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c428f074-c010-4225-960e-56aa65a799d8@acm.org>
Date: Mon, 12 May 2025 15:31:36 -0700
From: Bart Van Assche <bvanassche@....org>
To: Ziqi Chen <quic_ziqichen@...cinc.com>, quic_cang@...cinc.com,
 mani@...nel.org, beanhuo@...ron.com, avri.altman@....com,
 junwoo80.lee@...sung.com, martin.petersen@...cle.com,
 quic_nguyenb@...cinc.com, quic_nitirawa@...cinc.com,
 quic_rampraka@...cinc.com, neil.armstrong@...aro.org,
 luca.weiss@...rphone.com, konrad.dybcio@....qualcomm.com,
 peter.wang@...iatek.com
Cc: linux-arm-msm@...r.kernel.org, linux-scsi@...r.kernel.org,
 Alim Akhtar <alim.akhtar@...sung.com>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] scsi: ufs: core: skip UFS clkscale if host
 asynchronous scan in progress

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.