Re: [PATCH v3 2/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:

Pre-existing issues:
- [Critical] Missing cancel_work_sync() for ctx->reset_work during driver teardown leads to a Use-After-Free.
- [High] Skipping enable_irq() on the error path causes an IRQ disable depth leak, leaving the interrupt permanently disabled.
--

--- Patch [2]: [PATCH v3 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() ---
commit 58246b5ef131d7aeea7b5c2b693088e7255aa435
Author: Esben Haabendal <esben@geanix.com>

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

This commit fixes the error path in sn65dsi83_reset_work() by adding a
goto to ensure drm_bridge_exit() is called when sn65dsi83_reset_pipe()
fails, safely closing the critical section.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -419,11 +419,12 @@ static void sn65dsi83_reset_work(struct work_struct *ws)

[Severity: Critical]
This is a pre-existing issue, but could this work function execute after
the device is unbound, resulting in a use-after-free?

If the driver is removed and devres frees the ctx structure, the work
could still be pending since sn65dsi83_remove() does not appear to call
cancel_work_sync() before returning:

sn65dsi83_remove() {
        ...
        drm_bridge_unplug(&ctx->bridge);
        // missing cancel_work_sync(&ctx->reset_work);
}

If the workqueue executes sn65dsi83_reset_work() after teardown,
dereferencing ctx for drm_bridge_enter() might access freed memory.

>  	ret = sn65dsi83_reset_pipe(ctx);
>  	if (ret) {
>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> -		return;
> +		goto err_exit;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't jumping to err_exit
here leave the interrupt permanently disabled?

If a hardware error triggers the IRQ handler, sn65dsi83_handle_errors()
calls disable_irq_nosync(), which increments the IRQ disable depth. By
skipping the enable_irq() call on this error path, the depth counter
remains unbalanced.

Would it be better to mask the interrupt source via hardware registers
on failure, and still call enable_irq() so the kernel's depth counter
remains balanced?

>  	}
>  	if (ctx->irq)
>  		enable_irq(ctx->irq);
>  
> +err_exit:
>  	drm_bridge_exit(idx);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-ti-sn65dsi83-fixes-v3-0-9b403a405ccd@geanix.com?part=2