Re: [PATCH v6 06/13] drm/imx: dc: Add DPR channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Shared IRQ handler accesses hardware registers without checking active state and unconditionally returns IRQ_HANDLED.
- [High] Missing synchronize_irq() in runtime suspend for shared IRQ.
- [High] Probe error paths leave a dangling pointer in the PRG component.
- [High] Error paths in dc_dprc_configure cause severe PM usage count imbalances and leaks.
- [High] IRQ_MASK_STATUS is omitted from regmap write ranges, causing IRQ clear to fail and leading to an IRQ storm.
- [Medium] PRG component lookup returns an unprotected pointer and lacks a device link, risking concurrent use-after-free.
- [Medium] Probe fails to propagate -EPROBE_DEFER from platform_get_irq_byname.
- [Medium] DPU_DRPC_MAX_STRIDE allows 0x10000 which overflows the 16-bit PITCH field.
--

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

drm/imx: dc: Add DPR channel support
    
Display Prefetch Resolve Channel(DPRC) is a part of a prefetch engine.
It fetches display data, transforms it to linear format and stores it
to DPRC's RTRAM. Add a platform driver to support the DPRC.

> diff --git a/drivers/gpu/drm/imx/dc/dc-dprc.c b/drivers/gpu/drm/imx/dc/dc-dprc.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-dprc.c
[ ... ]
> +static const struct regmap_range dc_dprc_regmap_write_ranges[] = {
> +	regmap_reg_range(SYSTEM_CTRL0, SYSTEM_CTRL0 + TOG),
> +	regmap_reg_range(IRQ_MASK, IRQ_MASK + TOG),
> +	regmap_reg_range(IRQ_NONMASK_STATUS, MODE_CTRL0 + TOG),

[Severity: High]
Does this missing regmap range cause an IRQ storm? The write ranges cover 
0x20-0x2C and 0x40-0x5C, but IRQ_MASK_STATUS is at 0x30. When the IRQ 
handler tries to clear the interrupt by writing to IRQ_MASK_STATUS, won't 
regmap reject the write and leave the level-triggered interrupt asserted?

[ ... ]
> +void dc_dprc_configure(struct dc_dprc *dprc, unsigned int stream_id,
> +		       unsigned int width, unsigned int height,
> +		       unsigned int stride,
> +		       const struct drm_format_info *format,
> +		       dma_addr_t baddr, bool start)
> +{
[ ... ]
> +	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]
If pm_runtime_resume_and_get() fails, we return early here without taking 
a PM reference. Since the caller has no way to detect this failure (the 
function returns void), won't the eventual teardown call to dc_dprc_disable() 
unconditionally invoke pm_runtime_put() and cause a usage counter underflow?

[ ... ]
> +	switch (format->format) {
[ ... ]
> +	default:
> +		dev_err(dev, "unsupported format 0x%08x\n", format->format);
> +		return;
> +	}

[Severity: High]
If an unsupported format is provided, this returns early while still holding 
the DPRC PM reference and bypasses the PRG setup. When dc_dprc_disable() is 
called later, it will invoke dc_prg_disable() which puts the PRG PM reference 
(causing an underflow there), while the DPRC PM reference could end up leaking. 
Should these error paths be handled to keep PM counts balanced?

[ ... ]
> +static irqreturn_t dc_dprc_wrap_irq_handler(int irq, void *data)
> +{
> +	struct dc_dprc *dprc = data;
> +	struct device *dev = dprc->dev;
> +	u32 mask, status;
> +
> +	scoped_guard(spinlock, &dprc->lock) {
> +		/* cache valid IRQ status */
> +		regmap_read(dprc->reg, IRQ_MASK, &mask);

[Severity: High]
Since this is registered as a shared IRQ, could it fire while this device is 
runtime suspended and its clocks are off? If so, reading from the registers 
without first checking pm_runtime_get_if_active() could cause an external 
abort or bus hang.

[ ... ]
> +		/* clear status register */
> +		regmap_write(dprc->reg, IRQ_MASK_STATUS, status);
> +	}
[ ... ]
> +	if (status & IRQ_DPR_CRTL_DONE)
> +		dc_dprc_ctrl_done_handle(dprc);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Does this unconditionally return IRQ_HANDLED even when the interrupt wasn't 
for this device? If the read status was 0, returning IRQ_HANDLED masks 
spurious interrupts and violates shared IRQ conventions.

[ ... ]
> +bool dc_dprc_stride_supported(struct dc_dprc *dprc,
> +			      unsigned int stride, unsigned int width,
> +			      const struct drm_format_info *format,
> +			      dma_addr_t baddr)
> +{
> +	unsigned int prg_stride = width * format->cpp[0];
> +
> +	if (stride > DPU_DRPC_MAX_STRIDE)
> +		return false;

[Severity: Medium]
Since DPU_DRPC_MAX_STRIDE is defined as 0x10000, this permits a stride of 
0x10000. However, the PITCH macro used in dc_dprc_configure() uses 
GENMASK(31, 16) which is a 16-bit field that only holds up to 0xFFFF. 
Won't a stride of 0x10000 silently truncate to 0 when written to the hardware?

[ ... ]
> +static int dc_dprc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	wrap_irq = platform_get_irq_byname(pdev, "dpr_wrap");
> +	if (wrap_irq < 0)
> +		return -ENODEV;

[Severity: Medium]
Can this mask deferred probing? If platform_get_irq_byname() returns 
-EPROBE_DEFER because the interrupt controller isn't ready yet, overriding 
it with -ENODEV will prevent the driver from loading properly.

[ ... ]
> +	dprc->prg = dc_prg_lookup_by_phandle(dev, "fsl,prgs", 0);
> +	if (!dprc->prg)
> +		return dev_err_probe(dev, -EPROBE_DEFER,
> +				     "failed to lookup PRG\n");
> +
> +	dc_prg_set_dprc(dprc->prg, dprc);

[Severity: High]
If the probe function fails in a subsequent step (like devm_request_irq or 
devm_pm_runtime_enable), the devres cleanup will free the dprc struct. 
However, won't this leave a dangling pointer in the PRG component since 
dc_prg_set_dprc(dprc->prg, NULL) is never called on the error paths? This 
could lead to a use-after-free later.

[Severity: Medium]
Additionally, dc_prg_lookup_by_phandle() locates the PRG in a global list 
and returns a raw pointer without incrementing the device reference count 
or creating a device link. If the PRG device is dynamically unbound (e.g., 
via a DT overlay removal) after this lookup, couldn't this unprotected 
pointer become a use-after-free risk?

[ ... ]
> +static int dc_dprc_runtime_suspend(struct device *dev)
> +{
> +	struct dc_dprc *dprc = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(dprc->num_clks, dprc->clks);
> +
> +	return 0;
> +}

[Severity: High]
Because the IRQ is shared (IRQF_SHARED), it could be triggered concurrently 
by another device. If the handler is executing mid-flight on another CPU, 
it might attempt to read registers just as the clocks are turned off here, 
potentially causing a system crash. Should synchronize_irq() be called 
before powering down the hardware to ensure no handler is executing?

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