Re: [PATCH v7 11/13] drm/mediatek: mtk_dsi: Enable PM Runtime on probe

[sashiko-bot@kernel.org]

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 <angelogioacchino.delregno@collabora.com>
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/20260914115812.22751-1-angelogioacchino.delregno@collabora.com?part=11