diff mbox series

[v5,21/21] drm: bridge: dw_hdmi: Drop call to drm_bridge_hpd_notify()

Message ID 20260510124111.1226584-22-jonas@kwiboo.se (mailing list archive)
State New
Headers
Series drm: bridge: dw_hdmi: Misc enable/disable, CEC and EDID cleanup |

Commit Message

Jonas Karlman May 10, 2026, 12:41 p.m. UTC 
The use of calls to both drm_helper_hpd_irq_event() and
drm_bridge_hpd_notify() in HPD delayed_work may cause multiple hotplug
uevents and modesets when the bridge connector is used.

Use of drm_helper_hpd_irq_event() cause the internal DRM function
check_connector_changed() to be called, which in turn calls the
connector detect()/force() funcs to detect any connection status or
epoch changes, and when changed trigger a hotplug uevent. For dw-hdmi
connector this also help ensure that EDID and CEC phys addr is updated.

If only a call drm_bridge_hpd_notify() would be used, a custom connector
status/EDID change detection logic needs to be implemented, to fully
match what check_connector_changed() already provides. Update of EDID
and CEC phys addr typically is delayed until userspace trigger a modeset
and fill_modes()/get_modes() ops is called.

The bridge connector detect() func also ensures that any hpd_notify()
funcs are called for all bridges in the chain, so there is not really
any need to have a call to drm_bridge_hpd_notify() here.

With both calls there is two hotplug uevents, two modesets and a total
of four .hpd_notify() calls (using a bridge connector):

  dw_hdmi_hardirq(): EVENT=plugout
  dw_hdmi_hpd_work()
  drm_helper_hpd_irq_event():
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] status updated from connected to disconnected
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] Changed epoch counter 1 => 2
    [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:46:HDMI-A-1] generating connector hotplug event
   drm_client_hotplug():
    [drm:drm_fb_helper_hotplug_event]
    [drm:drm_client_modeset_probe]
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1]
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1] disconnected
    [drm:drm_edid_connector_update] [CONNECTOR:46:HDMI-A-1] EDID changed, epoch counter 3
    [drm:drm_client_modeset_probe] No connectors reported connected with modes
    [drm:drm_client_modeset_probe] [CONNECTOR:46:HDMI-A-1] enabled? no
    [drm:drm_client_firmware_config.isra.0] Not using firmware configuration
    [drm:drm_client_modeset_probe] picking CRTCs for 3840x2160 config
    [drm:drm_client_hotplug] fbdev: ret=0
  drm_bridge_hpd_notify():
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:46:HDMI-A-1] generating connector hotplug event
   drm_client_hotplug():
    [drm:drm_fb_helper_hotplug_event]
    [drm:drm_client_modeset_probe]
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1]
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1] disconnected
    [drm:drm_client_modeset_probe] No connectors reported connected with modes
    [drm:drm_client_modeset_probe] [CONNECTOR:46:HDMI-A-1] enabled? no
    [drm:drm_client_firmware_config.isra.0] Not using firmware configuration
    [drm:drm_client_modeset_probe] picking CRTCs for 3840x2160 config
    [drm:drm_client_hotplug] fbdev: ret=0

Change to only call drm_helper_hpd_irq_event() in HPD delayed_work to
ensure there is only one hotplug uevent and that EDID and CEC phys addr
is updated in a timely manner, independent from userspace having to
react the hotplug uevent.

With only a call the drm_helper_hpd_irq_event() there is only a single
hotplug uevent and only two .hpd_notify() calls:

  dw_hdmi_hardirq(): EVENT=plugout
  dw_hdmi_hpd_work()
  drm_helper_hpd_irq_event():
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] status updated from connected to disconnected
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] Changed epoch counter 1 => 2
    [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:46:HDMI-A-1] generating connector hotplug event
   drm_client_hotplug():
    [drm:drm_fb_helper_hotplug_event]
    [drm:drm_client_modeset_probe]
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1]
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1] disconnected
    [drm:drm_edid_connector_update] [CONNECTOR:46:HDMI-A-1] EDID changed, epoch counter 3
    [drm:drm_client_modeset_probe] No connectors reported connected with modes
    [drm:drm_client_modeset_probe] [CONNECTOR:46:HDMI-A-1] enabled? no
    [drm:drm_client_firmware_config.isra.0] Not using firmware configuration
    [drm:drm_client_modeset_probe] picking CRTCs for 3840x2160 config
    [drm:drm_client_hotplug] fbdev: ret=0

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v5: New patch
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

sashiko-bot@kernel.org May 12, 2026, 3:50 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removing `drm_bridge_hpd_notify()` violates the DRM bridge API contract, breaks hotplug for non-polling setups, and causes inefficient global polling.
- [High] Teardown asymmetry allows the interrupt handler to re-arm canceled delayed work, leading to a Use-After-Free.
- [High] TOCTOU race condition in asynchronous workqueue leads to Use-After-Free on DRM device teardown.
--

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

drm: bridge: dw_hdmi: Drop call to drm_bridge_hpd_notify()

This commit drops the call to drm_bridge_hpd_notify() in the HDMI hotplug
delayed work to prevent duplicate hotplug uevents and modesets, relying
solely on drm_helper_hpd_irq_event().

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 2ea8ce5eca367..d9c9d03f8eff2 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3019,14 +3019,28 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
>  static void dw_hdmi_hpd_work(struct work_struct *work)
>  {
>  	struct dw_hdmi *hdmi = container_of(work, struct dw_hdmi, hpd_work.work);

This is an existing issue not introduced by this patch, but does the
delayed work lifecycle have a use-after-free vulnerability during teardown?

If cancel_delayed_work_sync(&hdmi->hpd_work) is called in dw_hdmi_remove()
before hardware interrupts are muted, can dw_hdmi_hardirq() silently
re-arm the workqueue? If the hdmi structure is then freed by devres,
execution of this work function later would cause a use-after-free.

> -	enum drm_connector_status status;
>  
>  	if (WARN_ON(!hdmi->bridge.dev))

This is another pre-existing issue, but is there a time-of-check to
time-of-use race condition here?

Since dw_hdmi is an external bridge, its lifetime is independent of the
master DRM device. If the master DRM driver unbinds concurrently, it
could set hdmi->bridge.dev to NULL and free the DRM device.

Could this workqueue evaluate hdmi->bridge.dev as non-NULL, get interrupted,
and then pass a freed DRM device pointer to drm_helper_hpd_irq_event()
below?

>  		return;

[ ... ]

>  	drm_helper_hpd_irq_event(hdmi->bridge.dev);
> -	status = dw_hdmi_phy_read_hpd(hdmi, hdmi->phy.data);
> -	drm_bridge_hpd_notify(&hdmi->bridge, status);
>  }

Does removing drm_bridge_hpd_notify() violate the DRM_BRIDGE_OP_HPD
contract and break hotplug functionality for non-polling setups?

Looking at drm_helper_hpd_irq_event(), it immediately returns false if
dev->mode_config.poll_enabled is false. For setups relying strictly on
interrupt-driven bridge hotplug notifications without enabling polling,
would this leave hotplug completely broken?

Additionally, instead of handling just the HDMI port, will
drm_helper_hpd_irq_event() force a synchronous detect() cycle on all
connectors on the DRM device that have the DRM_CONNECTOR_POLL_HPD flag set,
causing an inefficient system-wide poll?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 2ea8ce5eca36..d9c9d03f8eff 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3019,14 +3019,28 @@  static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
 static void dw_hdmi_hpd_work(struct work_struct *work)
 {
 	struct dw_hdmi *hdmi = container_of(work, struct dw_hdmi, hpd_work.work);
-	enum drm_connector_status status;
 
 	if (WARN_ON(!hdmi->bridge.dev))
 		return;
 
+	/*
+	 * Notify the DRM core of the HPD event using drm_helper_hpd_irq_event()
+	 * instead of drm_bridge_hpd_notify(). This will cause the DRM function
+	 * check_connector_changed() to be called, which in turn calls the
+	 * connector detect()/force() funcs to detect any connection status or
+	 * epoch changes. Something that also triggers EDID and CEC phys address
+	 * updates.
+	 *
+	 * If we were to instead call drm_bridge_hpd_notify() here, we would
+	 * have to implement a very similar change detection logic or fully
+	 * relay on userspace to react on a hotplug uevent to ensure EDID and
+	 * CEC phys address are updated.
+	 *
+	 * The bridge connector detect() func also ensures that hpd_notify()
+	 * funcs are called for all bridges in the chain.
+	 */
+
 	drm_helper_hpd_irq_event(hdmi->bridge.dev);
-	status = dw_hdmi_phy_read_hpd(hdmi, hdmi->phy.data);
-	drm_bridge_hpd_notify(&hdmi->bridge, status);
 }
 
 static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {