Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing `reinit_completion` in `dw_dp_aux_transfer` allows stale completion signals to falsely succeed future transfers. - [High] The AUX device is registered and exposed to userspace before the hardware is initialized and the IRQ is enabled. - [High] Concurrent HPD interrupts can overwrite `dp->hotplug.long_hpd`, causing dropped hotplug events. -- --- Patch [3]: [PATCH v5 03/15] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal --- commit bc7168d1bf0fe1e3fae642d5cb72e8aa27942948 Author: Sebastian Reichel <[email protected]> drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal The driver is currently missing to fully clean up after itself. Ensure that the mutex is cleaned up. > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > index 21bd08571dcfe..b28040d69fabf 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > @@ -2031,10 +2031,13 @@ struct dw_dp *dw_dp_probe(struct platform_device > *pdev, const struct dw_dp_plat_ > dp->pixel_mode = plat_data->pixel_mode; > dp->plat_data.max_link_rate = plat_data->max_link_rate; > > - mutex_init(&dp->irq_lock); > INIT_WORK(&dp->hpd_work, dw_dp_hpd_work); [Severity: High] This is a pre-existing issue, but could concurrent HPD interrupts overwrite dp->hotplug.long_hpd, causing dropped hotplug events? In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_handle_hpd_event(), the flag is updated based on interrupt status: if (value & HPD_IRQ) { ... dp->hotplug.long_hpd = false; ... } if (value & HPD_HOT_PLUG) { ... dp->hotplug.long_hpd = true; ... } Since schedule_work() ensures only a single instance of the work runs, multiple interrupts firing in rapid succession (e.g., a hot plug followed immediately by an IRQ/short HPD from the sink requesting link training) could overwrite the long_hpd flag before the worker can process the first event. The worker would only see the final state (e.g., false for short HPD), permanently dropping the hotplug notification. > init_completion(&dp->complete); [Severity: High] This is a pre-existing issue, but is there a missing reinit_completion() in dw_dp_aux_transfer() that could allow stale completion signals to falsely succeed future transfers? In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer(), the driver waits for an AUX reply: regmap_write(dp->regmap, DW_DP_AUX_CMD, value); status = wait_for_completion_timeout(&dp->complete, timeout); if (!status) { dev_err(dp->dev, "timeout waiting for AUX reply\n"); return -ETIMEDOUT; } If an AUX transfer times out, the completion's internal done counter is not consumed. If the hardware later fires a delayed AUX_REPLY_EVENT interrupt, the IRQ handler will call complete(&dp->complete), incrementing the counter. Subsequent calls to dw_dp_aux_transfer() would then immediately return from wait_for_completion_timeout() without waiting for the actual hardware reply. [Severity: High] This is another pre-existing issue, but is the AUX device registered and exposed to userspace before the hardware is initialized and the IRQ is enabled? In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind(): ret = drm_dp_aux_register(&dp->aux); if (ret) { ... } ... dw_dp_init_hw(dp); ret = phy_init(dp->phy); ... enable_irq(dp->irq); If a userspace process opens the /dev/drm_dp_auxN device and issues a read during the window after drm_dp_aux_register() but before enable_irq(), the transfer will time out. This initial timeout would permanently corrupt the completion state as described above, causing future legitimate EDID reads and link training to fail. > > + ret = devm_mutex_init(dev, &dp->irq_lock); > + if (ret) > + return ERR_PTR(ret); > + > res = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(res)) > return ERR_CAST(res); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v5-0-9445c2e87...@collabora.com?part=3
