Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/imx: dc-dprc: invalid wait context calling dc_prg_shadow_enable() in IRQ - [High] drm/imx: dc-dprc: runtime PM race and leak in dc_dprc_wrap_irq_handler() - [Medium] drm/imx: dc-dprc: RPM reference leak and PRG imbalance in dc_dprc_configure() - [Medium] drm/imx: dc-dprc: error IRQs left masked on restart before runtime suspend --
commit cd34e5bff983d01a81bde212b10cde47c6bbe04b 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 and links it with the PRG driver. [Severity: High] As reported by Sashiko on v8 [1], in drivers/gpu/drm/imx/dc/dc-dprc.c at line 243, can dc_dprc_configure() leak a runtime PM reference and leave PRG runtime PM unbalanced when given an unsupported format or when PRG runtime resume fails? When dc_dprc_configure() is called with start set to true, it calls pm_runtime_resume_and_get(dev) at line 207 and writes hardware registers before checking format->format in the switch statement at line 230. If the format hits the default branch at line 243, the function logs an error and returns early without calling pm_runtime_put(dev) and before calling dc_prg_configure(). That leaks the runtime PM reference on dprc->dev and leaves dprc->prg->dev without a matching reference if dc_dprc_disable() is later called. Similarly, at line 259, dc_dprc_configure() calls dc_prg_configure(). In drivers/gpu/drm/imx/dc/dc-prg.c at line 137, if start is true and pm_runtime_resume_and_get() fails on prg->dev, dc_prg_configure() returns void without reporting the error. dc_dprc_configure() then unconditionally calls dc_dprc_enable() at line 262 and dc_dprc_reg_update() at line 264, which write to prg->reg while PRG clocks are still disabled. Could format validation be done before pm_runtime_resume_and_get(), and could dc_prg_configure() return an error code so dc_dprc_configure() can unwind the dprc->dev runtime PM reference on failure? [Severity: Medium] In drivers/gpu/drm/imx/dc/dc-dprc.c at line 276, does dc_dprc_disable_repeat_en() leave the hardware error interrupts masked if the channel is restarted before runtime suspend runs? In dc_dprc_runtime_resume() at line 507, the driver resets the hardware with dc_dprc_reset() and writes IRQ_CTRL_MASK to IRQ_MASK at line 511, which masks the three control interrupts while unmasking the five error interrupts. When stopping the channel, dc_dprc_disable_repeat_en() at line 276 sets IRQ_ALL_MASK in IRQ_MASK, masking both the control and error interrupts. Then dc_dprc_disable() at line 292 calls pm_runtime_put(dprc->dev), which schedules an asynchronous suspend on pm_wq. If dc_dprc_configure() is called again with start set to true before the asynchronous runtime suspend callback executes, pm_runtime_resume_and_get() at line 207 sees the device is already active and returns immediately without running dc_dprc_runtime_resume(). Because dc_dprc_configure() only unmasks IRQ_DPR_CRTL_DONE via dc_dprc_enable_ctrl_done_irq() at line 190, won't the five error interrupts remain masked and dc_dprc_reset() be skipped for the new stream? [Severity: High] As reported by Sashiko on v8 [2], in drivers/gpu/drm/imx/dc/dc-dprc.c at line 310, does calling dc_prg_shadow_enable() from dc_dprc_wrap_irq_handler() trigger an invalid wait context on PREEMPT_RT or lockdep? In dc_dprc_probe() at line 463, devm_request_irq() requests wrap_irq with IRQF_SHARED | IRQF_NO_THREAD, so dc_dprc_wrap_irq_handler() always runs in hardirq context. When IRQ_DPR_CRTL_DONE is handled at line 362, the call chain dc_dprc_wrap_irq_handler() -> dc_dprc_ctrl_done_handle() -> dc_prg_shadow_enable() writes to prg->reg using regmap_write(). Unlike dc_dprc_regmap_config, dc_prg_regmap_config in drivers/gpu/drm/imx/dc/dc-prg.c at line 71 sets fast_io to true but does not set use_raw_spinlock to true. As a result, prg->reg uses a standard spinlock_t, which becomes a sleepable lock on PREEMPT_RT and violates lockdep's raw lock nesting rules when acquired from an IRQF_NO_THREAD hardirq handler (as well as from dc_dprc_configure() when called with local IRQs and preemption disabled). Should dc_prg_regmap_config also set use_raw_spinlock to true? [Severity: High] As reported by Sashiko on v8 [3], in drivers/gpu/drm/imx/dc/dc-dprc.c at line 322, can the runtime PM handling in dc_dprc_wrap_irq_handler() race with dc_dprc_disable() and dc_dprc_runtime_suspend()? Because wrap_irq is requested with IRQF_SHARED at line 463, another DPR channel sharing the interrupt line can invoke dc_dprc_wrap_irq_handler() at any time, even after this channel has masked its own interrupts in dc_dprc_disable_repeat_en(). This creates three issues with the pm_runtime_get_noresume() / pm_runtime_active() / pm_runtime_put_noidle() pattern at lines 322 and 366: 1. If a shared interrupt runs dc_dprc_wrap_irq_handler() and increments usage_count via pm_runtime_get_noresume() at line 322 while dc_dprc_disable() at line 292 calls pm_runtime_put() (or while the async runtime suspend worker is checking usage_count), the runtime PM core sees usage_count > 0 and aborts the idle/suspend transition. When the IRQ handler then drops its reference with pm_runtime_put_noidle() at line 366, no new idle or suspend request is queued, leaving dprc->dev active with its clocks enabled indefinitely. 2. In rpm_suspend(), usage_count is checked before the status changes from RPM_ACTIVE to RPM_SUSPENDING and before dc_dprc_runtime_suspend() is called at line 487. If a shared interrupt fires on another CPU after the usage_count check passes, pm_runtime_get_noresume() does not stop the ongoing suspend and pm_runtime_active() still returns true until the status is updated, allowing the handler to read IRQ_MASK and IRQ_MASK_STATUS while dc_dprc_runtime_suspend() is disabling dprc->clks. 3. pm_runtime_active(dev) returns true whenever dev->power.disable_depth is non-zero, even if the device is runtime-suspended with its clocks off (for example, during system suspend after __device_suspend_late() disables runtime PM and before noirq suspend disables interrupts). A shared interrupt during that window will see pm_runtime_active(dev) as true and read dprc->reg while clocks are disabled. Could the driver track whether clocks are enabled under dprc->lock in the runtime PM callbacks instead of relying on pm_runtime_get_noresume() and pm_runtime_active() in the hardirq handler? [1] https://lore.kernel.org/r/[email protected] [2] https://lore.kernel.org/r/[email protected] [3] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
