Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Enabling PM runtime leaves the DSI hardware permanently suspended, resulting in system crashes during unpowered MMIO register accesses. - [High] The IRQ handler accesses hardware registers without verifying the device's PM runtime state. --
--- Patch [8]: [PATCH v4 08/10] drm/mediatek: mtk_dsi: Enable PM Runtime on probe --- commit 7ce79b5636ac6fccf8b21d5b0f6a67f31c6b10eb 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() to allow the power domains for the DSI IP to be turned on and off when necessary. The commit message assumes mtk_ddp_comp_power_on() will resume the device. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 93bc507d77e20..1726f5a2683cf 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -1731,6 +1731,10 @@ static int mtk_dsi_probe(struct platform_device *pdev) > if (ret) > return dev_err_probe(&pdev->dev, ret, "Failed to request DSI > irq\n"); > > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; > + [Severity: High] The commit message states that mtk_ddp_comp_power_on() will resume the device. Is this assumption correct? Looking at mtk_crtc_atomic_enable() in drivers/gpu/drm/mediatek/mtk_crtc.c, mtk_ddp_comp_power_on() is only called for the first component in the display pipeline (comp[0]), bypassing the DSI component entirely. Furthermore, even if mtk_ddp_comp_power_on() iterated over all components, it executes after mtk_dsi_bridge_atomic_pre_enable(). The pre-enable hook calls mtk_dsi_poweron(), which directly accesses hardware registers: drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_poweron() { ... mtk_dsi_enable(dsi); /* Bypass shadow and force commit only if the register is present */ if (data->reg_adv[DSI_SHADOW_DEBUG]) writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + data->reg_adv[DSI_SHADOW_DEBUG]); ... } Since the PM domain is still powered off at this point, will these direct MMIO accesses cause a bus hang or system crash? [Severity: High] Now that the device supports PM runtime, can the IRQ handler run while the device is in the RPM_SUSPENDED state? The interrupt is unconditionally enabled in mtk_dsi_bind() and remains enabled at the IRQ controller level even when the device is suspended. If a spurious or delayed interrupt fires, mtk_dsi_irq() directly reads the hardware register: drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_irq() { ... status = readl(dsi->regs + dsi->driver_data->reg_main[DSI_INTSTA]) & flag; ... } Should the IRQ handler use pm_runtime_get_if_active() before accessing hardware registers to avoid touching unpowered hardware? > ret = mipi_dsi_host_register(&dsi->host); > if (ret < 0) > return dev_err_probe(dev, ret, "Failed to register DSI host\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
