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

New issues:
- [High] Calling `enable_irq()` in `mtk_dsi_bind()` executes before the IRQ is requested, leaving the interrupt permanently disabled.

Pre-existing issues:
- [High] Data race on `dsi->irq_data` between process and hardirq context leads to dropped interrupts.
- [High] `mtk_dsi_host_send_cmd()` incorrectly interprets `-ERESTARTSYS` as success, causing the driver to process stale hardware memory.
- [High] Unbounded busy-wait loop in `mtk_dsi_irq()` hard-locks the CPU if hardware stalls.
--

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

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

This patch changes the DSI interrupt to be requested with IRQF_NO_AUTOEN
and enables it later during the component bind phase. This aims to prevent
interrupt storms and artifacts caused by interrupts arriving before the
display controller is fully ready.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 3e22553ca990f..284dae372c7c2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c

[ ... ]

> @@ -1052,7 +1053,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);
> +
> +	return 0;
>  }
>  
>  static void mtk_dsi_unbind(struct device *dev, struct device *master,
> @@ -1060,6 +1067,8 @@ static void mtk_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct mtk_dsi *dsi = dev_get_drvdata(dev);
>  
> +	disable_irq(dsi->irq);
> +
>  	drm_encoder_cleanup(&dsi->encoder);
>  }

[ ... ]

> @@ -1338,8 +1346,8 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to register DSI host\n");
>  
> -	ret = devm_request_irq(&pdev->dev, irq_num, mtk_dsi_irq,
> -			       IRQF_TRIGGER_NONE, dev_name(&pdev->dev), dsi);
> +	ret = devm_request_irq(&pdev->dev, dsi->irq, mtk_dsi_irq,
> +			       IRQF_NO_AUTOEN, dev_name(&pdev->dev), dsi);

[Severity: High]
Does mipi_dsi_host_register() trigger component binding synchronously before
the IRQ is actually requested here?

If mtk_dsi_probe() invokes mipi_dsi_host_register() before devm_request_irq():
1. mipi_dsi_host_register() synchronously probes child panel devices
2. The child devices invoke component_add() for the DSI component
3. If the aggregate device is ready, this synchronously triggers mtk_dsi_bind()
4. mtk_dsi_bind() then calls enable_irq(dsi->irq)

Since devm_request_irq() hasn't run yet, will enable_irq() encounter an
unrequested IRQ descriptor and abort without unmasking the hardware? When
devm_request_irq() subsequently runs with IRQF_NO_AUTOEN, it seems the IRQ
might remain disabled forever, completely breaking display operation.

[Severity: High]
This is a pre-existing issue, but can this code drop interrupts due to a data
race on dsi->irq_data?

Process context uses a load-modify-store to clear flags:

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

While hardirq context uses a non-atomic operation to set flags:

mtk_dsi_irq_data_set()
    dsi->irq_data |= irq_bit;

If an interrupt fires between the load and store in process context, the
interrupt handler will set its status flag, but the process context's store
will overwrite dsi->irq_data with its stale value, destroying the newly set
flag and potentially causing wait_event polling timeouts.

[Severity: High]
This is a pre-existing issue, but does mtk_dsi_host_send_cmd() incorrectly
interpret -ERESTARTSYS as success?

mtk_dsi_wait_for_irq_done() returns -ERESTARTSYS if interrupted by a signal:

mtk_dsi_wait_for_irq_done()
    ret = wait_event_interruptible_timeout(...);

However, mtk_dsi_host_send_cmd() evaluates the return value as a boolean:

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

Since -ERESTARTSYS is non-zero, its negation evaluates to false, causing
execution to bypass the error path and return 0. The caller then assumes the
hardware operation completed successfully and might proceed to read stale
memory from the hardware RX registers.

[Severity: High]
This is a pre-existing issue, but can the busy-wait loop in mtk_dsi_irq()
cause a hard lockup?

The mtk_dsi_irq() interrupt handler runs in hardirq context and contains
a polling loop without a timeout:

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

If the DSI hardware encounters an error, loses its clock, or gets stuck in a
busy state, the DSI_BUSY bit will never clear and the CPU will spin
indefinitely.

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