Re: [PATCH v6 04/11] drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected read-modify-write of shared state `dsi->irq_data` across process and hardirq contexts.
- [High] Incorrect handling of `wait_event_interruptible_timeout()` return value treats interrupted system calls as successful operations.
- [High] Infinite busy-wait loop in the hardware interrupt handler.
- [High] DSI Host is registered before the interrupt handler is requested, causing timeouts during panel probe.
--

--- Patch [4]: [PATCH v6 04/11] drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind ---
commit 4140e363279861f85198ff2dac937aba26d9610b
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind

This commit adds an error handling path during DSI component binding. It
ensures the encoder is properly cleaned up if the optional device reset
fails, avoiding a potential use-after-free on module removal.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 3237dc765701d..e4a156a474e08 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1090,7 +1090,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) {
> +		drm_encoder_cleanup(&dsi->encoder);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static void mtk_dsi_unbind(struct device *dev, struct device *master,

While reviewing this file, I noticed a few pre-existing regressions in other
parts of the code.

[Severity: High]
This isn't a bug introduced by this patch, but does this code risk losing
interrupts?

In drivers/gpu/drm/mediatek/mtk_dsi.c, mtk_dsi_irq_data_clear() modifies
shared state from process context:

static void mtk_dsi_irq_data_clear(struct mtk_dsi *dsi, u32 irq_bit)
{
        dsi->irq_data &= ~irq_bit;
}

Meanwhile, the hardirq handler calls mtk_dsi_irq_data_set():

static void mtk_dsi_irq_data_set(struct mtk_dsi *dsi, u32 irq_bit)
{
        dsi->irq_data |= irq_bit;
}

Because the process context modification is a non-atomic read-modify-write
without disabling interrupts, could an interrupt firing between the read
and write cause the bit set by the interrupt handler to be overwritten and
lost, leading to spurious timeouts?


[Severity: High]
This is a pre-existing issue, but does this negation correctly handle
signal interruptions?

In mtk_dsi_host_send_cmd(), the code checks the return value of
mtk_dsi_wait_for_irq_done():

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

A similar check exists in mtk_dsi_switch_to_cmd_mode():

        if (!mtk_dsi_wait_for_irq_done(dsi, irq_flag, t)) {
                drm_err(drm, "failed to switch cmd mode\n");
                return -ETIME;
        } else {
                return 0;
        }

Since mtk_dsi_wait_for_irq_done() returns the result of
wait_event_interruptible_timeout(), it can return -ERESTARTSYS if
interrupted by a signal. Because -ERESTARTSYS is non-zero, this evaluates to
false, taking the else branch and returning 0 (success). Could this cause
the driver to falsely assume the hardware operation completed successfully
and read uninitialized or stale data?


[Severity: High]
This is a pre-existing issue, but is it safe to poll this register
indefinitely inside the hardirq handler?

In mtk_dsi_irq(), the driver enters a loop waiting for DSI_BUSY to clear:

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

Since this runs in hardirq context without a timeout or counter, if a
hardware malfunction keeps the DSI_BUSY bit asserted, could this cause the
CPU to spin indefinitely with interrupts disabled, causing a hard lockup?


[Severity: High]
This isn't a bug introduced by this patch, but is the DSI host registered
too early?

In mtk_dsi_probe(), mipi_dsi_host_register() is called before the interrupt
handler is requested:

        ret = mipi_dsi_host_register(&dsi->host);
        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);

Registering the host immediately triggers enumeration and probing of child
panel drivers. If a panel driver sends a DCS command during its probe()
routine before devm_request_irq() completes, the hardware interrupt will be
missed. Could this cause the wait queue to time out and the panel probe to
spuriously fail?

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