Re: [PATCH v9 08/16] 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 in dc_prg_configure()
--

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

drm/imx: dc: Add PRG support

This commit adds a platform driver for the Display Prefetch Resolve Gasket
(PRG), which sits between a Display Prefetch Resolve Channel (DPRC) and a
FetchUnit as part of the prefetch engine.

> 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..b80ce5e641173
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-prg.c
> @@ -0,0 +1,317 @@

[ ... ]

> +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 v8 [1], in drivers/gpu/drm/imx/dc/dc-prg.c, what
happens if pm_runtime_resume_and_get() fails in dc_prg_configure() when start
is true?

Because dc_prg_configure() returns void, the error is not propagated to the
caller and the PRG device remains runtime-suspended with its clocks disabled
in dc_prg_runtime_resume().

Callers that proceed after dc_prg_configure() will then call dc_prg_enable(),
dc_prg_reg_update(), or dc_prg_shadow_enable(), performing MMIO writes to
PRG_CTRL and PRG_REG_UPDATE while the PRG clocks are off. Subsequent updates
with start set to false will also write to the PRG registers without clocks
enabled.

Furthermore, when the plane or CRTC is later disabled, dc_prg_disable() will
write to PRG_CTRL while unclocked and call 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 runtime PM active
state so subsequent register accesses and pm_runtime_put() are skipped when
resume fails?

[1] https://lore.kernel.org/r/20261030-imx8-dc-prefetch-v8-7-c4f81afd4533@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-imx8-dc-prefetch-v9-0-3b985e7636e2@nxp.com?part=8