Re: [PATCH v1 3/7] drm/bridge: analogix_dp: Extend mute/unmute HPD interrupts to accept irq bitmask

[sashiko-bot@kernel.org]

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 <damon.ding@rock-chips.com>

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/20260730032744.381566-1-damon.ding@rock-chips.com?part=3