Re: [PATCH v7 05/18] drm/bridge: synopsys: dw-dp: Reset AUX channel on transfer timeout

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The soft reset mechanism intended to prevent delayed completions from leaking into subsequent transfers is incomplete due to a software race condition with the threaded IRQ handler. `dw_dp_irq()` is registered via `devm_request_threaded_irq()` with no hard-IRQ handler, meaning it executes in process context. If the hardware asserts the AUX reply interrupt just before the transfer timeout expires, the IRQ thread may wake up and read the interrupt status (`value & AUX_REPLY_EVENT`), but get preempted before calling `complete(&dp->complete)`. The timeout handler will then reset the hardware and return `-ETIMEDOUT`. When a subsequent transfer starts, it calls `reinit_completion(&dp->complete)`. If the delayed IRQ thread resumes and calls `complete()` at this point, it will erroneously satisfy the new transfer's wait, causing it to read aborted or reset hardware status. To fully close this race, the timeout path must synchronize with the threaded IRQ handler (e.g., using `synchronize_irq()`) before returning.
--

--- Patch [5]: [PATCH v7 05/18] drm/bridge: synopsys: dw-dp: Reset AUX channel on transfer timeout ---
commit fd706e1cca6ddcdd0e07e034b13e849a9aa5e4b5
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Reset AUX channel on transfer timeout
    
This patch resets the AUX controller when a transfer times out to prevent a
delayed reply interrupt from leaking into a subsequent transfer.
Re-initializing the completion before each transfer does not protect against
a reply interrupt that fires after the initialization.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1493,6 +1493,11 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
>  	status = wait_for_completion_timeout(&dp->complete, timeout);
>  	if (!status) {
>  		dev_err(dp->dev, "timeout waiting for AUX reply\n");
> +		regmap_update_bits(dp->regmap, DW_DP_SOFT_RESET_CTRL,
> +				   AUX_RESET, FIELD_PREP(AUX_RESET, 1));
> +		usleep_range(10, 20);
> +		regmap_update_bits(dp->regmap, DW_DP_SOFT_RESET_CTRL,
> +				   AUX_RESET, FIELD_PREP(AUX_RESET, 0));
>  		return -ETIMEDOUT;

[Severity: High]
This is a pre-existing issue, but does this soft reset fully close the race
with the threaded IRQ handler?

If the hardware asserts the AUX reply interrupt exactly as the timeout
expires, the threaded IRQ handler dw_dp_irq() could wake up and read the
interrupt status:

dw_dp_irq() {
	...
	if (value & AUX_REPLY_EVENT) {
		regmap_write(dp->regmap, DW_DP_GENERAL_INTERRUPT, AUX_REPLY_EVENT);

If the thread is preempted right here, the timeout handling above executes,
the hardware is reset, and the function returns -ETIMEDOUT.

When a subsequent transfer begins and calls reinit_completion(&dp->complete),
could the delayed IRQ thread resume and execute the completion?

		complete(&dp->complete);
	}
}

This would erroneously satisfy the new transfer's wait and cause it to read
aborted hardware status. Would calling synchronize_irq() in this timeout path
prevent the completion from leaking?

>  	}
>

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