Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Enabling PM runtime leaves the DSI device perpetually suspended because `mtk_ddp_comp_power_on()` is only invoked for the first component in the display pipeline, never for the DSI device. Pre-existing issues: - [High] Bridge properties are initialized after registration, exposing an uninitialized state to concurrent lookups. - [High] Read-modify-write race on `dsi->irq_data` between process context and IRQ handler context leads to lost interrupts. -- commit 18651040b7788a002a5585acda6d5cf89879837e Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Enable PM Runtime on probe This commit enables PM runtime at the end of mtk_dsi_probe(), placing the DSI IP under PM domain control. It expects mtk_ddp_comp_power_on() and off functions to resume and suspend the device appropriately. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index d930cb3c4d74e..b668814b92000 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -1734,6 +1734,10 @@ static int mtk_dsi_probe(struct platform_device *pdev) [ ... ] > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; > + [Severity: High] Does this leave the DSI device perpetually suspended? The commit message states that mtk_ddp_comp_power_on() is expected to call pm_runtime_resume_and_get() for the DSI device. However, tracing mtk_crtc_atomic_enable() reveals that the power-on mechanism operates strictly on the first component in the display pipeline: drivers/gpu/drm/mediatek/mtk_crtc.c:mtk_crtc_atomic_enable() { ... struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0]; ... ret = mtk_ddp_comp_power_on(comp); ... } Since the DSI device is at the end of the pipeline and is never ddp_comp[0], its PM usage count wouldn't be incremented. Because the driver accesses hardware registers in mtk_dsi_poweron() while the device is still considered RPM_SUSPENDED by the PM framework, could this regression cause bus faults or unclocked access crashes? > ret = mipi_dsi_host_register(&dsi->host); > if (ret < 0) > return dev_err_probe(dev, ret, "Failed to register DSI host\n"); [Severity: High] This is a pre-existing issue, but does this expose an uninitialized bridge state to concurrent lookups? In mtk_dsi_probe(), mipi_dsi_host_register() is called before the bridge properties are initialized: drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_probe() { ... ret = mipi_dsi_host_register(&dsi->host); ... dsi->bridge.of_node = dev->of_node; dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; ... } Because mipi_dsi_host_register() can synchronously probe child panel devices, the child's attachment sequence triggers mtk_dsi_host_attach(), which calls drm_bridge_add(&dsi->bridge). This publishes the bridge to the global list before of_node and type are set. Could concurrent iterations over the global bridge_list (like drm_of_find_panel_or_bridge()) observe a NULL of_node and encounter lookup failures or spurious -EPROBE_DEFER errors? Additionally, while reviewing this code, I noticed another pre-existing issue in the same file: [Severity: High] This is a pre-existing issue, but can the read-modify-write pattern on dsi->irq_data lead to lost interrupts? The variable dsi->irq_data is updated from both process context and hardware interrupt context using unprotected read-modify-write operations: Process context: mtk_dsi_irq_data_clear() { dsi->irq_data &= ~irq_bit; } Hardware interrupt context: mtk_dsi_irq_data_set() { dsi->irq_data |= irq_bit; } If a hardware interrupt fires between the load and store instructions of the process context update, the process context will immediately overwrite the IRQ handler's updated value with its stale loaded value. Could this erase the interrupt flag and lead to missed wakeups and timeouts in wait_event_interruptible_timeout() during display initialization? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
