Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Unsafe IRQ enablement before clocks are enabled leads to hard lockup or interrupt storm. - [High] Data race on `dsi->irq_data` can lead to lost wakeups and hardware reset. - [Medium] Unconditional return of IRQ_HANDLED defeats spurious interrupt detection. -- --- Patch [5]: [PATCH v5 05/11] drm/mediatek: mtk_dsi: Enable interrupt at component bind time --- commit dc54549e6b7ff6ba83355eb88d72dfbf4c41558d Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Enable interrupt at component bind time This patch moves the DSI interrupt enablement to the component bind time and disables it on unbind, rather than doing so during probe/remove. It uses IRQF_NO_AUTOEN to prevent the interrupt from firing before the component is ready. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index e3d7338c35e39..d1aa258e3799c 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -225,6 +225,7 @@ struct mtk_dsi { > int refcount; > bool enabled; > bool lanes_ready; > + int irq; > u32 irq_data; > wait_queue_head_t irq_wait_queue; > const struct mtk_dsi_driver_data *driver_data; [ ... ] > @@ -1096,6 +1097,8 @@ static int mtk_dsi_bind(struct device *dev, struct > device *master, void *data) > return ret; > } > > + enable_irq(dsi->irq); [Severity: High] This isn't a bug introduced by this patch, but can enabling the interrupt here cause a hard lockup or interrupt storm? The DSI engine_clk and digital_clk are not enabled until mtk_dsi_poweron() is called later. If an interrupt fires while clocks are disabled, the handler mtk_dsi_irq() will read unclocked registers. If unclocked reads return 0xffffffff, status is non-zero and the loop: mtk_dsi_irq() { ... do { mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK); tmp = readl(dsi->regs + DSI_INTSTA); } while (tmp & DSI_BUSY); ... } executes infinitely (since DSI_BUSY is BIT(31)), causing a hard lockup in hardirq context. > + > return 0; > } [ ... ] > @@ -1381,17 +1385,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 is there a data race on dsi->irq_data? mtk_dsi_irq_data_set() runs in hardirq context and sets bits in dsi->irq_data: dsi->irq_data |= irq_bit; Concurrently, mtk_dsi_irq_data_clear() runs in process context and performs: dsi->irq_data &= ~irq_bit; If an interrupt fires exactly after the load but before the store in mtk_dsi_irq_data_clear(), could new flags set by the interrupt handler be overwritten and permanently lost when the process context writes back its stale value? [Severity: Medium] This is a pre-existing issue, but does mtk_dsi_irq() unconditionally return IRQ_HANDLED even when no interrupt flags are detected? If status is 0, the handler does nothing to clear the hardware state but still returns IRQ_HANDLED. Will this defeat the kernel's spurious interrupt detector and cause an interrupt storm if a spurious interrupt triggers or if the hardware asserts an unmasked interrupt flag not handled by the driver? > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "Failed to request DSI > irq\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
