On Fri, 27 Mar 2026 at 18:48, Vladimir Oltean <[email protected]> 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 <[email protected]> > Reviewed-by: Bart Van Assche <[email protected]> > Acked-by: Alim Akhtar <[email protected]> > Tested-by: Alim Akhtar <[email protected]> > ---
Reviewed-by: Peter Griffin <[email protected]> > Cc: Alim Akhtar <[email protected]> > Cc: Peter Griffin <[email protected]> > Cc: "James E.J. Bottomley" <[email protected]> > Cc: "Martin K. Petersen" <[email protected]> > Cc: Krzysztof Kozlowski <[email protected]> > Cc: Chanho Park <[email protected]> > > 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 >
