Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing cleanup of DSI host registration on devm_pm_runtime_enable() failure. - [High] The DSI device is never runtime-resumed by mtk_ddp_comp_power_on(), leaving its power domain off and causing hardware access faults. - [High] devm_pm_runtime_enable() is called too late in the probe sequence, creating a race condition with DRM device binding. --
--- Patch [8]: [PATCH v2 08/10] drm/mediatek: mtk_dsi: Enable PM Runtime on probe --- commit a3706ccca44411c23c1950cb6ce7edfa10269c16 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Enable PM Runtime on probe Enable PM Runtime at the end of mtk_dsi_probe() so that the mediatek-drm mtk_ddp_comp_power_on() function is able to do pm_runtime_resume_and_get() and, analogously, the function mtk_ddp_comp_power_off() to pm_runtime_put() for the DSI device. This makes sure that the power domains for the DSI IP are turned on and off when necessary. Signed-off-by: AngeloGioacchino Del Regno <[email protected]> > Enable PM Runtime at the end of mtk_dsi_probe() so that the > mediatek-drm mtk_ddp_comp_power_on() function is able to do > pm_runtime_resume_and_get() and, analogously, the function > mtk_ddp_comp_power_off() to pm_runtime_put() for the DSI > device. [Severity: High] Does mtk_ddp_comp_power_on() actually get called for the DSI device? Looking at mtk_crtc_atomic_enable(), it only powers on the first component in the 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 an encoder/bridge at the end of the pipeline (e.g., ddp_comp[6]), it won't be resumed here, leaving it in RPM_SUSPENDED state. Will this cause hardware access faults when DSI registers are subsequently accessed? > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 5af44c13bd5ac..5fbaa7d580d35 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -1684,6 +1684,10 @@ static int mtk_dsi_probe(struct platform_device *pdev) > dsi->bridge.of_node = dev->of_node; > dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; [Severity: High] If this returns early, will it leak the DSI host registration? Earlier in mtk_dsi_probe(), mipi_dsi_host_register(&dsi->host) adds the host to a global list. Since there is no mipi_dsi_host_unregister() call on this error path, and the devres-managed mtk_dsi structure will be freed, does this leave a dangling pointer in the MIPI DSI host list that could lead to a use-after-free? [Severity: High] Is it safe to enable PM runtime after registering the DSI host? By calling mipi_dsi_host_register() earlier in the function, the DSI host becomes visible to dependent panel drivers that may probe synchronously: mtk_dsi_probe() mipi_dsi_host_register() -> panel probe triggers mipi_dsi_attach() mtk_dsi_host_attach() component_add() -> DRM master bind triggers if all components are ready Since devm_pm_runtime_enable() hasn't executed yet, PM runtime isn't ready when the display pipeline starts initializing. Would it be better to move devm_pm_runtime_enable() before mipi_dsi_host_register() to avoid this race? > + > return 0; > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
