linux-kernel - Re: [PATCH V4 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: <4a72ea06-22a8-4f8c-92ad-b5b3afa25b70@oss.qualcomm.com>
Date: Fri, 9 May 2025 14:00:48 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Nitin Rawat <quic_nitirawa@...cinc.com>, vkoul@...nel.org,
        kishon@...nel.org, manivannan.sadhasivam@...aro.org,
        James.Bottomley@...senPartnership.com, martin.petersen@...cle.com,
        bvanassche@....org, andersson@...nel.org, neil.armstrong@...aro.org
Cc: 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 V4 10/11] scsi: ufs: qcom : Introduce phy_power_on/off
 wrapper function

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@...cinc.com>
>>> Signed-off-by: Can Guo <quic_cang@...cinc.com>
>>> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.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