Re: [PATCH] gpu: ipu-v3: fail PRG channel setup when buffer fill times out

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] A failed channel configuration sets `chan->used_pre = -1`, leading to an out-of-bounds array access in the VBLANK interrupt handler.
- [High] The error path fails to restore the out-parameter `*eba`, causing the caller to program the hardware with a disabled SRAM address.

Pre-existing issues:
- [High] Sleeping functions are called from within a DRM plane atomic update callback.
- [High] The return value of pm_runtime_get_sync() is ignored, risking a synchronous external abort.
--

commit 026bb13a574603516c5e8a7270cb349020d87261
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

gpu: ipu-v3: fail PRG channel setup when buffer fill times out

ipu_prg_channel_configure() waits for both PRG double buffers to become
ready, but ignores readl_poll_timeout() failures. A timeout is therefore
reported as a successful configuration and chan->enabled is set even
though the hardware did not report ready buffers. Propagate the timeout
error.

> diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
> index d38d3ba54d722..ff32f4ca57cb4 100644
> --- a/drivers/gpu/ipu-v3/ipu-prg.c
> +++ b/drivers/gpu/ipu-v3/ipu-prg.c
> @@ -327,10 +327,22 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
>  	writel(val, prg->regs + IPU_PRG_REG_UPDATE);
>  
>  	/* wait for both double buffers to be filled */
> -	readl_poll_timeout(prg->regs + IPU_PRG_STATUS, val,
> -			   (val & IPU_PRG_STATUS_BUFFER0_READY(prg_chan)) &&
> -			   (val & IPU_PRG_STATUS_BUFFER1_READY(prg_chan)),
> -			   5, 1000);
> +	ret = readl_poll_timeout(prg->regs + IPU_PRG_STATUS, val,
> +				 (val & IPU_PRG_STATUS_BUFFER0_READY(prg_chan)) &&
> +				 (val & IPU_PRG_STATUS_BUFFER1_READY(prg_chan)),
> +				 5, 1000);

[Severity: High]
This is a pre-existing issue, but does using readl_poll_timeout() here
cause us to sleep in an atomic context?

The macro internally uses usleep_range(), but ipu_prg_channel_configure()
is called from the DRM plane atomic update callback ipu_plane_atomic_update()
which runs in an atomic context where sleeping is forbidden.

Additionally, the pm_runtime_get_sync() call earlier in this function
can also sleep. Can we safely sleep here, or should this be avoided?

> +	if (ret) {
> +		val = readl(prg->regs + IPU_PRG_CTL);
> +		val |= IPU_PRG_CTL_BYPASS(prg_chan);
> +		writel(val, prg->regs + IPU_PRG_CTL);
> +
> +		val = IPU_PRG_REG_UPDATE_REG_UPDATE;
> +		writel(val, prg->regs + IPU_PRG_REG_UPDATE);
> +
> +		pm_runtime_put(prg->dev);

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
ipu_prg_channel_configure() we call pm_runtime_get_sync(prg->dev)
without checking its return value.

If the device fails to power on and we proceed to access unpowered PRG
MMIO registers like IPU_PRG_STRIDE, will it trigger a synchronous
external abort?

> +		ipu_prg_put_pre(prg, prg_chan);

[Severity: Critical]
Could this regression cause the call to ipu_prg_put_pre() to lead to an
out-of-bounds array access later?

When this fails, ipu_prg_put_pre() correctly releases the PRE but sets
chan->used_pre to -1. However, because the caller ipu_plane_atomic_update()
in ipuv3-plane.c ignores the error, ipu_state->use_pre remains true.

On the next VBLANK interrupt, ipu_plane_atomic_update_pending() blindly
delegates to the PRG:

ipu_plane_atomic_update_pending(...) {
    if (ipu_state->use_pre)
        return ipu_prg_channel_configure_pending(ipu_plane->ipu_ch);
}

Which then executes:

ipu_prg_channel_configure_pending(...) {
    ...
    return ipu_pre_update_pending(prg->pres[chan->used_pre]);
}

Will accessing prg->pres[-1] panic the kernel?

> +		return ret;

[Severity: High]
If we return on this error path, does this regression leave the
out-parameter *eba pointing to the now-disabled PRE SRAM?

In the caller ipu_plane_atomic_update() in ipuv3-plane.c, the return
value is ignored:

ipu_plane_atomic_update(...) {
    ...
    ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width,
                              height, fb->pitches[0],
                              fb->format->format, fb->modifier,
                              &eba);
    ...
    ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
}

Will this result in the IPU IDMAC fetching pixel data from a disabled
PRE SRAM buffer and potentially causing a bus hang?

> +	}
>  
>  	pm_runtime_put(prg->dev);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624143752.53943-1-pengpeng@iscas.ac.cn?part=1