[PATCH V5 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Manivannan Sadhasivam]

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

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