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 <[email protected]> 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/[email protected]?part=6
