On Wed, Mar 25, 2026 at 01:43:09PM +0200, Vladimir Oltean wrote:

On Tue, Mar 24, 2026 at 11:00:10AM +0530, Manivannan Sadhasivam wrote:

quoted

On Fri, Mar 20, 2026 at 12:32:24AM +0200, Vladimir Oltean wrote:

quoted

As explained in the similar ufs-exynos.c change, PHY consumer drivers
should not look at the phy->power_count, because in the general case
there might also be other consumers who have called phy_power_on() too,
so the fact that the power_count is non-zero does not mean that we did.

Moreover, struct phy will become opaque soon, so the qcom UFS driver
will not be able to apply this pattern. Keep parallel track of the PHY
power state, instead of looking at a field which will become unavailable
(phy->power_count).

About treating the phy_power_off() return code: from an API perspective,
this should have probably returned void, otherwise consumers would be
stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c,
does return 0 in its power_off() implementation. I consider it safe to
discard potential errors from phy_power_off() instead of complicating
the phy_powered_on logic.

You could even simplify the code by getting rid of the 'phy_powered_on' check
altogether. There is no real need to track the PHY power state in this driver.
It is safe to call phy_power_off() without any checks.

- Mani

Ok.. as the author of commit 7bac65687510 ("scsi: ufs: qcom: Power off
the PHY if it was already powered on in ufs_qcom_power_up_sequence()"),
I assume you have hardware to test. Would you mind writing a patch that
I could pick up to replace this one with?

Sure, will do.

I suppose that the power_count test is somehow no longer necessary after
commit 77d2fa54a945 ("scsi: ufs: qcom : Refactor phy_power_on/off
calls"), but frankly I don't see it - the ufshcd state machine is a bit
too complicated for me to just statically analyze.

I believe I added the power_count check for phy_exit(). But since that got
moved, the check becomes no longer necessary.

- Mani

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