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 <[email protected]> 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/[email protected]?part=3
