Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing cleanup of DRM encoder when device_reset_optional() fails in mtk_dsi_bind(). - [High] Read-modify-write data race on `dsi->irq_data` between process context and hardirq context. -- --- Patch [4]: [PATCH v4 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time --- commit 0607d3868a389c4103df3225d2bc7a47d44df3b0 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Enable interrupt at component bind time This patch requests the DSI interrupt with IRQF_NO_AUTOEN and enables it only when binding the DSI to its display controller component master. This prevents interrupt storms and delays caused by timeouts during probe. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 8ab5c3431dbbc..e24e7b91a3618 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > @@ -1090,7 +1091,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; [Severity: High] This isn't a bug introduced by this patch, but does this error path leak the DRM encoder? If device_reset_optional(dev) fails, it returns without calling drm_encoder_cleanup(&dsi->encoder). Because the component bind operation fails, the component framework will not invoke mtk_dsi_unbind(), meaning the encoder is never cleaned up. If the DSI driver is subsequently unbound or removed, its locally managed memory is freed while the encoder is still linked in the DRM core's list, which could lead to a use-after-free or kernel panic when the list is traversed. > + > + enable_irq(dsi->irq); > + > + return 0; > } [ ... ] > @@ -1375,17 +1383,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 is a pre-existing issue, but is there a read-modify-write data race on dsi->irq_data between process context and hardirq context? mtk_dsi_irq_data_clear() modifies dsi->irq_data using &= in process context (from mtk_dsi_host_transfer()) without disabling interrupts or using atomic bitwise operations. Concurrently, the hardirq handler mtk_dsi_irq() modifies the same variable using |=. If a DSI interrupt fires during the process-context read-modify-write sequence, won't the flag set by the interrupt handler be silently overwritten and lost when the process context stores its computed value? This could lead to hardware interrupt events being lost, causing timeouts in mtk_dsi_wait_for_irq_done() and potentially breaking display initialization or frame updates. > + 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=4
