Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Unprotected read-modify-write of shared state `dsi->irq_data` across process and hardirq contexts. - [High] Incorrect handling of `wait_event_interruptible_timeout()` return value treats interrupted system calls as successful operations. - [High] Infinite busy-wait loop in the hardware interrupt handler. - [High] DSI Host is registered before the interrupt handler is requested, causing timeouts during panel probe. -- --- Patch [4]: [PATCH v6 04/11] drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind --- commit 4140e363279861f85198ff2dac937aba26d9610b Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind This commit adds an error handling path during DSI component binding. It ensures the encoder is properly cleaned up if the optional device reset fails, avoiding a potential use-after-free on module removal. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 3237dc765701d..e4a156a474e08 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -1090,7 +1090,13 @@ static int mtk_dsi_bind(struct device *dev, struct > device *master, void *data) > if (ret) > return ret; > > - return device_reset_optional(dev); > + ret = device_reset_optional(dev); > + if (ret) { > + drm_encoder_cleanup(&dsi->encoder); > + return ret; > + } > + > + return 0; > } > > static void mtk_dsi_unbind(struct device *dev, struct device *master, While reviewing this file, I noticed a few pre-existing regressions in other parts of the code. [Severity: High] This isn't a bug introduced by this patch, but does this code risk losing interrupts? In drivers/gpu/drm/mediatek/mtk_dsi.c, mtk_dsi_irq_data_clear() modifies shared state from process context: static void mtk_dsi_irq_data_clear(struct mtk_dsi *dsi, u32 irq_bit) { dsi->irq_data &= ~irq_bit; } Meanwhile, the hardirq handler calls mtk_dsi_irq_data_set(): static void mtk_dsi_irq_data_set(struct mtk_dsi *dsi, u32 irq_bit) { dsi->irq_data |= irq_bit; } Because the process context modification is a non-atomic read-modify-write without disabling interrupts, could an interrupt firing between the read and write cause the bit set by the interrupt handler to be overwritten and lost, leading to spurious timeouts? [Severity: High] This is a pre-existing issue, but does this negation correctly handle signal interruptions? In mtk_dsi_host_send_cmd(), the code checks the return value of mtk_dsi_wait_for_irq_done(): if (!mtk_dsi_wait_for_irq_done(dsi, flag, 2000)) return -ETIME; else return 0; A similar check exists in mtk_dsi_switch_to_cmd_mode(): if (!mtk_dsi_wait_for_irq_done(dsi, irq_flag, t)) { drm_err(drm, "failed to switch cmd mode\n"); return -ETIME; } else { return 0; } Since mtk_dsi_wait_for_irq_done() returns the result of wait_event_interruptible_timeout(), it can return -ERESTARTSYS if interrupted by a signal. Because -ERESTARTSYS is non-zero, this evaluates to false, taking the else branch and returning 0 (success). Could this cause the driver to falsely assume the hardware operation completed successfully and read uninitialized or stale data? [Severity: High] This is a pre-existing issue, but is it safe to poll this register indefinitely inside the hardirq handler? In mtk_dsi_irq(), the driver enters a loop waiting for DSI_BUSY to clear: do { mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK); tmp = readl(dsi->regs + DSI_INTSTA); } while (tmp & DSI_BUSY); Since this runs in hardirq context without a timeout or counter, if a hardware malfunction keeps the DSI_BUSY bit asserted, could this cause the CPU to spin indefinitely with interrupts disabled, causing a hard lockup? [Severity: High] This isn't a bug introduced by this patch, but is the DSI host registered too early? In mtk_dsi_probe(), mipi_dsi_host_register() is called before the interrupt handler is requested: ret = mipi_dsi_host_register(&dsi->host); if (ret < 0) return dev_err_probe(dev, ret, "Failed to register DSI host\n"); ret = devm_request_irq(&pdev->dev, irq_num, mtk_dsi_irq, IRQF_TRIGGER_NONE, dev_name(&pdev->dev), dsi); Registering the host immediately triggers enumeration and probing of child panel drivers. If a panel driver sends a DCS command during its probe() routine before devm_request_irq() completes, the hardware interrupt will be missed. Could this cause the wait queue to time out and the panel probe to spuriously fail? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
