Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Devm action unmasks the IRQ during driver unload due to skipped runtime suspend, creating a Use-After-Free window. - [Medium] Asynchronous pm_runtime_put_autosuspend() in teardown paths races with pm_runtime_disable(), leaving the device powered on. - [Medium] The return value of dw_dp_runtime_resume() is ignored in dw_dp_probe() when runtime PM is disabled. --
--- Patch [14]: [PATCH v11 14/21] drm/bridge: synopsys: dw-dp: Add Runtime PM support --- commit af390b7b3091a44b56036dc11fc278ff1a6d55a3 Author: Sebastian Reichel <[email protected]> Subject: drm/bridge: synopsys: dw-dp: Add Runtime PM support This commit adds runtime power management stubs to the Synopsys DesignWare DisplayPort bridge driver. It splits the probe function to allow platform drivers to assign it early so that runtime events can be properly routed. > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > index 3af5b1f643c10..31f944e143ea5 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > @@ -1931,6 +1963,9 @@ static int dw_dp_bridge_attach(struct drm_bridge > *bridge, > > drm_dp_aux_unregister(&dp->aux); > > +err_runtime_pm_put: > + pm_runtime_put_autosuspend(dp->dev); > + > return ret; > } > > @@ -1941,6 +1976,9 @@ static void dw_dp_bridge_detach(struct drm_bridge > *bridge) > disable_irq(dp->irq); > cancel_work_sync(&dp->hpd_work); > drm_dp_aux_unregister(&dp->aux); > + > + if (!dp->usbc_mode) > + pm_runtime_put_autosuspend(dp->dev); > } [Severity: Medium] Will this asynchronous pm_runtime_put_autosuspend() race with pm_runtime_disable() during device removal or probe failure? In dw_dp_bridge_detach() and the error path of dw_dp_bridge_attach(), the driver drops its runtime reference using an asynchronous put. However, during component teardown or probe failure, this is quickly followed by the devm cleanup phase, which calls pm_runtime_disable() (registered via devm_pm_runtime_enable() in probe). According to power management guidelines, pm_runtime_disable() cancels any pending asynchronous suspend timers. Because the put was asynchronous and the timer is cancelled before it expires, the device remains in the RPM_ACTIVE state and is never physically suspended, causing a power leak. Could a synchronous put (like pm_runtime_put_sync()) be used here instead? [ ... ] > @@ -2205,41 +2280,67 @@ struct dw_dp *dw_dp_probe(struct platform_device > *pdev, const struct dw_dp_plat_ > + /* > + * Disable IRQ a second time; this ensures the interrupt is only > + * enabled when the bridge is attached AND runtime PM is enabled. > + * Also register a devm action to restore the correct balance during > + * device removal. > + */ > + disable_irq(dp->irq); > + > + ret = devm_add_action_or_reset(dev, dw_dp_enable_irq, dp); > + if (ret) > + return ret; [Severity: High] Does this devm action risk a use-after-free by unmasking the interrupt during driver unload? The driver uses the interrupt disable depth as a refcount: IRQF_NO_AUTOEN (depth=1) plus the explicit disable_irq() in probe (depth=2). It expects two enable_irq() calls (attach and runtime_resume) to reach depth 0. If the device fails to runtime suspend during teardown (e.g., due to the pm_runtime_put_autosuspend() cancellation race described above), dw_dp_runtime_suspend() is skipped, and its corresponding disable_irq() is missed. When dw_dp_bridge_detach() calls disable_irq(), the depth only returns to 1. Later, the devm cleanup action dw_dp_enable_irq() decrements the depth. Since the depth was 1, it reaches 0 and physically unmasks the interrupt. If an interrupt fires in the small window between dw_dp_enable_irq() and free_irq(), dw_dp_irq() queues dp->hpd_work. Since cancel_work_sync() was already called in dw_dp_bridge_detach(), this requeues the work on a dp structure that is about to be freed by devres, resulting in a use-after-free when the workqueue runs. > + if (dp->plat_data.autosuspend_delay > 0) { > + pm_runtime_use_autosuspend(dev); > + pm_runtime_set_autosuspend_delay(dev, > dp->plat_data.autosuspend_delay); > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; > + } > + > + if (!pm_runtime_enabled(dev)) { > + dw_dp_runtime_resume(dp); > + > + ret = devm_add_action_or_reset(dev, dw_dp_manual_suspend, dp); > + if (ret) > + return ret; > + } [Severity: Medium] Should the return value of dw_dp_runtime_resume() be checked here? When pm_runtime_enabled(dev) is false, dw_dp_runtime_resume() is called manually to power up the hardware, but its return value is ignored. If it fails (for example, if clk_prepare_enable() fails), it aborts early and leaves resources like aux_clk and the interrupt disabled. However, the probe continues and registers the dw_dp_manual_suspend devres action. During driver teardown, dw_dp_manual_suspend() unconditionally calls clk_disable_unprepare(dp->aux_clk) and disable_irq(dp->irq). Calling these on resources that were never enabled triggers subsystem warnings. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v11-0-0d508505f...@collabora.com?part=14
