Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
functions with mutex protection to ensure safe usage of is_phy_pwr_on
and prevent possible 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(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ff35cd15c72f..a7e9e06847f8 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -421,6 +421,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);
@@ -449,7 +481,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);
}
@@ -466,7 +498,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);
@@ -1018,7 +1050,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;
/*
@@ -1038,7 +1069,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
/* disable device ref_clk */
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, "%s: phy power off failed, ret=%d\n",
__func__, err);
@@ -1048,7 +1079,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, "%s: phy power on failed, ret = %d\n",
__func__, err);
@@ -1236,10 +1267,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 d0e6ec9128e7..3db29fbcd40b 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -252,6 +252,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.1On 5/3/25 6:24 PM, Nitin Rawat wrote: > Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper > functions with mutex protection to ensure safe usage of is_phy_pwr_on > and prevent possible 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> > --- The PHY framework does the same thing internally already, this seems unnecessary Konrad
On 5/9/2025 5:07 PM, Konrad Dybcio wrote:
> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
>> functions with mutex protection to ensure safe usage of is_phy_pwr_on
>> and prevent possible 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>
>> ---
>
> The PHY framework does the same thing internally already, this seems
> unnecessary
Hi Konrad,
Thanks for the review. There are scenarios where ufshcd_link_startup()
can call ufshcd_vops_link_startup_notify() multiple times during
retries. This leads to the PHY reference count increasing continuously,
preventing proper re-initialization of the PHY.
Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs:
qcom: Power off the PHY if it was already powered on in
ufs_qcom_power_up_sequence()"). However, I still want to maintain a
reference count (ref_cnt) to safeguard against similar conditions in the
code. Additionally, this approach helps avoid unnecessary phy_power_on
and phy_power_off calls. Please let me know your thoughts.
Regards,
Nitin
>
> Konrad
On 5/9/25 1:49 PM, Nitin Rawat wrote:
>
>
> On 5/9/2025 5:07 PM, Konrad Dybcio wrote:
>> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
>>> functions with mutex protection to ensure safe usage of is_phy_pwr_on
>>> and prevent possible 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>
>>> ---
>>
>> The PHY framework does the same thing internally already, this seems
>> unnecessary
>
> Hi Konrad,
>
> Thanks for the review. There are scenarios where ufshcd_link_startup() can call ufshcd_vops_link_startup_notify() multiple times during retries. This leads to the PHY reference count increasing continuously, preventing proper re-initialization of the PHY.
I'm assuming you're talking about the scenario where it jumps into
ufs_qcom_power_up_sequence() - you have a label in there called
`out_disable_phy` - add a phy_power_off() after phy_calibrate if
things fail and you should be good to go if I'm reading things right.
Please include something resembling a call stack in the commit message,
as currently everyone reviewing this has to make guesses about why this
needs to be done
> Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs:
> qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()"). However, I still want to maintain a reference count (ref_cnt) to safeguard against similar conditions in the code. Additionally, this approach helps avoid unnecessary phy_power_on and phy_power_off calls. Please let me know your thoughts.
These unnecessary calls only amount to a couple of jumps and compares,
just like your wrappers, as the framework keeps track of the enable
count as well
Konrad
On 5/9/2025 5:30 PM, Konrad Dybcio wrote:
> On 5/9/25 1:49 PM, Nitin Rawat wrote:
>>
>>
>> On 5/9/2025 5:07 PM, Konrad Dybcio wrote:
>>> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>>>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
>>>> functions with mutex protection to ensure safe usage of is_phy_pwr_on
>>>> and prevent possible 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>
>>>> ---
>>>
>>> The PHY framework does the same thing internally already, this seems
>>> unnecessary
>>
>> Hi Konrad,
>>
>> Thanks for the review. There are scenarios where ufshcd_link_startup() can call ufshcd_vops_link_startup_notify() multiple times during retries. This leads to the PHY reference count increasing continuously, preventing proper re-initialization of the PHY.
>
> I'm assuming you're talking about the scenario where it jumps into
> ufs_qcom_power_up_sequence() - you have a label in there called
> `out_disable_phy` - add a phy_power_off() after phy_calibrate if
> things fail and you should be good to go if I'm reading things right.
Hi Konrad,
I meant, ufs_qcom_power_up_sequence can be called multiple times from
ufshcd_link_startup as part of ufshcd_hba_enable calls for each
retries(max retries =3) and each attempt of ufs_qcom_power_up_sequence
is success increasing the power_count ref to value more than 1.
But this is handled using the patch 7bac65687510.
Similiar scenarios can be possible in ufs driver , where there can be 2
phy_power_on calls which may caused caused phy ref count to be more than
1 and this inconsistent behaviour may cause issue.
Hence having is_phy_pwr_on flag can help here.
Thanks,
Nitin
>
> Please include something resembling a call stack in the commit message,
> as currently everyone reviewing this has to make guesses about why this
> needs to be done
>
>
>> Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs:
>> qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()"). However, I still want to maintain a reference count (ref_cnt) to safeguard against similar conditions in the code. Additionally, this approach helps avoid unnecessary phy_power_on and phy_power_off calls. Please let me know your thoughts.
>
> These unnecessary calls only amount to a couple of jumps and compares,
> just like your wrappers, as the framework keeps track of the enable
> count as well
>
> Konrad
© 2016 - 2026 Red Hat, Inc.