diff mbox series

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

Message ID 20250515162722.6933-11-quic_nitirawa@quicinc.com
State New
Headers show
Series Refactor ufs phy powerup sequence | expand

Commit Message

Nitin Rawat May 15, 2025, 4:27 p.m. UTC 
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.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 44 +++++++++++++++++++++++++++++++------
 drivers/ufs/host/ufs-qcom.h |  4 ++++
 2 files changed, 41 insertions(+), 7 deletions(-)

Comments

Manivannan Sadhasivam May 21, 2025, 2:01 p.m. UTC | #1 
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.

- Mani

> Co-developed-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 44 +++++++++++++++++++++++++++++++------
>  drivers/ufs/host/ufs-qcom.h |  4 ++++
>  2 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 3ee4ab90dfba..583db910efd4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -479,6 +479,38 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
>  	return UFS_HS_G3;
>  }
>  
> +static int ufs_qcom_phy_power_on(struct ufs_hba *hba)
> +{
> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	struct phy *phy = host->generic_phy;
> +	int ret = 0;
> +
> +	guard(mutex)(&host->phy_mutex);
> +	if (!host->is_phy_pwr_on) {
> +		ret = phy_power_on(phy);
> +		if (!ret)
> +			host->is_phy_pwr_on = true;
> +	}
> +
> +	return ret;
> +}
> +
> +static int ufs_qcom_phy_power_off(struct ufs_hba *hba)
> +{
> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	struct phy *phy = host->generic_phy;
> +	int ret = 0;
> +
> +	guard(mutex)(&host->phy_mutex);
> +	if (host->is_phy_pwr_on) {
> +		ret = phy_power_off(phy);
> +		if (!ret)
> +			host->is_phy_pwr_on = false;
> +	}
> +
> +	return ret;
> +}
> +
>  static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> @@ -507,7 +539,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  		return ret;
>  
>  	if (phy->power_count) {
> -		phy_power_off(phy);
> +		ufs_qcom_phy_power_off(hba);
>  		phy_exit(phy);
>  	}
>  
> @@ -524,7 +556,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  		goto out_disable_phy;
>  
>  	/* power on phy - start serdes and phy's power and clocks */
> -	ret = phy_power_on(phy);
> +	ret = ufs_qcom_phy_power_on(hba);
>  	if (ret) {
>  		dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
>  			__func__, ret);
> @@ -1121,7 +1153,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  				 enum ufs_notify_change_status status)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	struct phy *phy = host->generic_phy;
>  	int err;
>  
>  	/*
> @@ -1142,7 +1173,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  				ufs_qcom_dev_ref_clk_ctrl(host, false);
>  			}
>  
> -			err = phy_power_off(phy);
> +			err = ufs_qcom_phy_power_off(hba);
>  			if (err) {
>  				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
>  				return err;
> @@ -1151,7 +1182,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  		break;
>  	case POST_CHANGE:
>  		if (on) {
> -			err = phy_power_on(phy);
> +			err = ufs_qcom_phy_power_on(hba);
>  			if (err) {
>  				dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
>  				return err;
> @@ -1339,10 +1370,9 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  static void ufs_qcom_exit(struct ufs_hba *hba)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	struct phy *phy = host->generic_phy;
>  
>  	ufs_qcom_disable_lane_clks(host);
> -	phy_power_off(phy);
> +	ufs_qcom_phy_power_off(hba);
>  	phy_exit(host->generic_phy);
>  }
>  
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 0a5cfc2dd4f7..f51b774e17be 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -281,6 +281,10 @@ struct ufs_qcom_host {
>  	u32 phy_gear;
>  
>  	bool esi_enabled;
> +	/* flag to check if phy is powered on */
> +	bool is_phy_pwr_on;
> +	/* Protect the usage of is_phy_pwr_on against racing */
> +	struct mutex phy_mutex;
>  };
>  
>  struct ufs_qcom_drvdata {
> -- 
> 2.48.1
>
Nitin Rawat May 21, 2025, 10:18 p.m. UTC | #2 
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.

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.

Regards,
Nitin



>> +			host->is_phy_pwr_on = false;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> @@ -507,7 +539,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>>   		return ret;
>>   
>>   	if (phy->power_count) {
>> -		phy_power_off(phy);
>> +		ufs_qcom_phy_power_off(hba);
>>   		phy_exit(phy);
>>   	}
>>   
>> @@ -524,7 +556,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>>   		goto out_disable_phy;
>>   
>>   	/* power on phy - start serdes and phy's power and clocks */
>> -	ret = phy_power_on(phy);
>> +	ret = ufs_qcom_phy_power_on(hba);
>>   	if (ret) {
>>   		dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
>>   			__func__, ret);
>> @@ -1121,7 +1153,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>   				 enum ufs_notify_change_status status)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	struct phy *phy = host->generic_phy;
>>   	int err;
>>   
>>   	/*
>> @@ -1142,7 +1173,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>   				ufs_qcom_dev_ref_clk_ctrl(host, false);
>>   			}
>>   
>> -			err = phy_power_off(phy);
>> +			err = ufs_qcom_phy_power_off(hba);
>>   			if (err) {
>>   				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
>>   				return err;
>> @@ -1151,7 +1182,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>   		break;
>>   	case POST_CHANGE:
>>   		if (on) {
>> -			err = phy_power_on(phy);
>> +			err = ufs_qcom_phy_power_on(hba);
>>   			if (err) {
>>   				dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
>>   				return err;
>> @@ -1339,10 +1370,9 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>>   static void ufs_qcom_exit(struct ufs_hba *hba)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	struct phy *phy = host->generic_phy;
>>   
>>   	ufs_qcom_disable_lane_clks(host);
>> -	phy_power_off(phy);
>> +	ufs_qcom_phy_power_off(hba);
>>   	phy_exit(host->generic_phy);
>>   }
>>   
>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>> index 0a5cfc2dd4f7..f51b774e17be 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -281,6 +281,10 @@ struct ufs_qcom_host {
>>   	u32 phy_gear;
>>   
>>   	bool esi_enabled;
>> +	/* flag to check if phy is powered on */
>> +	bool is_phy_pwr_on;
>> +	/* Protect the usage of is_phy_pwr_on against racing */
>> +	struct mutex phy_mutex;
>>   };
>>   
>>   struct ufs_qcom_drvdata {
>> -- 
>> 2.48.1
>>
>
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 3ee4ab90dfba..583db910efd4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -479,6 +479,38 @@  static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
 	return UFS_HS_G3;
 }
 
+static int ufs_qcom_phy_power_on(struct ufs_hba *hba)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct phy *phy = host->generic_phy;
+	int ret = 0;
+
+	guard(mutex)(&host->phy_mutex);
+	if (!host->is_phy_pwr_on) {
+		ret = phy_power_on(phy);
+		if (!ret)
+			host->is_phy_pwr_on = true;
+	}
+
+	return ret;
+}
+
+static int ufs_qcom_phy_power_off(struct ufs_hba *hba)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct phy *phy = host->generic_phy;
+	int ret = 0;
+
+	guard(mutex)(&host->phy_mutex);
+	if (host->is_phy_pwr_on) {
+		ret = phy_power_off(phy);
+		if (!ret)
+			host->is_phy_pwr_on = false;
+	}
+
+	return ret;
+}
+
 static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
@@ -507,7 +539,7 @@  static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		return ret;
 
 	if (phy->power_count) {
-		phy_power_off(phy);
+		ufs_qcom_phy_power_off(hba);
 		phy_exit(phy);
 	}
 
@@ -524,7 +556,7 @@  static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		goto out_disable_phy;
 
 	/* power on phy - start serdes and phy's power and clocks */
-	ret = phy_power_on(phy);
+	ret = ufs_qcom_phy_power_on(hba);
 	if (ret) {
 		dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
 			__func__, ret);
@@ -1121,7 +1153,6 @@  static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				 enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct phy *phy = host->generic_phy;
 	int err;
 
 	/*
@@ -1142,7 +1173,7 @@  static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				ufs_qcom_dev_ref_clk_ctrl(host, false);
 			}
 
-			err = phy_power_off(phy);
+			err = ufs_qcom_phy_power_off(hba);
 			if (err) {
 				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
 				return err;
@@ -1151,7 +1182,7 @@  static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 		break;
 	case POST_CHANGE:
 		if (on) {
-			err = phy_power_on(phy);
+			err = ufs_qcom_phy_power_on(hba);
 			if (err) {
 				dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
 				return err;
@@ -1339,10 +1370,9 @@  static int ufs_qcom_init(struct ufs_hba *hba)
 static void ufs_qcom_exit(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct phy *phy = host->generic_phy;
 
 	ufs_qcom_disable_lane_clks(host);
-	phy_power_off(phy);
+	ufs_qcom_phy_power_off(hba);
 	phy_exit(host->generic_phy);
 }
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 0a5cfc2dd4f7..f51b774e17be 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -281,6 +281,10 @@  struct ufs_qcom_host {
 	u32 phy_gear;
 
 	bool esi_enabled;
+	/* flag to check if phy is powered on */
+	bool is_phy_pwr_on;
+	/* Protect the usage of is_phy_pwr_on against racing */
+	struct mutex phy_mutex;
 };
 
 struct ufs_qcom_drvdata {