Re: [PATCH v4 08/10] drm/rockchip: dw_dp: Add runtime PM support

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Threaded IRQ handler accesses unclocked hardware registers if triggered while runtime suspended.
- [High] Double disable of devm-managed clocks due to uncoordinated runtime PM suspend.
- [Medium] Incorrect runtime PM teardown sequence prevents asynchronous suspends.
- [Medium] Missing idle trigger leaves the device fully powered on after initialization.
- [Medium] Calling dw_dp_bind() before pm_runtime_enable() causes -EACCES errors from the PM core.

Pre-existing issues:
- [High] Inverted logic for runtime PM reference management causes usage count leaks and underflows.
- [High] Missing runtime PM reference drop on the bind error path.
--

--- Patch [8]: [PATCH v4 08/10] drm/rockchip: dw_dp: Add runtime PM support ---
commit 73aba26d330e327d28e953d044baf166117572f7
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/rockchip: dw_dp: Add runtime PM support

This patch adds support for runtime PM to the Rockchip RK3576/3588 Synopsys
DesignWare DisplayPort driver.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 0ecbad24dd4e8..d7e1704fd9806 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -169,14 +174,25 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

[Severity: Medium]
Because dw_dp_bind() is called immediately before this block, does it
cause -EACCES errors from the PM core?

Inside dw_dp_bind(), the core driver calls pm_runtime_get_sync(dp->dev).
Since runtime PM is not yet enabled for the device at that point, the PM
core will return -EACCES.

[Severity: High]
This is a pre-existing issue, but does dw_dp_bind() leak a runtime PM
reference on its error path?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind():

    if (!dp->usbc_mode)
        pm_runtime_get_sync(dp->dev);

    ...

    ret = phy_init(dp->phy);
    if (ret) {
        dev_err_probe(dev, ret, "phy init failed\n");
        goto put_next_bridge;
    }

If phy_init() fails, the function jumps to put_next_bridge and returns
without dropping the reference acquired earlier, permanently preventing
the device from suspending.

[Severity: Medium]
Does this initialization sequence miss an idle trigger?

The driver calls pm_runtime_set_active() and pm_runtime_enable(), but does
not queue an idle check. The runtime PM core does not automatically queue
one on enable, which might leave the device fully powered on until the
first display modeset cycle. Could a manual call to pm_request_idle() be
needed to start the autosuspend timer?

>  	connector = drm_bridge_connector_init(drm_dev, encoder);
>  	if (IS_ERR(connector)) {
> -		dw_dp_unbind(dp->base);
> -		return dev_err_probe(dev, PTR_ERR(connector),
> -				     "Failed to init bridge connector\n");
> +		ret = PTR_ERR(connector);
> +		dev_err_probe(dev, ret, "Failed to init bridge connector\n");
> +		goto error;
>  	}
>  
>  	return 0;
> +
> +error:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +	dw_dp_unbind(dp->base);
> +	return ret;
>  }
>  
>  static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
> @@ -184,6 +200,8 @@ static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
>  {
>  	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
>  
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
>  	dw_dp_unbind(dp->base);
>  }

[Severity: Medium]
Does disabling runtime PM before calling dw_dp_unbind() prevent
asynchronous suspends?

By calling pm_runtime_disable() first, when dw_dp_unbind() subsequently
calls pm_runtime_put_autosuspend(), the asynchronous idle request is
rejected because runtime PM is already disabled. This leaves the device
permanently in the RPM_ACTIVE state and the hardware remains powered on
after the driver is unbound.

[Severity: High]
Could this teardown sequence result in a double disable of devm-managed
clocks?

The driver acquires clocks using devm_clk_get_enabled() during probe,
which registers a devres action to automatically disable them on removal.

However, dw_dp_runtime_suspend() manually calls clk_disable_unprepare().
If the device is already in the RPM_SUSPENDED state when unbound,
pm_runtime_disable() leaves it suspended. When devres subsequently
attempts to disable the already-disabled clocks, it will cause an
underflow.

[Severity: High]
This is a pre-existing issue, but is the logic for dropping the runtime
PM reference inverted in dw_dp_unbind()?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind():

    if (!dp->usbc_mode) {
        pm_runtime_get_sync(dp->dev);
    }

But in dw_dp_unbind():

    if (dp->usbc_mode)
        pm_runtime_put_autosuspend(dp->dev);

This means standard DP (usbc_mode = false) leaks the reference it
acquired, while USB-C DP (usbc_mode = true) attempts to drop a reference
it never acquired, causing usage count leaks and underflows.

[ ... ]
> @@ -254,6 +272,34 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
>  	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
>  }
>  
> +static int dw_dp_rockchip_runtime_suspend(struct device *dev)
> +{
> +	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
> +
> +	return dw_dp_runtime_suspend(dp->base);
> +}

[Severity: Critical]
Does leaving the IRQ enabled during runtime suspend risk a kernel panic?

This patch introduces runtime PM support, which can now trigger an issue
in the bridge driver. dw_dp_runtime_suspend() disables the apb_clk.
However, the driver leaves dp->irq enabled.

If a spurious interrupt or HPD event fires while the device is in the
RPM_SUSPENDED state, dw_dp_irq() will directly read
DW_DP_GENERAL_INTERRUPT via regmap:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_irq() {
    ...
    regmap_read(dp->regmap, DW_DP_GENERAL_INTERRUPT, &value);
}

Accessing these unclocked hardware registers without first checking the
runtime PM status (e.g., using pm_runtime_get_if_active()) will cause a
synchronous external abort or data abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=8