Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] Unchecked return value of clk_prepare_enable() in runtime resume. - [High] Unchecked return value of pm_runtime_get_sync() during atomic enable. - [High] Interrupt handler accesses hardware registers while device may be suspended. Pre-existing issues: - [High] Memory/refcount leak of the next_bridge acquired via of_drm_get_bridge_by_endpoint(). - [High] PHY power_count underflow causing permanent PHY failure after early return in atomic_enable. - [High] Uninitialized stack memory copied to buffer in dw_dp_aux_read_data(). -- commit 25b9778935f0029deadc6a4ac4d19728b07017ee Author: Sebastian Reichel <[email protected]> Subject: drm/bridge: synopsys: dw-dp: Add Runtime PM support The patch adds runtime PM stubs to the Synopsys DesignWare DisplayPort bridge driver and hooks them up to various hardware access paths. > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > index 7fa38145e35ce..7f4f36c614844 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > @@ -1465,6 +1465,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] This is a pre-existing issue, but I noticed a potential information leak in dw_dp_aux_read_data() which is called during these transfers: drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_read_data() { ... u32 value; regmap_read(dp->regmap, DW_DP_AUX_DATA0 + i * 4, &value); for (j = 0; j < num; j++) buffer[i * 4 + j] = value >> (j * 8); ... } If regmap_read() fails, the local variable "value" remains uninitialized. Could this copy uninitialized kernel stack memory into the buffer, which is then returned to userspace via the DRM DP AUX chardev? > switch (msg->request & ~DP_AUX_I2C_MOT) { > case DP_AUX_NATIVE_WRITE: > case DP_AUX_I2C_WRITE: [ ... ] > @@ -1655,6 +1657,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] Does this code need to check the return value of pm_runtime_get_sync()? If the device fails to resume (for example, if the clocks fail to enable), the driver will continue with hardware initialization routines like dw_dp_link_enable(). Can this lead to a crash by accessing a suspended device? > connector = drm_atomic_get_new_connector_for_encoder(state, > bridge->encoder); > if (!connector) { > dev_err(dp->dev, "failed to get connector\n"); > return; > } [Severity: High] This is a pre-existing issue, but does returning early here cause a phy->power_count underflow later? If we return early because connector is NULL, or if dw_dp_link_enable() fails, we skip powering on the PHY. However, because atomic_enable returns void, the DRM framework will still consider the bridge enabled and later call dw_dp_bridge_atomic_disable(), which unconditionally executes: phy_power_off(dp->phy); Could this drop the PHY's power_count below zero and permanently prevent it from powering on during subsequent modesets? [ ... ] > @@ -2153,6 +2160,26 @@ void dw_dp_unbind(struct dw_dp *dp) > } > EXPORT_SYMBOL_GPL(dw_dp_unbind); > > +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] Is it safe to leave the IRQ enabled while the device is suspended? If a spurious interrupt or a hot-plug event fires while the device is in the RPM_SUSPENDED state, dw_dp_irq() will execute and unconditionally call: regmap_read(dp->regmap, DW_DP_GENERAL_INTERRUPT, &value); Since the APB clock is disabled here, might this MMIO read hang the bus and cause a kernel panic? > +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend); > + > +int dw_dp_runtime_resume(struct dw_dp *dp) > +{ > + clk_prepare_enable(dp->apb_clk); > + clk_prepare_enable(dp->aux_clk); > + [Severity: High] Should the return values of clk_prepare_enable() be checked? If the clocks fail to enable, the function proceeds to call dw_dp_init_hw() which accesses hardware registers via regmap. Could this lead to a bus fault? > + dw_dp_init_hw(dp); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dw_dp_runtime_resume); There is also one additional pre-existing issue in a part of the file not modified by this patch: [Severity: High] This is a pre-existing issue, but I noticed a potential leak in dw_dp_bind(): drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind() { ... next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0); if (IS_ERR(next_bridge)) { ... ret = drm_bridge_attach(encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR); ... } Does this code leak the reference count for next_bridge? of_drm_get_bridge_by_endpoint() returns a bridge with an incremented refcount, and drm_bridge_attach() takes its own reference. Should there be a call to drm_bridge_put() to drop the initial reference so the next bridge module can be unloaded? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260612-synopsys-dw-dp-improvements-v3-0-dc61e6352...@collabora.com?part=7
