Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues: - [High] Inverted boolean condition in `dw_dp_unbind()` causes runtime PM usage count leak and unbalanced puts. - [High] Missing API return value check for `pm_runtime_get_sync()` causes unsafe hardware access on failure. - [High] Sleeping function `pm_runtime_get_sync()` called from atomic context. - [High] Missing runtime PM reference release on error path in `dw_dp_bind()`. - [High] Asynchronous `pm_runtime_put_autosuspend()` is incorrectly used in the teardown path. - [High] Clock disable imbalance between `devm_clk_get_enabled()` and explicit `clk_disable_unprepare()` in runtime suspend. - [Medium] Unconditional sleep in `dw_dp_bridge_detect()` causes severe performance degradation during display polling. Pre-existing issues: - [Critical] Use-After-Free due to missing cancellation of `hpd_work` in teardown. - [High] Preexisting atomic context violation in `dw_dp_bridge_atomic_disable()`. -- --- Patch [7]: [PATCH v4 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support --- commit e300d93b1e1336324cbbd71c5f0b1b81334c7912 Author: Sebastian Reichel <[email protected]> Date: Tue Jul 21 17:52:37 2026 +0000 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. Signed-off-by: Sebastian Reichel <[email protected]> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > index 8eb2699dc7ab0..636b9e1052978 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c [ ... ] > @@ -1467,6 +1468,8 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux > *aux, > if (WARN_ON(msg->size > 16)) > return -E2BIG; > > + ACQUIRE(pm_runtime_active_auto, pm)(dp->dev); > + [Severity: High] Does the ACQUIRE macro wrapper silently ignore the return value of the underlying get_sync operation? If the device fails to power on or clock enablement fails, the driver might proceed to access hardware registers anyway. Could we handle the error here and abort the transfer instead of proceeding? > switch (msg->request & ~DP_AUX_I2C_MOT) { > case DP_AUX_NATIVE_WRITE: > case DP_AUX_I2C_WRITE: [ ... ] > @@ -1657,6 +1660,8 @@ static void dw_dp_bridge_atomic_enable(struct > drm_bridge *bridge, > struct drm_connector_state *conn_state; > int ret; > > + pm_runtime_get_sync(dp->dev); > + [Severity: High] Are we allowed to call a sleeping function from atomic context here? The dw_dp_bridge_atomic_enable() callback executes in atomic context where sleeping is forbidden. However, pm_runtime_get_sync() triggers dw_dp_runtime_resume(), which calls clk_prepare_enable() and sleeps. Also, similar to the ACQUIRE wrapper above, the return value of pm_runtime_get_sync() is ignored here. If it fails, will the driver proceed to access hardware registers while the device is unclocked? > connector = drm_atomic_get_new_connector_for_encoder(state, > bridge->encoder); > if (!connector) { > dev_err(dp->dev, "failed to get connector\n"); [ ... ] > @@ -1711,6 +1716,7 @@ static void dw_dp_bridge_atomic_disable(struct > drm_bridge *bridge, > dw_dp_link_disable(dp); > bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE); > dw_dp_reset(dp); > + pm_runtime_put_autosuspend(dp->dev); > } [Severity: High] This isn't a bug introduced by this patch, but does dw_dp_reset() sleep inside an atomic context? The dw_dp_bridge_atomic_disable() callback runs in atomic context, but dw_dp_reset() unconditionally calls usleep_range() and regmap_read_poll_timeout(). Should this hardware reset sequence be moved out of the atomic path or use non-sleeping delays? > static bool dw_dp_hpd_detect_link(struct dw_dp *dp, struct drm_connector > *connector) > @@ -1731,6 +1737,14 @@ static enum drm_connector_status > dw_dp_bridge_detect(struct drm_bridge *bridge, > { > struct dw_dp *dp = bridge_to_dp(bridge); > > + ACQUIRE(pm_runtime_active_auto, pm)(dp->dev); > + > + /* > + * HPD_HOT_PLUG bit is asserted only after the sink holds HPD high for > + * at least 100ms. Thus we need to wait 100ms after runtime PM resume. > + */ > + msleep(110); > + [Severity: Medium] Will this sleep degrade display polling performance? By using the ACQUIRE wrapper, the return value of pm_runtime_get_sync() is discarded, making it impossible to differentiate a fresh resume (returning 0) from an already-active device (returning 1). As a result, this 110ms sleep executes unconditionally on every detect call, even when the device is already active (such as in native DP mode where it is kept active permanently). > if (!dw_dp_hpd_detect(dp)) > return connector_status_disconnected; > [ ... ] > @@ -2032,10 +2046,15 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder > *encoder) > } > > if (dw_dp_is_routed_to_usb_c(encoder)) { > + dp->usbc_mode = true; > dev_dbg(dev, "USB-C mode\n"); > > if (dp->plat_data.hpd_sw_sel) > dp->plat_data.hpd_sw_sel(dp->plat_data.data, 1); > + } else { > + dp->usbc_mode = false; > + /* Keep runtime PM enabled to have working native HPD IRQ */ > + pm_runtime_get_sync(dp->dev); > } > > dw_dp_init_hw(dp); [Severity: High] Does the error path for hardware initialization following this block leak the runtime PM reference? If dp->usbc_mode is false, pm_runtime_get_sync() increments the usage count. If phy_init() fails shortly after dw_dp_init_hw(), the function jumps to the err_put_next_bridge label and returns without balancing this get_sync operation, causing the PM reference to permanently leak. > @@ -2065,6 +2084,8 @@ EXPORT_SYMBOL_GPL(dw_dp_bind); > > void dw_dp_unbind(struct dw_dp *dp) > { > + if (dp->usbc_mode) > + pm_runtime_put_autosuspend(dp->dev); [Severity: High] Is this boolean condition inverted? In dw_dp_bind(), pm_runtime_get_sync() is called when usbc_mode is false. By checking for true here, we call put on a reference we never acquired (causing an underflow/warning for USB-C) and leak the reference when native DP mode is used. [Severity: High] Is it safe to use the asynchronous pm_runtime_put_autosuspend() variant in a teardown path? When device removal or pm_runtime_disable() follows immediately, the pending async idle/suspend work can be cancelled by the PM core. This could leave the hardware active indefinitely. Should a synchronous pm_runtime_put_sync() be used here instead? > disable_irq(dp->irq); [Severity: Critical] This is a pre-existing issue, but is there a use-after-free risk because hpd_work is not canceled? The hpd_work struct is allocated via devm_kzalloc. In this teardown path, the IRQ is disabled, but cancel_work_sync(&dp->hpd_work) is never called. If the work was scheduled by an interrupt just before disable_irq(), the work queue thread could execute after the devm subsystem frees the memory. > phy_exit(dp->phy); > drm_bridge_put(dp->next_bridge); [ ... ] > @@ -2163,6 +2184,35 @@ struct dw_dp *dw_dp_probe(struct platform_device > *pdev, const struct dw_dp_plat_ > } > EXPORT_SYMBOL_GPL(dw_dp_probe); > > +int dw_dp_runtime_suspend(struct dw_dp *dp) > +{ > + clk_disable_unprepare(dp->aux_clk); > + clk_disable_unprepare(dp->apb_clk); > + > + return 0; > +} [Severity: High] Will manually disabling these clocks here cause an imbalance on device teardown? If the apb_clk and aux_clk clocks are acquired using devm_clk_get_enabled(), a cleanup action is registered to call clk_disable_unprepare() upon device unbind. If the device is runtime suspended when it is unbound, the devm teardown mechanism will invoke clk_disable_unprepare() again on these already-disabled clocks, dropping the enable counters below zero. > +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend); > + > +int dw_dp_runtime_resume(struct dw_dp *dp) > +{ > + int ret; [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede...@collabora.com?part=7
