[PATCH] drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable

[PATCH] drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable

[Yongxing Mou]

If a downstream consumer (e.g. drm_bridge_connector attached by the
msm/dp driver) registers its HPD callback after an upstream driver
has already reported a HPD event through drm_aux_hpd_bridge_notify(),
the notification is dropped because bridge->hpd_cb is still NULL.
This can affect any user of drm_aux_hpd_bridge_notify() whose
downstream consumer arms HPD only after upstream events have started.

The race has been observed on Qualcomm X1E-based laptops during boot,
when pmic_glink_altmode reports the initial USB-C DP connection state
before the DP driver has finished probing and enabled HPD handling on
the bridge. The consumer then never observes the initial connected
state and the external display remains dark.

Cache the last HPD status reported through drm_aux_hpd_bridge_notify()
and replay it when HPD is enabled by the downstream consumer.

The replay is deferred to a work item so that the replayed HPD
notification is delivered outside drm_bridge_hpd_enable()'s call
context.

This follows the same pattern as display-connector, which also defers
an initial HPD notification from .hpd_enable(), but reuses the cached
status since aux-hpd-bridge cannot re-detect sink presence on its own.

Fixes: e560518a6c2e ("drm/bridge: implement generic DP HPD bridge")
Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
---
 drivers/gpu/drm/bridge/aux-hpd-bridge.c | 53 ++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
index a2e525aa5788..bb81aabf58d4 100644
--- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
+++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
@@ -8,6 +8,7 @@
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/workqueue.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
@@ -18,6 +19,17 @@ static DEFINE_IDA(drm_aux_hpd_bridge_ida);
 struct drm_aux_hpd_bridge_data {
 	struct drm_bridge bridge;
 	struct device *dev;
+
+	/*
+	 * Last HPD status pushed through drm_aux_hpd_bridge_notify().
+	 * Replayed from .hpd_enable so that consumers registering their
+	 * callback after the initial notification are caught up.
+	 *
+	 * Accessed lockless from the notify path (writer) and hpd_work
+	 * (reader) - use WRITE_ONCE()/READ_ONCE().
+	 */
+	enum drm_connector_status last_status;
+	struct work_struct hpd_work;
 };
 
 static void drm_aux_hpd_bridge_release(struct device *dev)
@@ -154,6 +166,8 @@ void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status sta
 	if (!data)
 		return;
 
+	WRITE_ONCE(data->last_status, status);
+
 	drm_bridge_hpd_notify(&data->bridge, status);
 }
 EXPORT_SYMBOL_GPL(drm_aux_hpd_bridge_notify);
@@ -165,11 +179,45 @@ static int drm_aux_hpd_bridge_attach(struct drm_bridge *bridge,
 	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
 }
 
+static void drm_aux_hpd_bridge_hpd_work(struct work_struct *work)
+{
+	struct drm_aux_hpd_bridge_data *data =
+		container_of(work, struct drm_aux_hpd_bridge_data, hpd_work);
+	enum drm_connector_status status = READ_ONCE(data->last_status);
+
+	if (status == connector_status_unknown)
+		return;
+
+	drm_bridge_hpd_notify(&data->bridge, status);
+}
+
+/*
+ * Deferred to a work item so that the replayed HPD notification is
+ * delivered outside drm_bridge_hpd_enable()'s call context.
+ */
+static void drm_aux_hpd_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct drm_aux_hpd_bridge_data *data =
+		container_of(bridge, struct drm_aux_hpd_bridge_data, bridge);
+
+	schedule_work(&data->hpd_work);
+}
+
+static void drm_aux_hpd_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct drm_aux_hpd_bridge_data *data =
+		container_of(bridge, struct drm_aux_hpd_bridge_data, bridge);
+
+	cancel_work_sync(&data->hpd_work);
+}
+
 static const struct drm_bridge_funcs drm_aux_hpd_bridge_funcs = {
 	.atomic_create_state = drm_atomic_helper_bridge_create_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
-	.attach	= drm_aux_hpd_bridge_attach,
+	.attach	     = drm_aux_hpd_bridge_attach,
+	.hpd_enable  = drm_aux_hpd_bridge_hpd_enable,
+	.hpd_disable = drm_aux_hpd_bridge_hpd_disable,
 };
 
 static int drm_aux_hpd_bridge_probe(struct auxiliary_device *auxdev,
@@ -192,6 +240,9 @@ static int drm_aux_hpd_bridge_probe(struct auxiliary_device *auxdev,
 	data->bridge.interlace_allowed = true;
 	data->bridge.ycbcr_420_allowed = true;
 
+	data->last_status = connector_status_unknown;
+	INIT_WORK(&data->hpd_work, drm_aux_hpd_bridge_hpd_work);
+
 	auxiliary_set_drvdata(auxdev, data);
 
 	return devm_drm_bridge_add(data->dev, &data->bridge);

---
base-commit: 415606a7be939835db9b0d6b711887586646346d
change-id: 20260803-drm-usbdp-preboot-ca19b593211c

Best regards,
-- 
Yongxing Mou <yongxing.mou@oss.qualcomm.com>

Re: [PATCH] drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cancellation of `hpd_work` on device unbind leads to access of a destroyed mutex and use-after-free.
- [High] Lockless read of `last_status` in `hpd_work` creates a TOCTOU race condition that can deliver stale HPD events out of order.
--

--- Patch [1]: [PATCH] drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable ---
commit ac67055ec791a818497d8d0b82453f5ad00a28d3
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable

This commit caches the last HPD status reported by an upstream driver
and replays it when a downstream consumer enables HPD handling on the
bridge. The replay is deferred to a work item to run outside the
.hpd_enable() call context.

> diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> index a2e525aa57887..bb81aabf58d49 100644
> --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
[ ... ]
> @@ -154,6 +166,8 @@ void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status sta
>  	if (!data)
>  		return;
>  
> +	WRITE_ONCE(data->last_status, status);
> +
>  	drm_bridge_hpd_notify(&data->bridge, status);

[Severity: High]
Could this create a race condition with drm_aux_hpd_bridge_hpd_work() where
stale HPD events are delivered out of order?

If drm_aux_hpd_bridge_hpd_work() reads the status locklessly but gets
preempted before calling drm_bridge_hpd_notify(), a concurrent hardware
event here could update the status and dispatch it synchronously. 

When drm_aux_hpd_bridge_hpd_work() resumes, it might then dispatch the older, 
stale status it cached earlier, leaving the consumer with an outdated 
connection state.

>  }
>  EXPORT_SYMBOL_GPL(drm_aux_hpd_bridge_notify);
> @@ -165,11 +179,45 @@ static int drm_aux_hpd_bridge_attach(struct drm_bridge *bridge,
>  	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
>  }
>  
> +static void drm_aux_hpd_bridge_hpd_work(struct work_struct *work)
> +{
> +	struct drm_aux_hpd_bridge_data *data =
> +		container_of(work, struct drm_aux_hpd_bridge_data, hpd_work);
> +	enum drm_connector_status status = READ_ONCE(data->last_status);
> +
> +	if (status == connector_status_unknown)
> +		return;
> +
> +	drm_bridge_hpd_notify(&data->bridge, status);

[Severity: High]
Can this work item execute after the device is unbound?

If the auxiliary device unbinds while this work item is pending, devres
teardown will call drm_bridge_remove_void(), which in turn destroys
bridge->hpd_mutex and can free the data structure. 

If this work item executes after that teardown, wouldn't it result in a 
use-after-free on data and an attempt to lock a destroyed mutex inside
drm_bridge_hpd_notify()?

[ ... ]
> @@ -192,6 +240,9 @@ static int drm_aux_hpd_bridge_probe(struct auxiliary_device *auxdev,
>  	data->bridge.interlace_allowed = true;
>  	data->bridge.ycbcr_420_allowed = true;
>  
> +	data->last_status = connector_status_unknown;
> +	INIT_WORK(&data->hpd_work, drm_aux_hpd_bridge_hpd_work);
> +

[Severity: High]
Since INIT_WORK() is used here, should there be a corresponding devm cleanup
action to ensure the work is cancelled on unbind?

Adding something like devm_add_action_or_reset() to call cancel_work_sync() 
would ensure the work item doesn't outlive the data structure it operates on.

>  	auxiliary_set_drvdata(auxdev, data);
>  
>  	return devm_drm_bridge_add(data->dev, &data->bridge);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-drm-usbdp-preboot-v1-1-2539b362be00@oss.qualcomm.com?part=1