Re: [PATCH v4 5/5] drm/vc4: dsi: Recover from a timed-out transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/vc4: dsi: race on reg_dma_mem before disable_irq() in vc4_dsi_host_transfer()
--

commit 748a80df1fc43dd3d1d753f275003873696caaaf
Author: Henry Zhang <henryzhang62@gmail.com>

drm/vc4: dsi: Recover from a timed-out transfer

This commit recovers the VC4 DSI block after a transfer timeout by resetting
the DSI controller and its analog front end, then restoring the hardware
configuration from pre_enable while holding the interrupt disabled.

> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 3ee6e8e86a147..6edda74e1117c 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c

[ ... ]

> @@ -1374,6 +1378,120 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
>  	return ret;
>  }
>  
> +/*
> + * Reset the DSI block and its analog front end, then restore the
> + * configuration done in pre_enable. Resetting the FIFOs alone does not
> + * clear the state the block can be left in after a transfer timed out
> + * (e.g. on an LP receive timeout): every following transfer then times
> + * out as well, until the next modeset.
> + */
> +static void vc4_dsi_reset_and_restore(struct vc4_dsi *dsi)
> +{
> +	u32 en = dsi->variant->port ? DSI1_CTRL_EN : DSI0_CTRL_CTRL0;
> +	u32 ctrl = DSI_PORT_READ(CTRL);

[ ... ]

> +	/* Ungate the block. */
> +	DSI_PORT_WRITE(CTRL, ctrl);
> +	/* Bring AFE out of reset. */
> +	DSI_PORT_WRITE(PHY_AFEC0, afec0 & ~DSI_PORT_BIT(PHY_AFEC0_RESET));
> +	DSI_PORT_WRITE(DISP0_CTRL, disp0);
> +
> +	DSI_PORT_WRITE(INT_STAT, DSI_PORT_READ(INT_STAT));
> +	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
> +}
> +
> +static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
> +				     const struct mipi_dsi_msg *msg)
> +{
> +	struct vc4_dsi *dsi = host_to_dsi(host);
> +	u32 disp0_ctrl;
> +	ssize_t ret;
> +
> +	guard(mutex)(&dsi->transfer_lock);
> +
> +	ret = vc4_dsi_transfer(dsi, msg);
> +	/* Only a running block can be reset and restored. */
> +	if (ret != -ETIMEDOUT || !dsi->enabled)
> +		return ret;
> +
> +	/*
> +	 * Leave the block usable for the next transfer. The message is not
> +	 * resent: it may already have reached the peripheral, and only the
> +	 * caller knows whether it is safe to repeat.
> +	 */
> +	disp0_ctrl = DSI_PORT_READ(DISP0_CTRL);
> +	dev_warn(&dsi->pdev->dev,
> +		 "transfer timed out (stat 0x%08x), resetting DSI\n",
> +		 DSI_PORT_READ(STAT));
> +	/*
> +	 * Keep the interrupt handler out until the block is restored and
> +	 * INT_STAT is cleared. The error interrupts are still enabled, and
> +	 * a handler running during the reset could complete the next
> +	 * transfer, or race with the register writes on DSI1 of the
> +	 * BCM2835, where both go through the DMA workaround. disable_irq()
> +	 * also waits for a late completion interrupt of the timed-out
> +	 * transfer.
> +	 */
> +	disable_irq(dsi->irq);
> +	vc4_dsi_reset_and_restore(dsi);
> +	DSI_PORT_WRITE(DISP0_CTRL, disp0_ctrl);
> +	enable_irq(dsi->irq);

[Severity: High]
Can a late or error interrupt race on dsi->reg_dma_mem in vc4_dsi_transfer()
before disable_irq(dsi->irq) is reached in vc4_dsi_host_transfer(),
corrupting DSI1_CTRL before vc4_dsi_reset_and_restore() saves and restores
it?

When wait_for_completion_timeout() times out in vc4_dsi_transfer(),
dsi->irq and INT_EN are still enabled while vc4_dsi_transfer() performs
five register writes before returning -ETIMEDOUT:

drivers/gpu/drm/vc4/vc4_dsi.c:vc4_dsi_transfer() {
	...
	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));

	if (ret)
		goto reset_fifo_and_return;
	...
reset_fifo_and_return:
	drm_err(drm, "DSI transfer failed, resetting: %d\n", ret);

	DSI_PORT_WRITE(TXPKT1C, DSI_PORT_READ(TXPKT1C) & ~DSI_TXPKT1C_CMD_EN);
	udelay(1);
	DSI_PORT_WRITE(CTRL,
		       DSI_PORT_READ(CTRL) |
		       DSI_PORT_BIT(CTRL_RESET_FIFOS));

	DSI_PORT_WRITE(TXPKT1C, 0);
	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
	return ret;
}

On BCM2835 DSI1 (where broken_axi_workaround is enabled), DSI_PORT_WRITE()
calls dsi_dma_workaround_write(), which writes the value into the single
shared buffer *dsi->reg_dma_mem and submits a DMA transfer from
dsi->reg_dma_paddr:

drivers/gpu/drm/vc4/vc4_dsi.c:dsi_dma_workaround_write() {
	...
	*dsi->reg_dma_mem = val;

	tx = chan->device->device_prep_dma_memcpy(chan,
						  dsi->reg_paddr + offset,
						  dsi->reg_dma_paddr,
						  4, 0);
	...
}

If the threaded handler vc4_dsi_irq_handler() runs concurrently (for
example on an LPRX/TA timeout error interrupt or a late completion
interrupt) and calls DSI_PORT_WRITE(INT_STAT, stat), it overwrites
*dsi->reg_dma_mem = stat without any lock.

If that happens during the read-modify-write of CTRL in
reset_fifo_and_return, the DMA transfer can write stat into DSI1_CTRL.
When vc4_dsi_reset_and_restore() then runs under disable_irq(dsi->irq), it
snapshots the corrupted register via u32 ctrl = DSI_PORT_READ(CTRL) and
writes it back at the end of the reset, leaving DSI1_CTRL corrupted (for
example with DSI1_CTRL_EN cleared so the block remains gated).

Should disable_irq(dsi->irq) be called in vc4_dsi_transfer() before the
post-timeout register writes (or before reset_fifo_and_return)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028164539.3471833-1-henryzhang62@gmail.com?part=5