Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/mediatek: DSI device is perpetually suspended causing external abort - [High] drm/mediatek: mtk_dsi_host_transfer accesses unpowered registers - [High] drm/mediatek: DSI IRQ handler accesses unpowered registers --
commit 26652cbd743202a1f8035ecf743f6c0b1cf91008 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() for the DSI device to ensure power domains are turned on and off when necessary. The commit message indicates that mtk_ddp_comp_power_on() and mtk_ddp_comp_power_off() will handle the PM runtime resume and put. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 3b33419aee2d9..4863d910511e9 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -1735,6 +1735,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); [Severity: High] Does enabling PM runtime here leave the DSI device perpetually suspended, causing an external abort when mtk_dsi_poweron() accesses the registers? The commit message assumes mtk_ddp_comp_power_on() will resume the device. However, mtk_crtc_atomic_enable() in drivers/gpu/drm/mediatek/mtk_crtc.c only calls mtk_ddp_comp_power_on() for comp[0] (typically OVL or RDMA). Since the DSI component is always at the end of the pipeline, it is never passed to this function and remains in the suspended state. [Severity: High] Can mtk_dsi_host_transfer() access unpowered registers when called asynchronously? This function is an exported MIPI DSI host operation that can be called by panel drivers to send commands. With PM runtime enabled, the function directly reads hardware registers without wrapping the access in pm_runtime_resume_and_get() and pm_runtime_put(): drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_host_transfer() { ... dsi_mode = readl(dsi->regs + dsi->driver_data->reg_main[DSI_MODE_CTRL]); ... } If the power domain is off, this could lead to a synchronous external abort. [Severity: High] Will the DSI IRQ handler access unpowered registers if an interrupt fires while the domain is suspended? The mtk_dsi_irq() handler directly accesses hardware registers: drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_irq() { ... status = readl(dsi->regs + reg_main[DSI_INTSTA]) & flag; ... } Since the driver does not appear to call disable_irq() during suspend, a pending or spurious interrupt firing while the domain is unpowered could cause the handler to access unpowered registers. Should the handler use pm_runtime_get_if_active() before accessing registers, or should the IRQ be disabled during power off? > + if (ret) > + return ret; > + > 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=11
