Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/bridge: analogix_dp: level IRQ storm when pm_runtime_resume_and_get() fails - [Medium] drm/bridge: analogix_dp: pm_runtime_resume_and_get() in IRQ clears HPD status --
commit 2a41fb565a5b1a6da39d3e4bf4efd7dfc99b3b81 Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Move IRQ enable/disable to bind/unbind This commit moves enable_irq() and disable_irq() from the bridge enable/disable handlers into analogix_dp_bind() and analogix_dp_unbind(). It also converts the interrupt handler to a pure threaded IRQ handler that acquires a runtime PM reference before accessing controller registers. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index a8a2d5d615aea..e92d86c678947 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -700,24 +700,22 @@ static int analogix_dp_enable_scramble(struct > analogix_dp_device *dp, > return ret < 0 ? ret : 0; > } > > -static irqreturn_t analogix_dp_hardirq(int irq, void *arg) > -{ > - struct analogix_dp_device *dp = arg; > - irqreturn_t ret = IRQ_NONE; > - u32 irq_type; > - > - irq_type = analogix_dp_get_irq_type(dp); > - if (irq_type) > - ret = IRQ_WAKE_THREAD; > - > - return ret; > -} > - > static irqreturn_t analogix_dp_irq_thread(int irq, void *arg) > { > struct analogix_dp_device *dp = arg; > u32 irq_type; > bool hpd_detected; > + int ret; > + > + /* > + * The device may have been runtime suspended since the interrupt > + * was raised. Hold a runtime PM reference before touching the > + * registers, as the controller clocks are only guaranteed to be > + * enabled while the device is runtime PM active. > + */ > + ret = pm_runtime_resume_and_get(dp->dev); > + if (ret) > + return IRQ_HANDLED; [Severity: High] Can returning IRQ_HANDLED when pm_runtime_resume_and_get() fails cause a level-triggered IRQ storm or unclocked MMIO access during system suspend and resume? On Rockchip and Exynos platforms, the native DP controller interrupt is a level-high interrupt (IRQ_TYPE_LEVEL_HIGH) requested with IRQF_ONESHOT. Moving disable_irq() from analogix_dp_bridge_disable() to analogix_dp_unbind() leaves dp->irq enabled across system suspend and resume, where rockchip_dp_pm_ops and exynos_dp_pm_ops use pm_runtime_force_suspend() in dpm_suspend and pm_runtime_force_resume() in dpm_resume (outside the dpm_suspend_noirq and dpm_resume_noirq window where device IRQs are masked). If an interrupt fires when dp->dev is runtime-suspended and dev->power.disable_depth > 0 (or if analogix_dp_resume() fails at runtime), pm_runtime_resume_and_get() returns a negative error code such as -EACCES. Returning IRQ_HANDLED here without clearing or masking the hardware interrupt status registers causes irq_thread_fn() to increment desc->threads_handled and irq_finalize_oneshot() to unmask the still-asserted level IRQ line. That immediately re-triggers the interrupt while preventing note_interrupt() in the spurious IRQ detector from ever disabling the screaming line. Furthermore, if dev->power.usage_count > 1 during pm_runtime_force_suspend() (such as when a later commit in this series, "drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection", holds a runtime PM reference in analogix_dp_bind(), or when runtime PM is forbidden via sysfs), pm_runtime_force_suspend() disables the DP clocks via analogix_dp_suspend() while leaving runtime_status and last_status set to RPM_ACTIVE with disable_depth > 0. In that state, rpm_resume() returns 1 and pm_runtime_resume_and_get() returns 0, allowing analogix_dp_irq_thread() to perform MMIO accesses on an unclocked controller. > > irq_type = analogix_dp_get_irq_type(dp); > if (!dp->hpd_gpiod && irq_type) [Severity: Medium] If the device has actually runtime suspended before analogix_dp_irq_thread() runs, won't pm_runtime_resume_and_get() wipe the pending interrupt status registers before analogix_dp_get_irq_type() can read them? When dp->dev is runtime suspended, pm_runtime_resume_and_get() invokes the runtime resume callback (rockchip_dp_resume() or exynos_dp_resume() -> analogix_dp_resume()): drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_resume() { ... if (dp->plat_data->power_on) dp->plat_data->power_on(dp->plat_data); phy_set_mode(dp->phy, PHY_MODE_DP); phy_power_on(dp->phy); analogix_dp_init_dp(dp); ... } On Rockchip, dp->plat_data->power_on() calls rockchip_dp_poweron() -> rockchip_dp_pre_init(), which asserts and deasserts dp->rst and dp->apbrst, wiping all controller registers. On all platforms, analogix_dp_init_dp() then resets the controller and clears all pending interrupts: drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_init_dp() { analogix_dp_reset(dp); analogix_dp_swreset(dp); analogix_dp_init_analog_param(dp); analogix_dp_init_interrupt(dp); ... analogix_dp_init_hpd(dp); analogix_dp_init_aux(dp); } Both analogix_dp_init_interrupt() and analogix_dp_init_hpd() explicitly clear ANALOGIX_DP_COMMON_INT_STA_4 and ANALOGIX_DP_INT_STA. By the time pm_runtime_resume_and_get() returns to analogix_dp_irq_thread(), analogix_dp_get_irq_type() reads cleared registers (0), so irq_type is 0 and the hotplug event is silently dropped. Note that the later commit in this series ("drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection") only keeps runtime PM active across bind/unbind on RK3576_EDP and RK3588_EDP, leaving EXYNOS_DP, RK3288_DP, RK3399_EDP, and other configurations susceptible to runtime suspend while the IRQ is enabled. Conversely, when dp->hpd_gpiod is used, analogix_dp_get_irq_type() (as well as analogix_dp_get_plug_in_status() in commit "drm/bridge: analogix_dp: Declare DRM_BRIDGE_OP_HPD for hotplug reporting") only reads the GPIO level via gpiod_get_value(dp->hpd_gpiod) and never touches DP controller MMIO registers. Does analogix_dp_irq_thread() need to call pm_runtime_resume_and_get() when dp->hpd_gpiod is present, since doing so powers on, resets, and reinitializes the DP controller and PHY on every GPIO HPD interrupt while the display is off? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
