Re: [PATCH v8 07/15] drm/imx: dc: Add PRG support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/imx: dc-prg: unclocked MMIO and RPM underflow on dc_prg_configure() error
--

commit dc9803b183fdde7606d3c14739f864581b99c2a3
Author: Liu Ying <victor.liu@nxp.com>

drm/imx: dc: Add PRG support

This commit adds a platform driver to support the Display Prefetch Resolve
Gasket (PRG), which sits between a Display Prefetch Resolve Channel (DPRC)
and a FetchUnit.

> diff --git a/drivers/gpu/drm/imx/dc/dc-prg.c b/drivers/gpu/drm/imx/dc/dc-prg.c
> new file mode 100644
> index 0000000000000..46108f7e09af3
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-prg.c
[ ... ]
> +void dc_prg_enable(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_CTRL + CLR, BYPASS);
> +}
> +
> +void dc_prg_disable(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_CTRL, BYPASS);
> +
> +	pm_runtime_put(prg->dev);
> +}
[ ... ]
> +void dc_prg_configure(struct dc_prg *prg,
> +		      unsigned int width, unsigned int height,
> +		      unsigned int stride, unsigned int bits_per_pixel,
> +		      dma_addr_t baddr, bool start)
> +{
> +	struct device *dev = prg->dev;
> +	unsigned int burst_size;
> +	u32 val;
> +	int ret;
> +
> +	if (start) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to get RPM: %d\n", ret);
> +			return;
> +		}
> +	}

[Severity: High]
As reported by Sashiko on v7 [1], what happens if pm_runtime_resume_and_get()
fails in dc_prg_configure() in drivers/gpu/drm/imx/dc/dc-prg.c when start is
true?

Because dc_prg_configure() returns void, the caller is not notified that the
PRG device remains runtime-suspended with its clocks disabled. Subsequent
calls to dc_prg_enable(), dc_prg_reg_update(), dc_prg_shadow_enable(), or
later dc_prg_configure() updates with start set to false will perform MMIO
writes to PRG registers while the hardware is unclocked and unpowered, which
can trigger a bus abort on i.MX8 SoCs.

Additionally, when the plane or CRTC is later disabled, dc_prg_disable()
writes to PRG_CTRL while unclocked and unconditionally calls
pm_runtime_put(prg->dev), underflowing the runtime PM usage counter on
prg->dev.

Should dc_prg_configure() return an error code or track the active state so
callers do not access unclocked registers or drop an unacquired runtime PM
reference?

[1] https://lore.kernel.org/r/20261026-imx8-dc-prefetch-v7-7-a0b50cc4fc65@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-imx8-dc-prefetch-v8-0-c4f81afd4533@nxp.com?part=7