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 <[email protected]> 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/[email protected]?part=1
