Re: [PATCH v4 1/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditionally re-enabling the interrupt after a failed hardware reset introduces a severe interrupt and workqueue storm.

Pre-existing issues:
- [Critical] Missing workqueue cancellation in the driver remove path leads to a Use-After-Free of the driver context.
--

--- Patch [1]: [PATCH v4 1/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() ---
commit 9e0c74443fddb87d6c513a0e21cb5c153b2b2c3a
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

This commit changes the error handling in sn65dsi83_reset_work() so that
enable_irq() and drm_bridge_exit() are called even if sn65dsi83_reset_pipe()
fails.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 42b451432bbb6..0ccfff401b2a6 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -417,10 +417,9 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  
>  	/* Reset the pipe */
>  	ret = sn65dsi83_reset_pipe(ctx);
> -	if (ret) {
> +	if (ret)
>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> -		return;
> -	}
> +
>  	if (ctx->irq)
>  		enable_irq(ctx->irq);

[Severity: High]
Does unconditionally re-enabling the interrupt after a failed hardware reset
introduce an interrupt and workqueue storm regression?

If sn65dsi83_reset_pipe() fails, the hardware IRQ status register
(REG_IRQ_STAT) remains uncleared because the reset sequence did not
successfully reach sn65dsi83_atomic_enable().

Since enable_irq(ctx->irq) is now called in sn65dsi83_reset_work() even on
failure, will the hardware immediately trigger the interrupt again? This would
cause the interrupt handler to run, disable the IRQ, and schedule reset_work.
The work runs, fails again, and re-enables the IRQ, creating an infinite loop
that could lock up a CPU core.

[Severity: Critical]
This is a pre-existing issue, but does the driver missing workqueue
cancellation in the remove path lead to a use-after-free of the driver context?

The driver context ctx is dynamically allocated via devm_drm_bridge_alloc() and
is automatically freed by devres after sn65dsi83_remove() returns:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_remove() {
	struct sn65dsi83 *ctx = i2c_get_clientdata(client);

	drm_bridge_unplug(&ctx->bridge);
}

The teardown path never calls cancel_work_sync(&ctx->reset_work). If an
interrupt schedules the work just before removal, and it hasn't completed when
remove() returns, the scheduled work will run after devres frees ctx.

The work function sn65dsi83_reset_work() then dereferences the freed ctx:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
	...
	if (!drm_bridge_enter(&ctx->bridge, &idx))
		return;
	...
}

Should cancel_work_sync() be added to sn65dsi83_remove() to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-ti-sn65dsi83-fixes-v4-0-ae9c95c8d162@geanix.com?part=1