linux-kernel - Re: [PATCH V5 10/11] scsi: ufs: qcom : Introduce phy_power

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: <226fa4de-f3a7-4bf0-a960-263443af7de8@quicinc.com>
Date: Thu, 22 May 2025 15:56:30 +0530
From: Nitin Rawat <quic_nitirawa@...cinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: <vkoul@...nel.org>, <kishon@...nel.org>,
        <James.Bottomley@...senpartnership.com>, <martin.petersen@...cle.com>,
        <bvanassche@....org>, <andersson@...nel.org>,
        <neil.armstrong@...aro.org>, <dmitry.baryshkov@....qualcomm.com>,
        <konrad.dybcio@....qualcomm.com>, <quic_rdwivedi@...cinc.com>,
        <quic_cang@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
        <linux-phy@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH V5 10/11] scsi: ufs: qcom : Introduce phy_power_on/off
 wrapper function



On 5/22/2025 3:01 PM, Manivannan Sadhasivam wrote:
> On Thu, May 22, 2025 at 03:48:29AM +0530, Nitin Rawat wrote:
>>
>>
>> On 5/21/2025 7:31 PM, Manivannan Sadhasivam wrote:
>>> On Thu, May 15, 2025 at 09:57:21PM +0530, Nitin Rawat wrote:
>>>
>>> Subject should mention ufs_qcom_phy_power_{on/off} as phy_power_{on/off} are
>>> generic APIs.
>>>
>>>> There can be scenarios where phy_power_on is called when PHY is
>>>> already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
>>>> can be called multiple times from ufshcd_link_startup as part of
>>>> ufshcd_hba_enable call for each link startup retries(max retries =3),
>>>> causing the PHY reference count to increase and leading to inconsistent
>>>> phy behavior.
>>>>
>>>> Similarly, there can be scenarios where phy_power_on or phy_power_off
>>>> might be called directly from the UFS controller driver when the PHY
>>>> is already powered on or off respectiely, causing similar issues.
>>>>
>>>> To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
>>>> wrappers for phy_power_on and phy_power_off. These wrappers will use an
>>>> is_phy_pwr_on flag to check if the PHY is already powered on or off,
>>>> avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
>>>> to ensure safe usage and prevent race conditions.
>>>>
>>>
>>> This smells like the phy_power_{on/off} calls are not balanced and you are
>>> trying to workaround that in the UFS driver.
>>
>> Hi Mani,
>>
>> Yes, there can be scenarios that were not previously encountered because
>> phy_power_on and phy_power_off were only called during system suspend
>> (spm_lvl = 5). However, with phy_power_on now moved to
>> ufs_qcom_setup_clocks, there is a slightly more probability of phy_power_on
>> being called twice, i.e., phy_power_on being invoked when the PHY is already
>> on.
>>
>> For instance, if the PHY power is already on and the UFS driver calls
>> ufs_qcom_setup_clocks from an error handling context, phy_power_on could be
>> called again which may increase phy_count and can cause inconsistent phy
>> bheaviour . Therefore, we need to have a flag, is_phy_pwr_on, in the
>> controller driver, protected by a mutex, to indicate the state of
>> phy_power_on and phy_power_off.
>>
> 
> If phy_power_on() is called twice without phy_power_off(), there can be only 2
> possibilities:
> 
> 1. phy_power_off() is not balanced
> 2. phy_power_on() is called from a wrong place
> 
>> This approach is also present in Qualcomm downstream UFS driver and similiar
>> solution in mtk ufs driver to have flag in controller indictring phy power
>> state in their upstream UFS drivers.
>>
> 
> No, having this check in the host driver is clearly a workaround for a broken
> behavior. I do not want to carry this mess all along.
> 

Hi Mani,

I double checked the code again error handling scenarios which i mention 
is my earlier reply is actually under runtime suspend check so this 
issue won't occur there which means as of now we have no scenarios which 
exists as per our understanding where existing code will break w.r.t 
phy_on/_off , hence i can drop this patch from this series.

Regards,
Nitin


> - Mani
>