Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Enabling the DSI IRQ before hardware clocks are enabled, or leaving it enabled while clocks are disabled, risks bus hangs from spurious or pending interrupts. - [High] Unprotected read-modify-write operations on `dsi->irq_data` can lead to lost interrupts and spurious timeouts. - [High] Signal interruptions during DSI transfers are incorrectly treated as successful completions, potentially leaking stale hardware register data. -- --- Patch [4]: [PATCH v3 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time --- commit 9bfbf9137c89977bfb322670b9598bed791b5832 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Enable interrupt at component bind time This patch changes the DSI interrupt initialization by requesting it with IRQF_NO_AUTOEN during probe. The interrupt is then explicitly enabled during component bind and disabled during unbind. This prevents spurious interrupts and storms before the hardware engine is fully initialized. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index ec5b94f7fa68e..bdb9ea2e7b6f6 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > @@ -1088,7 +1089,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) > + return ret; > + > + enable_irq(dsi->irq); [Severity: High] This is a pre-existing issue, but does enabling the IRQ here risk a bus hang if an interrupt fires before the hardware clocks are fully enabled? Looking at mtk_dsi_bind(), the IRQ is unmasked here, but the engine_clk and digital_clk are not enabled until mtk_dsi_poweron() is called later. If a spurious or pending interrupt fires during this window, the handler: mtk_dsi_irq() status = readl(dsi->regs + DSI_INTSTA) & flag; will access clock-gated registers, which can cause an unrecoverable system error or bus stall on MediaTek SoCs. A similar window seems to exist in mtk_dsi_poweroff() where clocks are disabled without masking the IRQ. > + > + return 0; > } [ ... ] > @@ -1372,17 +1380,15 @@ static int mtk_dsi_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, dsi); > > + ret = devm_request_irq(&pdev->dev, dsi->irq, mtk_dsi_irq, > + IRQF_NO_AUTOEN, dev_name(&pdev->dev), dsi); [Severity: High] This isn't a bug introduced by this patch, but are there data races in how the interrupt handler updates the status flags? The irq_data field is modified concurrently by process context and hardirq context without locking: Process context: mtk_dsi_irq_data_clear() dsi->irq_data &= ~irq_bit; Hardirq context: mtk_dsi_irq_data_set() dsi->irq_data |= irq_bit; If mtk_dsi_irq() fires between the non-atomic load and store in process context, the flag set by the interrupt handler would be overwritten and lost, potentially causing spurious timeouts. [Severity: High] This is a pre-existing issue, but does the wait condition correctly handle signals? When waiting for the interrupt: mtk_dsi_host_send_cmd() if (!mtk_dsi_wait_for_irq_done(dsi, flag, 2000)) return -ETIME; else return 0; If wait_event_interruptible_timeout() in mtk_dsi_wait_for_irq_done() is interrupted by a signal, it returns -ERESTARTSYS. Since !negative evaluates to false (0), the error check is bypassed and it returns 0 (success). This could cause the driver to read uninitialized RX data from DSI registers and corrupt the communication state machine. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
