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