Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/imx: dc-dprc: unsafe runtime PM and IRQ sync in dc_dprc_wrap_irq_handler() - [High] drm/imx: dc: race between dc_dprc_ctrl_done_handle() and DPRC disable - [Medium] drm/imx: dc: missing error handling and RPM cleanup in dc_dprc_configure() - [Medium] drm/imx: dc: unchecked device_link_add() and unbind race in dc_dprc_get() --
commit af166b9c0f4a6b977aa85b013620057bbb1169c8 Author: Liu Ying <[email protected]> drm/imx: dc: Add DPR channel support This commit adds a platform driver for the Display Prefetch Resolve Channel (DPRC) in the i.MX8 Display Controller, working alongside the PRG to fetch and transform display data into RTRAM. > diff --git a/drivers/gpu/drm/imx/dc/dc-dprc.c > b/drivers/gpu/drm/imx/dc/dc-dprc.c > new file mode 100644 > index 0000000000000..a8ddcd117a52b > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/dc-dprc.c [ ... ] > +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) > +{ > + unsigned int prg_stride = width * format->cpp[0]; > + unsigned int bpp = format->cpp[0] * 8; > + struct device *dev = dprc->dev; > + unsigned int p1_w, p1_h; > + 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; > + } > + > + dc_dprc_set_stream_id(dprc, stream_id); > + } [ ... ] > + switch (format->format) { > + case DRM_FORMAT_XRGB8888: > + /* > + * It turns out pixel components are mapped directly > + * without position change via DPR processing with > + * the following color component configurations. > + * Leave the pixel format to be handled by the > + * display controllers. > + */ > + val |= A_COMP_SEL(3) | R_COMP_SEL(2) | > + G_COMP_SEL(1) | B_COMP_SEL(0); > + val |= PIX_SIZE_32BIT; > + break; > + default: > + dev_err(dev, "unsupported format 0x%08x\n", format->format); > + return; > + } > + regmap_write(dprc->reg, MODE_CTRL0, val); > + > + if (start) { > + /* software shadow load for the first frame */ > + val = SW_SHADOW_LOAD_SEL | SHADOW_LOAD_EN; > + regmap_write(dprc->reg, SYSTEM_CTRL0, val); > + > + /* and then, run... */ > + val |= RUN_EN | REPEAT_EN; > + regmap_write(dprc->reg, SYSTEM_CTRL0, val); > + } > + > + dc_prg_configure(dprc->prg, width, height, prg_stride, bpp, baddr, > start); > + > + if (start) > + dc_dprc_enable(dprc); > + > + dc_dprc_reg_update(dprc); [Severity: Medium] Can dc_dprc_configure() leak the runtime PM reference on dprc->dev or perform unclocked register writes to dprc->prg->reg on error? When start is true, dc_dprc_configure() calls pm_runtime_resume_and_get(dev) before checking format->format. If the switch hits the default branch, it returns early without calling pm_runtime_put(dev) and before dc_prg_configure() acquires a matching runtime PM reference on dprc->prg->dev. In addition, dc_prg_configure() can fail in pm_runtime_resume_and_get() when start is true: drivers/gpu/drm/imx/dc/dc-prg.c:dc_prg_configure() { if (start) { ret = pm_runtime_resume_and_get(dev); if (ret < 0) { dev_err(dev, "failed to get RPM: %d\n", ret); return; } } } Because dc_prg_configure() returns void, dc_dprc_configure() still proceeds to call dc_dprc_enable() -> dc_prg_enable() and dc_dprc_reg_update() -> dc_prg_reg_update(), writing to prg->reg while prg->dev remains suspended and unclocked, and leaving the runtime PM reference counts unbalanced between dprc->dev and prg->dev. > + > + if (start) > + dc_dprc_enable_ctrl_done_irq(dprc); > + > + dev_dbg(dev, "w: %u, h: %u, s: %u, fmt: 0x%08x\n", > + width, height, stride, format->format); > +} > + > +void dc_dprc_disable_repeat_en(struct dc_dprc *dprc) > +{ > + regmap_write(dprc->reg, SYSTEM_CTRL0 + CLR, REPEAT_EN); > + dev_dbg(dprc->dev, "disable REPEAT_EN\n"); > +} > + > +void dc_dprc_disable(struct dc_dprc *dprc) > +{ > + dc_prg_disable(dprc->prg); > + > + pm_runtime_put(dprc->dev); > + > + dev_dbg(dprc->dev, "disable\n"); > +} > + > +void dc_dprc_disable_at_boot(struct dc_dprc *dprc) > +{ > + dc_prg_disable_at_boot(dprc->prg); > + > + clk_bulk_disable_unprepare(dprc->num_clks, dprc->clks); > + > + dev_dbg(dprc->dev, "disable at boot\n"); > +} > + > +static void dc_dprc_ctrl_done_handle(struct dc_dprc *dprc) > +{ > + regmap_write(dprc->reg, SYSTEM_CTRL0, REPEAT_EN); > + > + dc_prg_shadow_enable(dprc->prg); > + > + dev_dbg(dprc->dev, "CTRL done handle\n"); > +} [Severity: High] What prevents dc_dprc_wrap_irq_handler() -> dc_dprc_ctrl_done_handle() from racing with dc_dprc_disable_repeat_en() and dc_dprc_disable()? When dc_dprc_configure() runs with start set to true, it unmasks the control done interrupt via dc_dprc_enable_ctrl_done_irq(). If the plane or CRTC is subsequently disabled, neither dc_dprc_disable_repeat_en() nor dc_dprc_disable() masks that interrupt or synchronizes with dc_dprc_wrap_irq_handler(): 1. dc_dprc_disable_repeat_en() clears repeat_en in system_ctrl0 with local interrupts disabled, without taking dprc->lock or masking the control done interrupt. If the control done interrupt is already pending or running on another CPU (or in an interrupt thread on preempt_rt), dc_dprc_ctrl_done_handle() can run right after and write repeat_en back to system_ctrl0. 2. dc_dprc_disable() calls dc_prg_disable(dprc->prg), which puts the PRG into bypass mode and calls pm_runtime_put(prg->dev), and then calls asynchronous pm_runtime_put(dprc->dev). While dprc->dev is still runtime-active or an interrupt handler is in flight, dc_dprc_ctrl_done_handle() can call dc_prg_shadow_enable(dprc->prg), writing to prg->reg after prg->dev has already been disabled and had its runtime PM reference dropped. > + > +static irqreturn_t dc_dprc_wrap_irq_handler(int irq, void *data) > +{ > + struct dc_dprc *dprc = data; > + struct device *dev = dprc->dev; > + u32 mask = 0, status = 0; > + irqreturn_t result; > + > + pm_runtime_get_noresume(dev); > + if (!pm_runtime_active(dev)) { > + result = IRQ_NONE; > + goto out; > + } [ ... ] > + result = IRQ_HANDLED; > +out: > + pm_runtime_put_noidle(dev); > + return result; > +} [ ... ] > +struct dc_dprc *dc_dprc_get(struct device *dev, struct dc_dprc *dprc) > +{ > + struct dc_dprc *iter; > + > + guard(mutex)(&dc_dprc_list_mutex); > + list_for_each_entry(iter, &dc_dprc_list, list) { > + if (iter == dprc) { > + device_link_add(dev, dprc->dev, > + DL_FLAG_AUTOREMOVE_CONSUMER); > + return dprc; > + } > + } > + > + return NULL; > +} [Severity: Medium] Should dc_dprc_get() (and dc_prg_lookup_by_phandle() when called from dc_dprc_probe()) check whether device_link_add() returns NULL, and can dc_dprc_get() race with driver unbind? If device_link_add() fails and returns NULL, dc_dprc_get() and dc_prg_lookup_by_phandle() still return a valid pointer without establishing the consumer-supplier teardown link. Because dc_dprc_driver is registered before dc_prg_driver in dc_drivers[], platform_unregister_drivers() unbinds dc_prg_driver before dc_dprc_driver when no device link exists, so dc_dprc_remove() calling dc_prg_set_dprc(dprc->prg, NULL) would write to a freed dprc->prg. Furthermore, during supplier unbind in __device_release_driver(), device_links_busy() runs before dc_dprc_remove() acquires dc_dprc_list_mutex and removes dprc->list from dc_dprc_list. If a consumer calls dc_dprc_get() during that window, dprc is still on dc_dprc_list, device_link_add() creates the link after device_links_busy() has already passed, and dc_dprc_get() returns dprc right as it is being unbound and freed. > + > +static int dc_dprc_probe(struct platform_device *pdev) > +{ [ ... ] > + ret = devm_request_irq(dev, wrap_irq, dc_dprc_wrap_irq_handler, > + IRQF_SHARED, dev_name(dev), dprc); > + if (ret < 0) { > + dev_err(dev, "failed to request dpr_wrap IRQ(%d): %d\n", > + wrap_irq, ret); > + return ret; > + } > + > + dev_set_drvdata(dev, dprc); > + > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return dev_err_probe(dev, ret, "failed to enable PM runtime\n"); [ ... ] > +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] Could dc_dprc_wrap_irq_handler() access unclocked registers or leak the runtime PM active state when using pm_runtime_get_noresume() and pm_runtime_active()? There appear to be three issues with how runtime PM and the shared wrap_irq interact across dc_dprc_wrap_irq_handler(), dc_dprc_probe(), and dc_dprc_runtime_suspend(): 1. In include/linux/pm_runtime.h, pm_runtime_active(dev) evaluates to true whenever dev->power.disable_depth is non-zero: static inline bool pm_runtime_active(struct device *dev) { return dev->power.runtime_status == RPM_ACTIVE || dev->power.disable_depth; } Because dc_dprc_probe() registers the shared interrupt with devm_request_irq() before calling devm_pm_runtime_enable(dev), disable_depth is non-zero while the handler is live during probe and during reverse devres teardown (as well as during late system suspend). If another device sharing wrap_irq raises an interrupt during these windows, pm_runtime_active(dev) returns true and dc_dprc_wrap_irq_handler() reads dprc->reg while dprc->clks are disabled. 2. Even when runtime PM is enabled, pm_runtime_get_noresume() and pm_runtime_active() do not serialize with rpm_suspend() under dev->power.lock, and dc_dprc_runtime_suspend() disables dprc->clks without calling synchronize_irq(). An interrupt arriving after rpm_suspend() has checked usage_count == 0 can see runtime_status == RPM_ACTIVE and access MMIO registers while dc_dprc_runtime_suspend() turns off the clocks. 3. If dc_dprc_wrap_irq_handler() increments usage_count from 1 to 2 via pm_runtime_get_noresume() while dc_dprc_disable() concurrently calls pm_runtime_put(dprc->dev), dc_dprc_disable() drops usage_count from 2 to 1 without triggering idle/suspend. When dc_dprc_wrap_irq_handler() then exits via pm_runtime_put_noidle(dev), usage_count drops from 1 to 0 without running rpm_idle(), leaving the device stuck active with its clocks left on. Would it be safer to call devm_pm_runtime_enable() before devm_request_irq() in dc_dprc_probe(), use pm_runtime_get_if_active() paired with pm_runtime_put() in dc_dprc_wrap_irq_handler(), and synchronize the interrupt in dc_dprc_runtime_suspend()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
