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: <zelvl7b5ov66lzbgay42ncdbkof3flv7g3gybqexth5hty6mvq@eemod4qy6gqs>
Date: Thu, 22 May 2025 15:01:20 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Nitin Rawat <quic_nitirawa@...cinc.com>
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 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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்