Re: [PATCH v5 05/11] drm/mediatek: mtk_dsi: Enable interrupt at component bind time

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unsafe IRQ enablement before clocks are enabled leads to hard lockup or interrupt storm.
- [High] Data race on `dsi->irq_data` can lead to lost wakeups and hardware reset.
- [Medium] Unconditional return of IRQ_HANDLED defeats spurious interrupt detection.
--

--- Patch [5]: [PATCH v5 05/11] drm/mediatek: mtk_dsi: Enable interrupt at component bind time ---
commit dc54549e6b7ff6ba83355eb88d72dfbf4c41558d
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

This patch moves the DSI interrupt enablement to the component bind time and
disables it on unbind, rather than doing so during probe/remove. It uses
IRQF_NO_AUTOEN to prevent the interrupt from firing before the component
is ready.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index e3d7338c35e39..d1aa258e3799c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -225,6 +225,7 @@ struct mtk_dsi {
>  	int refcount;
>  	bool enabled;
>  	bool lanes_ready;
> +	int irq;
>  	u32 irq_data;
>  	wait_queue_head_t irq_wait_queue;
>  	const struct mtk_dsi_driver_data *driver_data;

[ ... ]

> @@ -1096,6 +1097,8 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>  
> +	enable_irq(dsi->irq);

[Severity: High]
This isn't a bug introduced by this patch, but can enabling the interrupt
here cause a hard lockup or interrupt storm?

The DSI engine_clk and digital_clk are not enabled until mtk_dsi_poweron() is
called later. If an interrupt fires while clocks are disabled, the handler
mtk_dsi_irq() will read unclocked registers.

If unclocked reads return 0xffffffff, status is non-zero and the loop:

mtk_dsi_irq() {
    ...
    do {
        mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK);
        tmp = readl(dsi->regs + DSI_INTSTA);
    } while (tmp & DSI_BUSY);
    ...
}

executes infinitely (since DSI_BUSY is BIT(31)), causing a hard lockup in
hardirq context.

> +
>  	return 0;
>  }

[ ... ]

> @@ -1381,17 +1385,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 isn't a bug introduced by this patch, but is there a data race on
dsi->irq_data?

mtk_dsi_irq_data_set() runs in hardirq context and sets bits in dsi->irq_data:
    dsi->irq_data |= irq_bit;

Concurrently, mtk_dsi_irq_data_clear() runs in process context and performs:
    dsi->irq_data &= ~irq_bit;

If an interrupt fires exactly after the load but before the store in
mtk_dsi_irq_data_clear(), could new flags set by the interrupt handler be
overwritten and permanently lost when the process context writes back its
stale value?


[Severity: Medium]
This is a pre-existing issue, but does mtk_dsi_irq() unconditionally return
IRQ_HANDLED even when no interrupt flags are detected?

If status is 0, the handler does nothing to clear the hardware state but
still returns IRQ_HANDLED. Will this defeat the kernel's spurious interrupt
detector and cause an interrupt storm if a spurious interrupt triggers or if
the hardware asserts an unmasked interrupt flag not handled by the driver?

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

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