Re: [PATCH v3 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Enabling the DSI IRQ before hardware clocks are enabled, or leaving it enabled while clocks are disabled, risks bus hangs from spurious or pending interrupts.
- [High] Unprotected read-modify-write operations on `dsi->irq_data` can lead to lost interrupts and spurious timeouts.
- [High] Signal interruptions during DSI transfers are incorrectly treated as successful completions, potentially leaking stale hardware register data.
--

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

This patch changes the DSI interrupt initialization by requesting it with
IRQF_NO_AUTOEN during probe. The interrupt is then explicitly enabled
during component bind and disabled during unbind. This prevents spurious
interrupts and storms before the hardware engine is fully initialized.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ec5b94f7fa68e..bdb9ea2e7b6f6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c

[ ... ]

> @@ -1088,7 +1089,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;
> +
> +	enable_irq(dsi->irq);

[Severity: High]
This is a pre-existing issue, but does enabling the IRQ here risk a bus
hang if an interrupt fires before the hardware clocks are fully enabled?

Looking at mtk_dsi_bind(), the IRQ is unmasked here, but the engine_clk
and digital_clk are not enabled until mtk_dsi_poweron() is called later.
If a spurious or pending interrupt fires during this window, the handler:

mtk_dsi_irq()
    status = readl(dsi->regs + DSI_INTSTA) & flag;

will access clock-gated registers, which can cause an unrecoverable system
error or bus stall on MediaTek SoCs. A similar window seems to exist in
mtk_dsi_poweroff() where clocks are disabled without masking the IRQ.

> +
> +	return 0;
>  }

[ ... ]

> @@ -1372,17 +1380,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 are there data races in how
the interrupt handler updates the status flags?

The irq_data field is modified concurrently by process context and hardirq
context without locking:

Process context:
mtk_dsi_irq_data_clear()
    dsi->irq_data &= ~irq_bit;

Hardirq context:
mtk_dsi_irq_data_set()
    dsi->irq_data |= irq_bit;

If mtk_dsi_irq() fires between the non-atomic load and store in process
context, the flag set by the interrupt handler would be overwritten and lost,
potentially causing spurious timeouts.

[Severity: High]
This is a pre-existing issue, but does the wait condition correctly handle
signals?

When waiting for the interrupt:

mtk_dsi_host_send_cmd()
    if (!mtk_dsi_wait_for_irq_done(dsi, flag, 2000))
        return -ETIME;
    else
        return 0;

If wait_event_interruptible_timeout() in mtk_dsi_wait_for_irq_done() is
interrupted by a signal, it returns -ERESTARTSYS. Since !negative evaluates
to false (0), the error check is bypassed and it returns 0 (success). This
could cause the driver to read uninitialized RX data from DSI registers and
corrupt the communication state machine.

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