Re: [PATCH v9 09/16] drm/imx: dc: Add DPR channel support

[sashiko-bot@kernel.org]

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 <victor.liu@nxp.com>

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/20261030-imx8-dc-prefetch-v8-8-c4f81afd4533@nxp.com
[2] https://lore.kernel.org/r/20261030-imx8-dc-prefetch-v8-8-c4f81afd4533@nxp.com
[3] https://lore.kernel.org/r/20261030-imx8-dc-prefetch-v8-8-c4f81afd4533@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-imx8-dc-prefetch-v9-0-3b985e7636e2@nxp.com?part=9