From: Peter Griffin <peter.griffin@linaro.org>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: linux-phy@lists.infradead.org, Vinod Koul <vkoul@kernel.org>,
Neil Armstrong <neil.armstrong@linaro.org>,
dri-devel@lists.freedesktop.org,
freedreno@lists.freedesktop.org,
linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, linux-can@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-ide@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
linux-pci@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-riscv@lists.infradead.org,
linux-rockchip@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-sunxi@lists.linux.dev, linux-tegra@vger.kernel.org,
linux-usb@vger.kernel.org, netdev@vger.kernel.org,
spacemit@lists.linux.dev, UNGLinuxDriver@microchip.com,
Bart Van Assche <bvanassche@acm.org>,
Alim Akhtar <alim.akhtar@samsung.com>,
"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Chanho Park <chanho61.park@samsung.com>
Subject: Re: [PATCH v6 phy-next 09/28] scsi: ufs: exynos: stop poking into struct phy guts
Date: Fri, 27 Mar 2026 20:23:54 +0000 [thread overview]
Message-ID: <CADrjBPqMwtrae7LB9A8xipg6R0rHGewe69awQ_jJnsf=2c=eTw@mail.gmail.com> (raw)
In-Reply-To: <20260327184706.1600329-10-vladimir.oltean@nxp.com>
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
>
next prev parent reply other threads:[~2026-03-27 20:24 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-27 18:46 [PATCH v6 phy-next 00/28] Split Generic PHY consumer and provider Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 01/28] ata: add <linux/pm_runtime.h> where missing Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 02/28] PCI: Add missing headers transitively included by <linux/phy/phy.h> Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 03/28] usb: add " Vladimir Oltean
2026-03-30 13:19 ` Greg Kroah-Hartman
2026-03-27 18:46 ` [PATCH v6 phy-next 04/28] drm: add <linux/pm_runtime.h> where missing Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 05/28] phy: " Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 06/28] phy: spacemit: include missing <linux/phy/phy.h> Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 07/28] net: lan969x: include missing <linux/of.h> Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 08/28] PCI: Remove device links to PHY Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 09/28] scsi: ufs: exynos: stop poking into struct phy guts Vladimir Oltean
2026-03-27 20:19 ` Martin K. Petersen
2026-03-27 20:23 ` Peter Griffin [this message]
2026-03-27 18:46 ` [PATCH v6 phy-next 10/28] scsi: ufs: qcom: keep parallel track of PHY power state Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 11/28] scsi: ufs: qcom: include missing <linux/interrupt.h> Vladimir Oltean
2026-03-27 20:20 ` Martin K. Petersen
2026-03-27 18:46 ` [PATCH v6 phy-next 12/28] drm/rockchip: dw_hdmi: avoid direct dereference of phy->dev.of_node Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 13/28] usb: host: tegra: " Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 14/28] usb: gadget: tegra-xudc: " Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 15/28] drm/msm/dp: remove debugging prints with internal struct phy state Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 16/28] phy: move provider API out of public <linux/phy/phy.h> Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 17/28] phy: make phy_get_mode(), phy_(get|set)_bus_width() NULL tolerant Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 18/28] phy: introduce phy_get_max_link_rate() helper for consumers Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 19/28] drm/rockchip: dsi: include PHY provider header Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 20/28] drm: bridge: cdns-mhdp8546: use consumer API for getting PHY bus width Vladimir Oltean
2026-03-27 18:46 ` [PATCH v6 phy-next 21/28] media: sunxi: a83-mips-csi2: include PHY provider header Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 22/28] net: renesas: rswitch: " Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 23/28] pinctrl: tegra-xusb: " Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 24/28] power: supply: cpcap-charger: include missing <linux/property.h> Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 25/28] phy: include PHY provider header (1/2) Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 26/28] phy: include PHY provider header (2/2) Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 27/28] phy: remove temporary provider compatibility from consumer header Vladimir Oltean
2026-03-27 18:47 ` [PATCH v6 phy-next 28/28] MAINTAINERS: add regexes for linux-phy Vladimir Oltean
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CADrjBPqMwtrae7LB9A8xipg6R0rHGewe69awQ_jJnsf=2c=eTw@mail.gmail.com' \
--to=peter.griffin@linaro.org \
--cc=James.Bottomley@hansenpartnership.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alim.akhtar@samsung.com \
--cc=bvanassche@acm.org \
--cc=chanho61.park@samsung.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=linux-tegra@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=neil.armstrong@linaro.org \
--cc=netdev@vger.kernel.org \
--cc=spacemit@lists.linux.dev \
--cc=vkoul@kernel.org \
--cc=vladimir.oltean@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).