Re: [PATCH v6 05/13] drm/imx: dc: Add PRG support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Sleeping functions called from atomic context in hardware programming path
- [High] Undefined behavior and potential integer overflow in burst size calculation.
- [High] Use-After-Free risk when returning an un-refcounted device pointer from lookup.
- [Medium] Clock underflow warning triggered by unconditional clock disable at boot.
- [Low] Uninitialized __free() variable declaration violates cleanup guidelines.
--

commit 9751a7be8ad4e06c86087a3daf3b43e0c5ce886d
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) for the i.MX display controller. It introduces the PRG
initialization, power management, clock handling, and configuration
functions.

> 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..111ff026eec89
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-prg.c

[ ... ]

> +void dc_prg_disable_at_boot(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_CTRL, BYPASS);
> +
> +	clk_bulk_disable_unprepare(prg->num_clks, prg->clks);

[Severity: Medium]
Will this unconditionally disabled clock trigger a clock underflow warning
during boot?

If the bootloader leaves the display on, paths like dc_crtc_reset call
dc_prg_disable_at_boot. Since the clocks were just acquired during probe
with devm_clk_bulk_get_all, they haven't been prepared or enabled by this
driver yet. Calling clk_bulk_disable_unprepare without a matching enable
can cause a WARN_ON in the Common Clock Framework.

> +}
> +
> +static unsigned int dc_prg_burst_size_fixup(dma_addr_t baddr)
> +{
> +	unsigned int burst_size;
> +
> +	burst_size = 1 << __ffs(baddr);

[Severity: High]
Could this bit shift result in undefined behavior or an integer overflow?

If baddr is 0, which might happen if a buffer is allocated at IOVA 0 by
the IOMMU, __ffs(0) leads to undefined behavior. Additionally, if the
lowest set bit of baddr is 31 (like 0x80000000), 1 << 31 will overflow a
signed 32-bit integer.

[ ... ]

> +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);

[Severity: High]
Is it safe to call sleeping functions from this execution path?

This function appears to be reachable from the plane atomic update
callback via dc_dprc_configure when start is true. Plane atomic updates
are executed in atomic context during non-blocking commits, but
pm_runtime_resume_and_get is a synchronous operation that can sleep.

Additionally, the runtime resume callback dc_prg_runtime_resume invokes
dc_prg_reset, which calls fsleep(10). Sleeping in an atomic path can
cause system instability, lockdep warnings, or deadlocks.

[ ... ]

> +struct dc_prg *
> +dc_prg_lookup_by_phandle(struct device *dev, const char *name, int index)
> +{
> +	struct device_node *prg_node __free(device_node);

[Severity: Low]
Does this uninitialized variable declaration violate the cleanup subsystem
guidelines?

According to include/linux/cleanup.h and subsystem rules, variables
declared with __free should be defined and initialized in a single
statement. Keeping it uninitialized increases the risk of uninitialized
pointer dereferences if early returns are added later.

> +	struct dc_prg *prg;
> +
> +	prg_node = of_parse_phandle(dev->of_node, name, index);
> +	if (!prg_node)
> +		return NULL;
> +
> +	guard(mutex)(&dc_prg_list_mutex);
> +	list_for_each_entry(prg, &dc_prg_list, list) {
> +		if (prg_node == prg->dev->of_node)
> +			return prg;

[Severity: High]
Does returning the raw pointer here create a use-after-free risk?

The returned device pointer is not pinned with a reference count
(like with get_device). After dropping dc_prg_list_mutex and returning,
the device could be concurrently unbound. This would cause dc_prg_remove
to free the object, leaving the caller holding a dangling pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-imx8-dc-prefetch-v6-0-26643e3f6cb7@nxp.com?part=5