Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Using `guard(pm_runtime_noresume)` in the interrupt handler allows hardware access on a suspended device. - [High] Unhandled failure of PM resume in `dw_dp_bridge_atomic_enable()` causes unbalanced PM operations and unclocked register accesses during teardown. Pre-existing issues: - [High] `drm_dp_aux_unregister()` is called too late in `dw_dp_unbind()`, allowing userspace to trigger AUX transfers on torn-down hardware. -- --- Patch [11]: [PATCH v6 11/16] drm/bridge: synopsys: dw-dp: Add Runtime PM support --- commit fb3ecb0112d9fff7c3a992e9273e0b7eaa8b8174 Author: Sebastian Reichel <[email protected]> drm/bridge: synopsys: dw-dp: Add Runtime PM support Add runtime PM stubs to the Synopsys DesignWare DisplayPort bridge driver. Support is not enabled automatically and must be hooked up in the vendor specific glue code. > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c [ ... ] > @@ -1659,6 +1665,12 @@ static void dw_dp_bridge_atomic_enable(struct > drm_bridge *bridge, > struct drm_connector_state *conn_state; > int ret; > > + ret = pm_runtime_get_active(dp->dev, RPM_TRANSPARENT); > + if (ret) { > + dev_err(dp->dev, "runtime PM failure\n"); > + return; > + } > + [Severity: High] If acquiring the PM reference fails here, we log an error and return early, but does the DRM core proceed as if the bridge is enabled? Later, when tearing down the display pipeline, dw_dp_bridge_atomic_disable() is called unconditionally. It executes dw_dp_reset() (which accesses hardware registers) and pm_runtime_put_autosuspend() (which drops a reference that was never acquired). Could this lead to unclocked register accesses causing a bus fault, and a PM usage counter underflow? > connector = drm_atomic_get_new_connector_for_encoder(state, > bridge->encoder); > if (!connector) { > dev_err(dp->dev, "failed to get connector\n"); [ ... ] > @@ -1966,6 +1988,9 @@ static irqreturn_t dw_dp_irq(int irq, void *data) > struct dw_dp *dp = data; > u32 value; > > + /* interrupt can only trigger for running device */ > + guard(pm_runtime_noresume)(dp->dev); > + > regmap_read(dp->regmap, DW_DP_GENERAL_INTERRUPT, &value); [Severity: High] Does using guard(pm_runtime_noresume) allow hardware access on a suspended device? Since dw_dp_runtime_suspend() disables clocks but does not disable the IRQ, a shared interrupt or a glitch on the IRQ line while the device is in runtime suspend could cause this handler to execute and read DW_DP_GENERAL_INTERRUPT while clocks are off. Should this use pm_runtime_get_if_active() instead to avoid a bus fault? > if (!value) > return IRQ_NONE; [ ... ] > @@ -2101,6 +2152,11 @@ void dw_dp_unbind(struct dw_dp *dp) > disable_irq(dp->irq); > cancel_work_sync(&dp->hpd_work); > phy_exit(dp->phy); > + if (!dp->usbc_mode) > + pm_runtime_put_sync(dp->dev); > + /* suspend once if runtime PM is disabled */ > + if (!pm_runtime_enabled(dp->dev)) > + dw_dp_runtime_suspend(dp); > drm_bridge_put(dp->next_bridge); > drm_dp_aux_unregister(&dp->aux); [Severity: High] This is a pre-existing issue, but is drm_dp_aux_unregister() called too late here? During device unbind, phy_exit() and disable_irq() are called before drm_dp_aux_unregister(). Because the AUX device is still registered and accessible to userspace, a userspace process could initiate an AUX transfer that calls dw_dp_aux_transfer() while the PHY is uninitialized and the IRQ is disabled, potentially causing a hardware fault or timeout. > drm_bridge_remove(&dp->bridge); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v6-0-041d99a19...@collabora.com?part=11
