Re: [PATCH v3 4/4] 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: synchronize_irq() called before masking INT_EN on timeout
--

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

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

When a DSI transfer times out while the block is enabled, this commit resets
the DSI block and its analog front end and restores the hardware
configuration from pre_enable so subsequent transfers can succeed without
requiring a full modeset.

> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index ea3a6d06b045a..42f50d2ca401c 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c

[ ... ]

> @@ -1374,6 +1378,114 @@ 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);
> +	u32 afec0 = DSI_PORT_READ(PHY_AFEC0);
> +	u32 afec1 = DSI_PORT_READ(PHY_AFEC1);
> +	u32 disp0 = DSI_PORT_READ(DISP0_CTRL) & ~DSI_DISP0_ENABLE;
> +	u32 disp1 = DSI_PORT_READ(DISP1_CTRL);
> +	u32 hstx_to = DSI_PORT_READ(HSTX_TO_CNT);
> +	u32 lprx_to = DSI_PORT_READ(LPRX_TO_CNT);
> +	u32 ta_to = DSI_PORT_READ(TA_TO_CNT);
> +	u32 pr_to = DSI_PORT_READ(PR_TO_CNT);
> +	u32 phyc = DSI_PORT_READ(PHYC);
> +	u32 clt0 = DSI_PORT_READ(HS_CLT0);
> +	u32 clt1 = DSI_PORT_READ(HS_CLT1);
> +	u32 clt2 = DSI_PORT_READ(HS_CLT2);
> +	u32 dlt3 = DSI_PORT_READ(HS_DLT3);
> +	u32 dlt4 = DSI_PORT_READ(HS_DLT4);
> +	u32 dlt5 = DSI_PORT_READ(HS_DLT5);
> +	u32 dlt6 = DSI_PORT_READ(HS_DLT6);
> +	u32 dlt7 = DSI_PORT_READ(HS_DLT7);
> +
> +	/*
> +	 * Mask the interrupts while the block and its AFE are in reset: the
> +	 * error interrupts would otherwise fire continuously.
> +	 */
> +	DSI_PORT_WRITE(INT_EN, 0);

[ ... ]

> +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));
> +	/*
> +	 * The completion interrupt of the timed-out transfer may still be
> +	 * in flight; let it finish so that it cannot complete the next one.
> +	 */
> +	synchronize_irq(dsi->irq);
> +	vc4_dsi_reset_and_restore(dsi);

[Severity: High]
Should INT_EN be masked before calling synchronize_irq(dsi->irq) in
vc4_dsi_host_transfer() in drivers/gpu/drm/vc4/vc4_dsi.c (or should
synchronize_irq(dsi->irq) be called after DSI_PORT_WRITE(INT_EN, 0) in
vc4_dsi_reset_and_restore())?

When vc4_dsi_transfer() returns -ETIMEDOUT via reset_fifo_and_return, it
leaves INTERRUPTS_ALWAYS_ENABLED active in INT_EN and does not clear INT_STAT:

drivers/gpu/drm/vc4/vc4_dsi.c:vc4_dsi_transfer() {
	...
reset_fifo_and_return:
	...
	DSI_PORT_WRITE(TXPKT1C, 0);
	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
	return ret;
}

Because synchronize_irq(dsi->irq) in vc4_dsi_host_transfer() runs before
vc4_dsi_reset_and_restore() performs 18 register reads and writes
DSI_PORT_WRITE(INT_EN, 0), any error or timeout interrupt from
INTERRUPTS_ALWAYS_ENABLED can still fire in the window after synchronize_irq()
returns and before INT_EN is cleared.

If an interrupt fires in that window on BCM2835 DSI1 (where
broken_axi_workaround is enabled and devm_request_threaded_irq() is used with
vc4_dsi_irq_defer_to_thread_handler()), the threaded handler
vc4_dsi_irq_handler() will run concurrently with vc4_dsi_reset_and_restore():

drivers/gpu/drm/vc4/vc4_dsi.c:vc4_dsi_irq_handler() {
	u32 stat = DSI_PORT_READ(INT_STAT);
	...
	DSI_PORT_WRITE(INT_STAT, stat);
}

1. In vc4_dsi_irq_handler(), DSI_PORT_WRITE(INT_STAT, stat) invokes
dsi_dma_workaround_write(), which writes to the shared 4-byte buffer
*dsi->reg_dma_mem and submits a DMA transfer on dsi->reg_dma_chan without
serialization:

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);
	...
}

Can this race with the register writes in vc4_dsi_reset_and_restore() and
corrupt the values being written to the DSI registers?

2. While vc4_dsi_reset_and_restore() holds the block and AFE in reset across
mdelay(1), error bits in INT_STAT assert continuously. Since
vc4_dsi_irq_handler() reads raw INT_STAT without masking by INT_EN, if the
concurrent IRQ thread reads INT_STAT while INT_HSTX_TO is asserted during
reset (or if a late completion bit latched before TXPKT1C_CMD_EN was cleared),
won't it call vc4_dsi_xfer_complete() and signal dsi->xfer_completion after
synchronize_irq() has already returned, leaving a stale completion for the
next transfer?

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