Re: [PATCH] drm/connector: Cache out-of-band hotplug events

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Out-of-order delivery of OOB hotplug events due to a Time-of-Check to Time-of-Use (TOCTOU) race condition during connector registration.
- [Medium] Permanent memory leak of fwnode references and drm_oob_hotplug_state structs for dynamically generated fwnodes.
--

--- Patch [1]: [PATCH] drm/connector: Cache out-of-band hotplug events ---
commit 4d3add94a455defa7c034e56e4bb755bf2de93af
Author: Sebastian Reichel <[email protected]>

drm/connector: Cache out-of-band hotplug events

When the USB-C state machine finished negotiating DP AltMode before the
DRM device has been probed, the out-of-band hotplug events fired to
early and are lost. Without replugging the display or reloading the
USB-C driver, the DRM driver assumes nothing is plugged.

Link: https://lore.kernel.org/dri-devel/20767137.geO5KgaWL5@diego/
Link: https://lore.kernel.org/dri-devel/[email protected]/

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8b4baed060f3a..b69e3776b1539 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c

[ ... ]

> @@ -829,6 +859,36 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  
> +/**
> + * drm_connector_replay_oob_hotplug_event - send cached OOB HPD event
> + * @connector: the connector that should receive the event
> + *
> + * Send the cached out-of-band hotplug as a new out-of-band hotplug event.
> + */
> +static void drm_connector_replay_oob_hotplug_event(struct drm_connector *connector)
> +{
> +	struct fwnode_handle *fwnode = connector->fwnode;
> +	enum drm_connector_status status;
> +	struct drm_oob_hotplug_state *e;
> +	bool found = false;
> +
> +	if (!fwnode || !connector->funcs->oob_hotplug_event)
> +		return;
> +
> +	scoped_guard(mutex, &oob_hotplug_list_lock) {
> +		list_for_each_entry(e, &oob_hotplug_list, head) {
> +			if (e->fwnode == fwnode || fwnode->secondary == e->fwnode) {
> +				status = e->status;
> +				found = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (found)
> +		connector->funcs->oob_hotplug_event(connector, status);

[Severity: High]
Does this create a race condition that could result in out-of-order event
delivery?

When this function drops the oob_hotplug_list_lock after reading the state,
a concurrent hardware hotplug event could be processed by
drm_connector_oob_hotplug_event() in another thread.

If that thread updates the cache and fires the connector callback with a
new connected status, would this function then execute its callback with
the older, stale cached status, permanently desynchronizing the state?

> +}
> +

[ ... ]

> @@ -3671,6 +3737,41 @@ struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
>  	return found;
>  }
>  
> +/**
> + * drm_connector_record_oob_hotplug_status - Cache OOB hotplug status
> + * @fwnode - fwnode for the DRM connector
> + * @status - out-of-band status info
> + *
> + * Cache the latest out-of-band hotplug status for a fwnode so it can be
> + * (re)played from when the DRM device is (re)registered after this event
> + * arrived.
> + */
> +static void drm_connector_record_oob_hotplug_status(struct fwnode_handle *fwnode,
> +						    enum drm_connector_status status)
> +{
> +	struct drm_oob_hotplug_state *e;
> +
> +	if (!fwnode)
> +		return;
> +
> +	guard(mutex)(&oob_hotplug_list_lock);
> +
> +	list_for_each_entry(e, &oob_hotplug_list, head) {
> +		if (e->fwnode == fwnode) {
> +			e->status = status;
> +			return;
> +		}
> +	}
> +
> +	e = kzalloc(sizeof(*e), GFP_KERNEL);
> +	if (!e)
> +		return;
> +
> +	e->fwnode = fwnode_handle_get(fwnode);
> +	e->status = status;
> +	list_add_tail(&e->head, &oob_hotplug_list);

[Severity: Medium]
Will this leak memory and fwnode references for dynamically generated fwnodes?

It appears that a new state object is allocated and appended to the global
oob_hotplug_list, and a reference to the fwnode is taken. However, there
does not seem to be any eviction mechanism to remove these items when a
connector is destroyed.

If dynamic fwnodes are continually added and removed, could this result in
unbounded growth of the list and eventual resource exhaustion?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-drm-connector-oob-hotplug-cache-v1-1-e92e146fdc7b@collabora.com?part=1