Re: [PATCH v7 02/18] drm/bridge: synopsys: dw-dp: Cancel pending HPD work on unbind

[sashiko-bot@kernel.org]

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 causes immediate return on subsequent transfers if a previous one timed out.
- [High] The driver requests a hardware reset control but never deasserts it, potentially leaving the hardware in a reset state.
- [Medium] The DP AUX I2C adapter is exposed to userspace before the hardware is initialized, and unregistered after the hardware is powered down.
--

--- Patch [2]: [PATCH v7 02/18] drm/bridge: synopsys: dw-dp: Cancel pending HPD work on unbind ---
commit 9df51b0b37728e172be280b5d55126b4cd203eb4
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Cancel pending HPD work on unbind
    
This commit fixes a race condition during device unbind by canceling any
pending HPD work after disabling the interrupt, preventing the worker thread
from accessing invalid resources.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 67d9cd0bf5fd4..8393f9c9b0f00 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1996,6 +1996,7 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
>  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: Medium]
This is a pre-existing issue, but the AUX adapter is unregistered after the
PHY is shut down via phy_exit().

Can this allow in-flight transactions to access unpowered hardware? A
userspace process could initiate an I2C transaction during device unbind after
phy_exit() is called but before drm_dp_aux_unregister(), causing I2C
transfers to timeout or fail.

Similarly, is the AUX adapter registered before hardware initialization in
dw_dp_bind()? It appears drm_dp_aux_register() is called before
dw_dp_init_hw() and phy_init() complete, exposing uninitialized hardware to
potential userspace access.

>  }


While reviewing this file, I also noticed a couple of other pre-existing
issues outside of this patch's scope:

[Severity: High]
This isn't a bug introduced by this patch, but does dw_dp_aux_transfer() fail
to reinitialize the completion before waiting?

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer() {
    ...
    status = wait_for_completion_timeout(&dp->complete, timeout);
    ...
}

If an AUX reply interrupt fires after wait_for_completion_timeout() has timed
out, the completion structure's done count is incremented.

On the next transfer, will wait_for_completion_timeout() return immediately
without waiting for the new hardware reply? Consecutive I2C/AUX transfers
could fail prematurely, potentially breaking display detection.


[Severity: High]
This is also a pre-existing issue, but the driver requests a hardware reset
control without ever deasserting it:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_probe() {
    ...
    dp->rstc = devm_reset_control_get(dev, NULL);
    ...
}

Can this leave the DisplayPort hardware in a reset state? There are no
corresponding calls to reset_control_deassert() anywhere in the driver.

If the bootloader has asserted the reset line, the driver might fail to
communicate with the hardware and potentially cause bus hangs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-synopsys-dw-dp-improvements-v7-0-b7640fa8cf48@collabora.com?part=2