Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Selective unmasking combined with unconditional status clearing permanently masks concurrently triggered hotplug interrupts, causing lost events. - [High] Accessing DP controller mask registers in mute/unmute functions when using GPIO HPD causes a bus hang during runtime suspend. --
--- Patch [3]: [PATCH v1 3/7] drm/bridge: analogix_dp: Extend mute/unmute HPD interrupts to accept irq bitmask --- commit 09d81009e86378eb20f782e0e10f313daaa0f8d2 Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Extend mute/unmute HPD interrupts to accept irq bitmask This commit updates the global HPD interrupt mute/unmute routines to accept an IRQ bitmask parameter. It passes the bitmask from the hardirq and thread handlers to selectively mask/unmask flags triggered by the current IRQ event, preparing for subsequent accurate HPD event handling. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index ccb8a374b40b7..a3dc5eac91b8f 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c [ ... ] > @@ -734,7 +734,7 @@ static irqreturn_t analogix_dp_irq_thread(int irq, void > *arg) > > if (irq_type) { > analogix_dp_clear_hotplug_interrupts(dp); > - analogix_dp_unmute_hpd_interrupt(dp); > + analogix_dp_unmute_hpd_interrupt(dp, irq_type); [Severity: High] Could this selective unmasking combined with the unconditional status clearing permanently mask concurrently triggered hotplug interrupts? Since analogix_dp_irq_thread() samples irq_type earlier via analogix_dp_get_irq_type(), what happens if a new native hotplug interrupt (like HOTPLUG_CHG) fires immediately after that read? The hardirq would preempt the thread, read the updated status, and mask the new interrupt. When the thread resumes, analogix_dp_clear_hotplug_interrupts() unconditionally clears all hotplug status bits: reg = HOTPLUG_CHG | HPD_LOST | PLUG; writel(reg, dp->reg_base + ANALOGIX_DP_COMMON_INT_STA_4); destroying the new interrupt's status. Then, this selective analogix_dp_unmute_hpd_interrupt() call uses the stale irq_type snapshot that lacks the new interrupt, leaving the newly triggered interrupt permanently masked in ANALOGIX_DP_COMMON_INT_MASK_4 and lost until a driver reload. > } > > return IRQ_HANDLED; > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > index f4f859cb29366..42c1da160ad02 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c [ ... ] > -void analogix_dp_mute_hpd_interrupt(struct analogix_dp_device *dp) > +void analogix_dp_mute_hpd_interrupt(struct analogix_dp_device *dp, u32 > irq_type) > { > - u32 reg; > + u32 reg, mask = 0; > > - /* 0: mask, 1: unmask */ > - reg = readl(dp->reg_base + ANALOGIX_DP_COMMON_INT_MASK_4); > - reg &= ~COMMON_INT_MASK_4; > - writel(reg, dp->reg_base + ANALOGIX_DP_COMMON_INT_MASK_4); > + if (!irq_type) > + return; > > - reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA_MASK); > - reg &= ~INT_STA_MASK; > - writel(reg, dp->reg_base + ANALOGIX_DP_INT_STA_MASK); > + if (irq_type & COMMON_INT_4_HPD_IRQ) { > + /* 0: mask, 1: unmask */ > + reg = readl(dp->reg_base + ANALOGIX_DP_COMMON_INT_MASK_4); [Severity: High] Does this code perform an unprotected MMIO read on the DP controller when using a GPIO for HPD? If dp->hpd_gpiod is active, the DP controller might be in runtime suspend (with power/clocks off) while the GPIO HPD IRQ remains enabled. A cable plug would trigger the GPIO interrupt, and the call to analogix_dp_mute_hpd_interrupt() in analogix_dp_hardirq() will attempt to read the DP controller's mask registers here, potentially causing a bus hang. > + if (irq_type & DP_IRQ_TYPE_HP_CABLE_IN) > + mask |= PLUG; > + if (irq_type & DP_IRQ_TYPE_HP_CABLE_OUT) > + mask |= HPD_LOST; > + if (irq_type & DP_IRQ_TYPE_HP_CHANGE) > + mask |= HOTPLUG_CHG; > + reg &= ~mask; > + writel(reg, dp->reg_base + ANALOGIX_DP_COMMON_INT_MASK_4); > + } [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
