Re: [PATCH v7 19/23] drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The debounce logic completely filters out the 100ms EDID change pulse, causing the DRM core to miss the hotplug event.
- [High] The shared IRQ handler is freed before hardware interrupts are masked, potentially causing an unhandled interrupt storm.
--

commit 2b07e0d99061a1389b7eef1c604f98ccf0e7d115
Author: Jonas Karlman <jonas@kwiboo.se>

drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event

This commit introduces a delayed work queue to debounce HDMI hotplug detect
(HPD) events. It aims to improve the handling of EDID change pulses from the
sink by applying an 1100 msec timeout between the event and the DRM core
notification.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c

[ ... ]

> @@ -3101,8 +3136,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  			status == connector_status_connected ?
>  			"plugin" : "plugout");
>  
> -		if (hdmi->bridge.dev)
> -			drm_helper_hpd_irq_event(hdmi->bridge.dev);
> +		mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
> +				 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
>  	}

Does this debounce logic unintentionally filter out the 100ms EDID change pulse
entirely?

When the sink signals an EDID change by pulsing HPD low for 100ms and then high
again, mod_delayed_work() will be called on both edges. The second call on the
rising edge will reset the timer, pushing the work execution to 1100ms after
the pin returns high.

When dw_hdmi_hpd_work() finally runs and calls drm_helper_hpd_irq_event(), the
DRM connector detect function will see the current HPD status as connected.
Since the previous status known to the DRM core was also connected (because the
brief disconnect was never sampled), check_connector_changed() will conclude
that nothing changed. The epoch counter will not increment, and the userspace
uevent might be lost.

[ ... ]

> @@ -3532,6 +3593,18 @@ EXPORT_SYMBOL_GPL(dw_hdmi_probe);
>  
>  void dw_hdmi_remove(struct dw_hdmi *hdmi)
>  {
> +	struct platform_device *pdev = to_platform_device(hdmi->dev);
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	/* Free, mute and clear phy interrupts */
> +	devm_free_irq(hdmi->dev, irq, hdmi);
> +	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
> +	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
> +		    HDMI_IH_PHY_STAT0);

Could freeing the shared IRQ handler before masking the hardware interrupts
cause an unhandled interrupt storm?

If the HDMI device asserts a level-triggered interrupt exactly when
devm_free_irq() is called, the interrupt line will stay asserted but the
dw_hdmi handler will no longer be registered to clear it. Because the IRQ is
shared (requested with IRQF_SHARED), the kernel will repeatedly invoke other
handlers on this IRQ line.

Those other handlers will all return IRQ_NONE, which can cause the kernel to
permanently disable the shared IRQ line.

Should the hardware interrupts be explicitly masked and cleared before the
handler is freed?

> +
> +	/* Cancel any pending hot plug work */
> +	cancel_delayed_work_sync(&hdmi->hpd_work);
> +
>  	drm_bridge_remove(&hdmi->bridge);

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic