[PATCH v2 0/2] phy: starfive: Fix JH7110 DPHY runtime PM cleanup

[PATCH v2 0/2] phy: starfive: Fix JH7110 DPHY runtime PM cleanup

[Can Peng]

The JH7110 DPHY TX and RX drivers both enable runtime PM during probe
without arranging for it to be disabled on later probe failures or on
driver unbind.

Use devm_pm_runtime_enable() in both drivers so the runtime PM state is
cleaned up automatically when probe fails after runtime PM has been
enabled and when the driver is unbound.

Changes in v2:
- Turn the single TX fix into a two-patch series.
- Add the matching runtime PM cleanup fix for the RX driver.

Can Peng (2):
  phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe
  phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe

 drivers/phy/starfive/phy-jh7110-dphy-rx.c | 5 ++++-
 drivers/phy/starfive/phy-jh7110-dphy-tx.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.53.0

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 1/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

[Can Peng]

stf_dphy_probe() enables runtime PM before getting the clock and
reset controls, creating the PHY and registering the PHY provider. If
any of those steps fails, probe returns with runtime PM still enabled.

The driver also has no remove callback, so runtime PM is left enabled
on driver unbind after a successful probe.

Use devm_pm_runtime_enable() so runtime PM is disabled automatically
on later probe failures and on driver unbind.

Fixes: d3ab79553308 ("phy: starfive: Add mipi dphy tx support")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 drivers/phy/starfive/phy-jh7110-dphy-tx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/starfive/phy-jh7110-dphy-tx.c b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
index c64d1c91b130..181491a93807 100644
--- a/drivers/phy/starfive/phy-jh7110-dphy-tx.c
+++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
@@ -392,6 +392,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
 {
 	struct phy_provider *phy_provider;
 	struct stf_dphy *dphy;
+	int ret;
 
 	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);
 	if (!dphy)
@@ -406,7 +407,9 @@ static int stf_dphy_probe(struct platform_device *pdev)
 	if (IS_ERR(dphy->topsys))
 		return PTR_ERR(dphy->topsys);
 
-	pm_runtime_enable(&pdev->dev);
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
 
 	dphy->txesc_clk = devm_clk_get(&pdev->dev, "txesc");
 	if (IS_ERR(dphy->txesc_clk))
-- 
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free (UAF) of `dphy` when the PHY provider is unbound but a consumer is still active.
- [Medium] The runtime PM fix is incomplete as it misses the identical leak in the companion RX driver.
- [Medium] Resource leak of `txesc_clk` on the error path of `stf_dphy_init()`.
- [Medium] Incomplete hardware teardown and clock leak in `stf_dphy_exit()` upon reset failure.
--

--- Patch [1]: [PATCH v2 1/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe ---
commit a6c202e493e39186dc1b4cf599b709a5e76356a2
Author: Can Peng <pengcan@kylinos.cn>
Subject: phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

This commit changes pm_runtime_enable() to devm_pm_runtime_enable() in the
JH7110 DPHY TX driver's probe function. This ensures that runtime PM is
properly disabled on probe failures or driver unbind.

> diff --git a/drivers/phy/starfive/phy-jh7110-dphy-tx.c b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> index c64d1c91b1307..181491a938079 100644
> --- a/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> @@ -392,6 +392,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
>  	struct stf_dphy *dphy;
> +	int ret;
>  
>  	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but is it safe to tie the lifetime of dphy to
the provider device using devm_kzalloc? 

If the provider is unbound via sysfs, devres will automatically free dphy.
However, a consumer might still be bound due to a stateless device link, and
if it calls phy_power_off(), it triggers stf_dphy_power_off() which could
dereference the freed dphy->dev:

stf_dphy_power_off() {
    ...
    struct stf_dphy *dphy = phy_get_drvdata(phy);

    return pm_runtime_put_sync(dphy->dev);
}

>  	if (!dphy)
>  		return -ENOMEM;

[ ... ]

> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);

[Severity: Medium]
This isn't a bug introduced by this patch, but does the companion RX driver
in drivers/phy/starfive/phy-jh7110-dphy-rx.c also need this same fix? 

It appears to have the exact same pattern where pm_runtime_enable() is not
managed by devres and is not reverted on a subsequent provider registration
error or driver unbind:

stf_dphy_probe() {
    ...
    pm_runtime_enable(&pdev->dev);
    
    phy_set_drvdata(dphy->phy, dphy);
    phy_provider = devm_of_phy_provider_register(&pdev->dev,
                                                 of_phy_simple_xlate);

    return PTR_ERR_OR_ZERO(phy_provider);
}

> +	if (ret)
> +		return ret;
>  
>  	dphy->txesc_clk = devm_clk_get(&pdev->dev, "txesc");

[Severity: Medium]
This is a pre-existing issue, but does the error path in stf_dphy_init() leak
the txesc_clk clock prepare and enable counts? 

If reset_control_deassert() fails, it returns without unwinding the previous
clk_prepare_enable():

stf_dphy_init() {
    ...
    ret = reset_control_deassert(dphy->sys_rst);
    if (ret) {
        dev_err(dphy->dev, "Failed to deassert sys_rst\n");
        return ret;
    }
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does stf_dphy_exit() abandon the remaining
hardware teardown sequence if reset_control_assert() fails? 

The early return skips calling clk_disable_unprepare() and
stf_dphy_hw_reset():

stf_dphy_exit() {
    ...
    ret = reset_control_assert(dphy->sys_rst);
    if (ret) {
        dev_err(dphy->dev, "Failed to assert sys_rst\n");
        return ret;
    }

    clk_disable_unprepare(dphy->txesc_clk);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718090054.444513-1-pengcan@kylinos.cn?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 2/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe

[Can Peng]

stf_dphy_probe() enables runtime PM before registering the PHY provider.
If devm_of_phy_provider_register() fails, probe returns with runtime PM
still enabled.

The driver also has no remove callback, so runtime PM is left enabled
on driver unbind after a successful probe.

Use devm_pm_runtime_enable() so runtime PM is disabled automatically
on later probe failures and on driver unbind.

Fixes: f8aa660841bc ("phy: starfive: Add mipi dphy rx support")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 drivers/phy/starfive/phy-jh7110-dphy-rx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/starfive/phy-jh7110-dphy-rx.c b/drivers/phy/starfive/phy-jh7110-dphy-rx.c
index 0b039e1f71c5..d06f21ad6332 100644
--- a/drivers/phy/starfive/phy-jh7110-dphy-rx.c
+++ b/drivers/phy/starfive/phy-jh7110-dphy-rx.c
@@ -150,6 +150,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
 {
 	struct phy_provider *phy_provider;
 	struct stf_dphy *dphy;
+	int ret;
 
 	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);
 	if (!dphy)
@@ -190,7 +191,9 @@ static int stf_dphy_probe(struct platform_device *pdev)
 		return PTR_ERR(dphy->phy);
 	}
 
-	pm_runtime_enable(&pdev->dev);
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
 
 	phy_set_drvdata(dphy->phy, dphy);
 	phy_provider = devm_of_phy_provider_register(&pdev->dev,
-- 
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The phy_configure callback accesses hardware registers while the device is still in reset and without ensuring the PM domain is active.
- [High] The driver acquires and configures clocks but never enables them.
--

--- Patch [2]: [PATCH v2 2/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe ---
commit fe8a50ded4b6e0393fd6d6a27d22735266427a91
Author: Can Peng <pengcan@kylinos.cn>

phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe

stf_dphy_probe() enables runtime PM before registering the PHY provider.
If devm_of_phy_provider_register() fails, probe returns with runtime PM
still enabled. This commit uses devm_pm_runtime_enable() to ensure
runtime PM is disabled automatically on later probe failures or unbind.

> diff --git a/drivers/phy/starfive/phy-jh7110-dphy-rx.c b/drivers/phy/starfive/phy-jh7110-dphy-rx.c
> index 0b039e1f71c55..d06f21ad63325 100644
> --- a/drivers/phy/starfive/phy-jh7110-dphy-rx.c
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-rx.c
> @@ -150,6 +150,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
>  	struct stf_dphy *dphy;
> +	int ret;
>  
>  	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);
>  	if (!dphy)
[ ... ]
> @@ -190,7 +191,9 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  		return PTR_ERR(dphy->phy);
>  	}
>  
> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
>  
>  	phy_set_drvdata(dphy->phy, dphy);
>  	phy_provider = devm_of_phy_provider_register(&pdev->dev,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the PM
changes, I noticed an issue in stf_dphy_configure().

The phy_configure callback accesses hardware registers directly:

stf_dphy_configure() {
    ...
    writel(FIELD_PREP(STF_DPHY_ENABLE_CLK, 1) |
           FIELD_PREP(STF_DPHY_ENABLE_CLK1, 1) |
    ...
}

However, the PM domain is resumed and the reset is deasserted later in
stf_dphy_power_on(). 

Because the RX driver lacks a phy_init() callback, if a consumer (like
a MIPI CSI-2/DSI controller) calls phy_configure() prior to phy_power_on(),
will this cause a synchronous external abort or silent write failure by
accessing registers of a device that is still held in reset and unpowered?

[Severity: High]
This is also a pre-existing issue, but I noticed a potential problem in
stf_dphy_power_on() regarding clock enablement.

The driver acquires cfg_clk, ref_clk, and tx_clk in probe() and sets their
rates in stf_dphy_power_on():

stf_dphy_power_on() {
    ...
    clk_set_rate(dphy->cfg_clk, 99000000);
    clk_set_rate(dphy->ref_clk, 49500000);
    clk_set_rate(dphy->tx_clk, 19800000);
    ...
}

Are these clocks ever actually enabled? I do not see any corresponding
clk_prepare_enable() calls in the driver. Will the PHY hardware fail to
operate or hang on register access if the clocks are not implicitly kept on
by the bootloader or a PM domain?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718090054.444513-1-pengcan@kylinos.cn?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy