Re: [PATCH 3/3] drm/msm/dsi: Drop dev_pm_opp_set_rate(0)

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Asynchronous pm_runtime_put() in power-off path races with device teardown, leaking bus clocks.
- [High] Ignored return values for clock enablement in msm_dsi_host_xfer_prepare() leading to unclocked register access.
- [High] Unclocked register access crash due to missing cancel_work_sync() for err_work.
- [High] Double put_device() on the PHY device when dsi_get_phy() defers probe.
- [High] Asynchronous pm_runtime_put() causes hardware sequencing violation by disabling regulators while bus clocks are still running.
--

--- Patch [3]: [PATCH 3/3] drm/msm/dsi: Drop dev_pm_opp_set_rate(0) ---
commit 9d791038937d59c654ef91edf07d2b7ff4432725
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

drm/msm/dsi: Drop dev_pm_opp_set_rate(0)

This commit removes the dev_pm_opp_set_rate(0) call from
dsi_link_clk_disable_6g() because it removes the power vote without
parking the clock. It relies on the subsequent pm_runtime_put() to rescind
the power vote.

>     Every call site of ops->link_clk_disable() is followed by
>     pm_runtime_put(), so the power vote will be rescinded if deemed safe.

[Severity: High]
This isn't a bug introduced by this patch, but does relying on the
asynchronous pm_runtime_put() cause a resource leak during teardown?

In msm_dsi_host_power_off(), pm_runtime_put() is called to queue an async
suspend. However, if the device is immediately torn down,
msm_dsi_host_destroy() cancels any pending async work:

drivers/gpu/drm/msm/dsi/dsi_host.c:msm_dsi_host_destroy() {
    ...
    pm_runtime_disable(&msm_host->pdev->dev);
    ...
}

Because the async suspend is canceled, does this leave the device in the
RPM_ACTIVE state and permanently leak the bus clocks? Should
pm_runtime_put_sync() be used instead?


[Severity: High]
This is a pre-existing issue, but does this asynchronous pm_runtime_put() also
cause a hardware power-sequencing violation?

In msm_dsi_host_power_off(), the regulators are disabled immediately after
the async put:

drivers/gpu/drm/msm/dsi/dsi_host.c:msm_dsi_host_power_off() {
    ...
    pm_runtime_put(&msm_host->pdev->dev);

    regulator_bulk_disable(msm_host->cfg_hnd->cfg->num_regulators,
                           msm_host->supplies);
    ...
}

Since pm_runtime_put() returns immediately while bus clocks are still running,
does this remove power before the clocks are actually disabled by the
asynchronous suspend?


[Severity: High]
This is a pre-existing issue, but is there a risk of an unclocked register
access crash during power off?

When the display is turned off in msm_dsi_host_power_off(), hardware clocks
are disabled but there is no call to cancel_work_sync() for err_work:

drivers/gpu/drm/msm/dsi/dsi_host.c:msm_dsi_host_power_off() {
    ...
    cfg_hnd->ops->link_clk_disable(msm_host);
    pm_runtime_put(&msm_host->pdev->dev);
    ...
}

If err_work executes after clocks are disabled or is flushed during driver
teardown:

drivers/gpu/drm/msm/dsi/dsi_host.c:dsi_err_worker() {
    ...
        dsi_sw_reset(msm_host);
    ...
}

Will this unconditional hardware access cause a synchronous external abort?


> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 59125d342fcb6..7e4e3718b536c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -550,8 +550,6 @@ int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host)
>  
>  void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host)
>  {
> -	/* Drop the performance state vote */
> -	dev_pm_opp_set_rate(&msm_host->pdev->dev, 0);

[Severity: High]
This is a pre-existing issue, but is it possible for a similar sequence to
cause a kernel panic due to ignored return values when enabling clocks?

In msm_dsi_host_xfer_prepare(), the return values for enablement are ignored:

drivers/gpu/drm/msm/dsi/dsi_host.c:msm_dsi_host_xfer_prepare() {
    ...
    pm_runtime_get_sync(&msm_host->pdev->dev);
    cfg_hnd->ops->link_clk_set_rate(msm_host);
    cfg_hnd->ops->link_clk_enable(msm_host);
    ...
    msm_host->dma_cmd_ctrl_restore = dsi_read(msm_host, REG_DSI_CTRL);
    ...
}

If clock enablement fails, will the unconditional dsi_read() to an unclocked
register cause a synchronous external abort?


[Severity: High]
This isn't a bug introduced by this patch, but does dsi_get_phy() double free
the PHY device on probe deferral?

If the PHY driver is not ready, dsi_get_phy() drops the reference but leaves a
dangling pointer in msm_dsi->phy_dev:

drivers/gpu/drm/msm/dsi/dsi.c:dsi_get_phy() {
    ...
        put_device(&phy_pdev->dev);
        DRM_DEV_ERROR(&pdev->dev, "%s: phy driver is not ready\n", __func__);
        return -EPROBE_DEFER;
    ...
}

When the error is handled during dsi_init(), it calls dsi_destroy() which
drops the reference a second time:

drivers/gpu/drm/msm/dsi/dsi.c:dsi_destroy() {
    ...
    if (msm_dsi->phy_dev) {
        put_device(msm_dsi->phy_dev);
    ...
}

Could this lead to a reference count underflow?

>  	clk_disable_unprepare(msm_host->esc_clk);
>  	clk_disable_unprepare(msm_host->pixel_clk);
>  	clk_disable_unprepare(msm_host->byte_intf_clk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-topic-dpu_power-v1-0-e7783b859a70@oss.qualcomm.com?part=3