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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The completion `dp->complete` is not re-initialized before starting a new AUX transfer, causing subsequent transfers to falsely complete immediately if a previous transfer timed out.
- [High] The AUX read length calculation incorrectly subtracts 1 instead of adding 1, causing all DP AUX reads to fail with `-EBUSY`.
--

--- Patch [2]: [PATCH v5 02/15] drm/bridge: synopsys: dw-dp: Cancel pending HPD work on unbind ---
commit 7fd4acc667a3adf3c285f78c69b39dea1318c0a1
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Subject: drm/bridge: synopsys: dw-dp: Cancel pending HPD work on unbind

This commit fixes a race condition during device unbind where the HPD worker
thread might access invalid resources if started exactly when unbind happens.
It avoids this by canceling any pending HPD work immediately after disabling
the interrupt.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 368e1f85e6253..21bd08571dcfe 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2009,6 +2009,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);
>  	drm_bridge_remove(&dp->bridge);

[Severity: High]
This isn't a bug introduced by this patch, but looking at dw_dp_aux_transfer()
in drivers/gpu/drm/bridge/synopsys/dw-dp.c, it seems dp->complete is not
re-initialized before waiting for completion:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer() {
    ...
    if (msg->size > 0)
        value = FIELD_PREP(AUX_LEN_REQ, msg->size - 1);
    ...
    regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

    status = wait_for_completion_timeout(&dp->complete, timeout);
    ...
}

If a previous transfer timed out, the completion count would remain 0.
If the hardware eventually finishes and fires a late interrupt, the interrupt
handler could call complete(&dp->complete).

Could a subsequent call to dw_dp_aux_transfer() then falsely complete
immediately if wait_for_completion_timeout() consumes this stale completion?

Would adding a call to reinit_completion(&dp->complete) before writing the
command resolve this?

[Severity: High]
This is a pre-existing issue, but further down in dw_dp_aux_transfer(), is there
a logic error in the AUX read length calculation?

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer() {
    ...
    if (msg->size > 0 && msg->reply == DP_AUX_NATIVE_REPLY_ACK) {
        if (msg->request & DP_AUX_I2C_READ) {
            size_t count = FIELD_GET(AUX_BYTES_READ, value) - 1;

            if (count != msg->size)
                return -EBUSY;
    ...
}

Because 1 is subtracted from FIELD_GET(AUX_BYTES_READ, value) instead of being
added, count can never equal msg->size.

For example, if the hardware reports msg->size or msg->size - 1, subtracting 1
guarantees count evaluates to either msg->size - 1 or msg->size - 2.

Does this cause all DP AUX reads to unconditionally fail with -EBUSY, breaking
EDID reading and link training?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v5-0-9445c2e87441@collabora.com?part=2