| 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
| ||
|
Message-ID: <f93dc302-f10d-40c6-a852-cd76ad5e74da@quicinc.com>
Date: Thu, 10 Apr 2025 21:11:17 +0530
From: Nitin Rawat <quic_nitirawa@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <vkoul@...nel.org>, <kishon@...nel.org>,
<manivannan.sadhasivam@...aro.org>,
<James.Bottomley@...senpartnership.com>, <martin.petersen@...cle.com>,
<konrad.dybcio@....qualcomm.com>, <quic_rdwivedi@...cinc.com>,
<linux-arm-msm@...r.kernel.org>, <linux-phy@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
Can Guo <quic_cang@...cinc.com>
Subject: Re: [PATCH V2 6/6] scsi: ufs: host : Introduce phy_power_on/off
wrapper function
On 3/19/2025 1:20 AM, Bjorn Andersson wrote:
> On Tue, Mar 18, 2025 at 08:19:44PM +0530, 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.
>>
>
> Please describe the problem properly here. Are you introducing the mutex
> because there is a problem, or because you want to be "safe"?
>
> Why is it "refcounted", is the calling code paths no longer in sync? How
> long has the current implementation been broken?
>
> Regards,
> Bjorn
>
Hi Bjorn,
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.
Regrads,
Nitin
>> Co-developed-by: Can Guo <quic_cang@...cinc.com>
>> Signed-off-by: Can Guo <quic_cang@...cinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.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 5c7b6c75d669..8f80724e64b9 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);
>> @@ -1017,7 +1049,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;
>>
>> /*
>> @@ -1037,7 +1068,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, "phy power off failed, ret=%d\n", err);
>> return err;
>> @@ -1046,7 +1077,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;
>> @@ -1233,10 +1264,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.1
>>
>>
Powered by blists - more mailing lists