Re: [PATCH v5 17/21] drm: bridge: dw_hdmi: Rework HDP and RXSENSE interrupt handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The delayed work `hpd_work` can be re-armed during device removal, leading to a Use-After-Free.
- [High] The new HPD debouncing logic entirely swallows short HPD pulses meant to signal EDID changes.
- [High] The IRQ handler can overwrite the teardown interrupt mute, leaving interrupts enabled after driver unloads.
--

commit 7856139a882918cd7a789f36dd7c8fda0cd5ba3d
Author: Jonas Karlman <jonas@kwiboo.se>

drm: bridge: dw_hdmi: Rework HDP and RXSENSE interrupt handling

This commit removes RXSENSE handling to simplify the HPD interrupt handling
and instead depends on a debounced HPD work to refresh the EDID. It also
ensures the initial HPD interrupt polarity is based on current HPD status.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index f32eea9106b00..7fd2de41bc02c 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c

[ ... ]

> @@ -3051,78 +3002,29 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
>  
>  void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
>  {

[ ... ]

>  static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  {
>  	struct dw_hdmi *hdmi = dev_id;

[ ... ]

> +	intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
> +	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
> +		enum drm_connector_status status;
>  
> -		if (!(phy_stat & (HDMI_PHY_HPD | HDMI_PHY_RX_SENSE)))
> -			status = connector_status_disconnected;
> -	}
> +		/* Set HPD interrupt polarity based on current HPD status. */
> +		status = dw_hdmi_phy_read_hpd(hdmi, hdmi->phy.data);
> +		hdmi_modb(hdmi, status == connector_status_connected ?
> +			  0 : HDMI_PHY_HPD, HDMI_PHY_HPD, HDMI_PHY_POL0);
>  
> -	if (status != connector_status_unknown) {
>  		dev_dbg(hdmi->dev, "EVENT=%s\n",
>  			status == connector_status_connected ?
>  			"plugin" : "plugout");
>  
> +		mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
> +				 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));

Can this delayed work be re-armed during device removal, leading to a
use-after-free?

In dw_hdmi_remove(), cancel_delayed_work_sync(&hdmi->hpd_work) is called
before the hardware PHY interrupts are muted. Furthermore, the IRQ is
managed by the devm framework, so the handler is not unregistered until
after dw_hdmi_remove() returns.

If an HPD interrupt fires in the window after the work is canceled but
before interrupts are disabled, the IRQ handler will execute and
unconditionally call mod_delayed_work(), placing the work back on the
queue. When the devm framework subsequently frees the hdmi structure, the
queued work remains. When the timer expires, dw_hdmi_hpd_work() will
execute and dereference the freed hdmi pointer.

Also, does this debouncing logic swallow short HPD pulses meant to signal
EDID changes?

HDMI 1.4b specifications state that a sink indicates an EDID change by
driving a low voltage level pulse on the HPD pin for at least 100ms. The
delayed work debounce waits for HOTPLUG_DEBOUNCE_MS.

When a pulse occurs, the timer is queued on the falling edge and
re-queued/extended on the rising edge. When dw_hdmi_hpd_work() finally runs
after the debounce period, the HPD line has returned to high. Because this
steady-state matches the existing connector status,
drm_helper_hpd_irq_event() will not see a status change and thus will not
generate a hotplug event. This completely prevents the system from reading
the new EDID.

> +	}
>  
>  	hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
> -	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE),
> -		    HDMI_IH_MUTE_PHY_STAT0);
> +	hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);

Can this overwrite the teardown interrupt mute, leaving interrupts enabled
after the driver unloads?

During device teardown, dw_hdmi_remove() attempts to disable all interrupts
by writing ~0 to this exact register. Since dw_hdmi_remove() does not
synchronize with the IRQ handler (the IRQ is freed by devres only after
remove returns), the IRQ handler can run concurrently and overwrite the
teardown's ~0 with ~HDMI_IH_PHY_STAT0_HPD.

This leaves the PHY interrupts unmuted at the hardware level. Because the
IRQ is registered with IRQF_SHARED, a subsequent HPD hardware event will
assert the shared IRQ line with no registered handler, causing an unhandled
interrupt storm and forcing the kernel to permanently disable the shared
IRQ line.

>  
>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510124111.1226584-1-jonas@kwiboo.se?part=17