Re: [PATCH v4 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup of DRM encoder when device_reset_optional() fails in mtk_dsi_bind().
- [High] Read-modify-write data race on `dsi->irq_data` between process context and hardirq context.
--

--- Patch [4]: [PATCH v4 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time ---
commit 0607d3868a389c4103df3225d2bc7a47d44df3b0
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dsi: Enable interrupt at component bind time

This patch requests the DSI interrupt with IRQF_NO_AUTOEN and enables it only
when binding the DSI to its display controller component master. This prevents
interrupt storms and delays caused by timeouts during probe.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 8ab5c3431dbbc..e24e7b91a3618 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -1090,7 +1091,13 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	return device_reset_optional(dev);
> +	ret = device_reset_optional(dev);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak
the DRM encoder?

If device_reset_optional(dev) fails, it returns without calling
drm_encoder_cleanup(&dsi->encoder). Because the component bind operation
fails, the component framework will not invoke mtk_dsi_unbind(), meaning
the encoder is never cleaned up.

If the DSI driver is subsequently unbound or removed, its locally managed
memory is freed while the encoder is still linked in the DRM core's list,
which could lead to a use-after-free or kernel panic when the list is
traversed.

> +
> +	enable_irq(dsi->irq);
> +
> +	return 0;
>  }
[ ... ]
> @@ -1375,17 +1383,15 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, dsi);
>  
> +	ret = devm_request_irq(&pdev->dev, dsi->irq, mtk_dsi_irq,
> +			       IRQF_NO_AUTOEN, dev_name(&pdev->dev), dsi);

[Severity: High]
This is a pre-existing issue, but is there a read-modify-write data race on
dsi->irq_data between process context and hardirq context?

mtk_dsi_irq_data_clear() modifies dsi->irq_data using &= in process context
(from mtk_dsi_host_transfer()) without disabling interrupts or using atomic
bitwise operations. Concurrently, the hardirq handler mtk_dsi_irq() modifies
the same variable using |=.

If a DSI interrupt fires during the process-context read-modify-write
sequence, won't the flag set by the interrupt handler be silently
overwritten and lost when the process context stores its computed value?

This could lead to hardware interrupt events being lost, causing timeouts
in mtk_dsi_wait_for_irq_done() and potentially breaking display
initialization or frame updates.

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to request DSI irq\n");
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713142746.40259-1-angelogioacchino.delregno@collabora.com?part=4