Re: [PATCH v6 phy-next 09/28] scsi: ufs: exynos: stop poking into struct phy guts

Peter Griffin
2026-03-27T20:24:16Z

On Fri, 27 Mar 2026 at 18:48, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> The Exynos host controller driver is clearly a PHY consumer (gets the
> ufs->phy using devm_phy_get()), but pokes into the guts of struct phy
> to get the generic_phy->power_count.
>
> The UFS core (specifically ufshcd_link_startup()) may call the variant
> operation exynos_ufs_pre_link() -> exynos_ufs_phy_init() multiple times
> if the link startup fails and needs to be retried.
>
> However ufs-exynos shouldn't be doing what it's doing, i.e. looking at
> the generic_phy->power_count, because in the general sense of the API, a
> single Generic PHY may have multiple consumers. If ufs-exynos looks at
> generic_phy->power_count, there's no guarantee that this ufs-exynos
> instance is the one who previously bumped that power count. So it may be
> powering down the PHY on behalf of another consumer.
>
> The correct way in which this should be handled is ufs-exynos should
> *remember* whether it has initialized and powered up the PHY before, and
> power it down during link retries. Not rely on the power_count (which,
> btw, on the writer side is modified under &phy->mutex, but on the reader
> side is accessed unlocked). This is a discouraged pattern even if here
> it doesn't cause functional problems.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Acked-by: Alim Akhtar <alim.akhtar@samsung.com>
> Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Peter Griffin <peter.griffin@linaro.org>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Chanho Park <chanho61.park@samsung.com>
>
> v5->v6: collect tags from Alim Akhtar
> v4->v5: collect tag, add "scsi: " prefix to commit title
> v3->v4: none
> v2->v3:
> - add Cc Chanho Park, author of commit 3d73b200f989 ("scsi: ufs:
> ufs-exynos: Change ufs phy control sequence")
> v1->v2:
> - add better ufs->phy_powered_on handling in exynos_ufs_exit(),
> exynos_ufs_suspend() and exynos_ufs_resume() which ensures we won't
> enter a phy->power_count underrun condition
> ---
> drivers/ufs/host/ufs-exynos.c | 24 ++++++++++++++++++++----
> drivers/ufs/host/ufs-exynos.h | 1 +
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> index 76fee3a79c77..274e53833571 100644
> --- a/drivers/ufs/host/ufs-exynos.c
> +++ b/drivers/ufs/host/ufs-exynos.c
> @@ -963,9 +963,10 @@ static int exynos_ufs_phy_init(struct exynos_ufs *ufs)
>
> phy_set_bus_width(generic_phy, ufs->avail_ln_rx);
>
> - if (generic_phy->power_count) {
> + if (ufs->phy_powered_on) {
> phy_power_off(generic_phy);
> phy_exit(generic_phy);
> + ufs->phy_powered_on = false;
> }
>
> ret = phy_init(generic_phy);
> @@ -979,6 +980,8 @@ static int exynos_ufs_phy_init(struct exynos_ufs *ufs)
> if (ret)
> goto out_exit_phy;
>
> + ufs->phy_powered_on = true;
> +
> return 0;
>
> out_exit_phy:
> @@ -1527,6 +1530,9 @@ static void exynos_ufs_exit(struct ufs_hba *hba)
> {
> struct exynos_ufs *ufs = ufshcd_get_variant(hba);
>
> + if (!ufs->phy_powered_on)
> + return;
> +
> phy_power_off(ufs->phy);
> phy_exit(ufs->phy);
> }
> @@ -1728,8 +1734,10 @@ static int exynos_ufs_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
> if (ufs->drv_data->suspend)
> ufs->drv_data->suspend(ufs);
>
> - if (!ufshcd_is_link_active(hba))
> + if (!ufshcd_is_link_active(hba) && ufs->phy_powered_on) {
> phy_power_off(ufs->phy);
> + ufs->phy_powered_on = false;
> + }
>
> return 0;
> }
> @@ -1737,9 +1745,17 @@ static int exynos_ufs_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
> static int exynos_ufs_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> {
> struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> + int err;
>
> - if (!ufshcd_is_link_active(hba))
> - phy_power_on(ufs->phy);
> + if (!ufshcd_is_link_active(hba) && !ufs->phy_powered_on) {
> + err = phy_power_on(ufs->phy);
> + if (err) {
> + dev_err(hba->dev, "Failed to power on PHY: %pe\n",
> + ERR_PTR(err));
> + } else {
> + ufs->phy_powered_on = true;
> + }
> + }
>
> exynos_ufs_config_smu(ufs);
> exynos_ufs_fmp_resume(hba);
> diff --git a/drivers/ufs/host/ufs-exynos.h b/drivers/ufs/host/ufs-exynos.h
> index abe7e472759e..683b9150e2ba 100644
> --- a/drivers/ufs/host/ufs-exynos.h
> +++ b/drivers/ufs/host/ufs-exynos.h
> @@ -227,6 +227,7 @@ struct exynos_ufs {
> int avail_ln_rx;
> int avail_ln_tx;
> int rx_sel_idx;
> + bool phy_powered_on;
> struct ufs_pa_layer_attr dev_req_params;
> struct ufs_phy_time_cfg t_cfg;
> ktime_t entry_hibern8_t;
> --
> 2.43.0
>