Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Calling `enable_irq()` in `mtk_dsi_bind()` executes before the IRQ is requested, leaving the interrupt permanently disabled. Pre-existing issues: - [High] Data race on `dsi->irq_data` between process and hardirq context leads to dropped interrupts. - [High] `mtk_dsi_host_send_cmd()` incorrectly interprets `-ERESTARTSYS` as success, causing the driver to process stale hardware memory. - [High] Unbounded busy-wait loop in `mtk_dsi_irq()` hard-locks the CPU if hardware stalls. -- --- Patch [4]: [PATCH v2 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time --- commit ab0ed7e073fb408fbb00443f437ebcc970005f69 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Enable interrupt at component bind time This patch changes the DSI interrupt to be requested with IRQF_NO_AUTOEN and enables it later during the component bind phase. This aims to prevent interrupt storms and artifacts caused by interrupts arriving before the display controller is fully ready. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 3e22553ca990f..284dae372c7c2 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > @@ -1052,7 +1053,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); > + > + return 0; > } > > static void mtk_dsi_unbind(struct device *dev, struct device *master, > @@ -1060,6 +1067,8 @@ static void mtk_dsi_unbind(struct device *dev, struct > device *master, > { > struct mtk_dsi *dsi = dev_get_drvdata(dev); > > + disable_irq(dsi->irq); > + > drm_encoder_cleanup(&dsi->encoder); > } [ ... ] > @@ -1338,8 +1346,8 @@ static int mtk_dsi_probe(struct platform_device *pdev) > 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); > + ret = devm_request_irq(&pdev->dev, dsi->irq, mtk_dsi_irq, > + IRQF_NO_AUTOEN, dev_name(&pdev->dev), dsi); [Severity: High] Does mipi_dsi_host_register() trigger component binding synchronously before the IRQ is actually requested here? If mtk_dsi_probe() invokes mipi_dsi_host_register() before devm_request_irq(): 1. mipi_dsi_host_register() synchronously probes child panel devices 2. The child devices invoke component_add() for the DSI component 3. If the aggregate device is ready, this synchronously triggers mtk_dsi_bind() 4. mtk_dsi_bind() then calls enable_irq(dsi->irq) Since devm_request_irq() hasn't run yet, will enable_irq() encounter an unrequested IRQ descriptor and abort without unmasking the hardware? When devm_request_irq() subsequently runs with IRQF_NO_AUTOEN, it seems the IRQ might remain disabled forever, completely breaking display operation. [Severity: High] This is a pre-existing issue, but can this code drop interrupts due to a data race on dsi->irq_data? Process context uses a load-modify-store to clear flags: mtk_dsi_irq_data_clear() dsi->irq_data &= ~irq_bit; While hardirq context uses a non-atomic operation to set flags: mtk_dsi_irq_data_set() dsi->irq_data |= irq_bit; If an interrupt fires between the load and store in process context, the interrupt handler will set its status flag, but the process context's store will overwrite dsi->irq_data with its stale value, destroying the newly set flag and potentially causing wait_event polling timeouts. [Severity: High] This is a pre-existing issue, but does mtk_dsi_host_send_cmd() incorrectly interpret -ERESTARTSYS as success? mtk_dsi_wait_for_irq_done() returns -ERESTARTSYS if interrupted by a signal: mtk_dsi_wait_for_irq_done() ret = wait_event_interruptible_timeout(...); However, mtk_dsi_host_send_cmd() evaluates the return value as a boolean: mtk_dsi_host_send_cmd() if (!mtk_dsi_wait_for_irq_done(dsi, flag, 2000)) return -ETIME; else return 0; Since -ERESTARTSYS is non-zero, its negation evaluates to false, causing execution to bypass the error path and return 0. The caller then assumes the hardware operation completed successfully and might proceed to read stale memory from the hardware RX registers. [Severity: High] This is a pre-existing issue, but can the busy-wait loop in mtk_dsi_irq() cause a hard lockup? The mtk_dsi_irq() interrupt handler runs in hardirq context and contains a polling loop without a timeout: mtk_dsi_irq() do { mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK); tmp = readl(dsi->regs + DSI_INTSTA); } while (tmp & DSI_BUSY); If the DSI hardware encounters an error, loses its clock, or gets stuck in a busy state, the DSI_BUSY bit will never clear and the CPU will spin indefinitely. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
