Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing reinit_completion() before AUX transfer causes subsequent transfers to falsely succeed if a timeout previously occurred or stray interrupts arrived. - [High] Short HPD and Long HPD events share a single boolean state variable, causing critical hot plug events to be lost if interrupts arrive in quick succession. - [High] Hardware is powered down and interrupts are disabled before the DP AUX interface is unregistered, allowing userspace to crash the system or hang by accessing powered-down hardware. -- --- Patch [3]: [PATCH v6 03/16] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal --- commit 4a4bd90d7d64ae717e418a4501d8f02733f02d38 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. Fixes: 86eecc3a9c2e ("drm/bridge: synopsys: Add DW DPTX Controller support library") Reported-by: Sashiko <[email protected]> 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 21bd08571dcfe..b28040d69fabf 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c [ ... ] This is a pre-existing issue, but I noticed a potential problem in dw_dp_aux_transfer(): value |= FIELD_PREP(AUX_ADDR, msg->address); regmap_write(dp->regmap, DW_DP_AUX_CMD, value); status = wait_for_completion_timeout(&dp->complete, timeout); if (!status) { [Severity: High] Does this completion need to be reinitialized before the transfer? If a previous transfer timed out and the hardware later replies, could a subsequent transfer falsely succeed immediately because the completion counter was left > 0? [ ... ] This isn't a bug introduced by this patch, but there is a potential event loss in the HPD handler. If a physical hotplug triggers an interrupt: dw_dp_handle_hpd_event() if (value & HPD_HOT_PLUG) { dp->hotplug.long_hpd = true; } Could a subsequent short HPD interrupt (e.g., from a link status change) overwrite this state before dw_dp_hpd_work() evaluates it? dw_dp_handle_hpd_event() if (value & HPD_IRQ) { dp->hotplug.long_hpd = false; [Severity: High] If dw_dp_hpd_work() runs after this short pulse, will it read long_hpd as false and perform a link retrain instead of notifying userspace of the hotplug? [ ... ] This is also a pre-existing issue, but I noticed a potential hardware hang in the unbind sequence just above the probe function: void dw_dp_unbind(struct dw_dp *dp) { disable_irq(dp->irq); cancel_work_sync(&dp->hpd_work); phy_exit(dp->phy); drm_dp_aux_unregister(&dp->aux); [Severity: High] Does this order allow userspace to access unpowered hardware? Since phy_exit() powers down the PHY before the AUX interface is unregistered, could a concurrent read/write to the /dev/drm_dp_auxN character device cause a synchronous external abort or hardware hang? > @@ -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); > init_completion(&dp->complete); > > + 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-v6-0-041d99a19...@collabora.com?part=3
