Re: [PATCH v4 2/3] usb: roles: look up role switches attached to a graph endpoint

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: roles: propagate -EPROBE_DEFER from usb_role_switch_at_endpoint()
--

commit 9ff4dd8101d254581dbf9332fc352de1c83d6e87
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

usb: roles: look up role switches attached to a graph endpoint

This patch adds usb_role_switch_at_endpoint() to look up a USB role switch
registered on a remote OF graph endpoint and calls it from
usb_role_switch_get() and fwnode_usb_role_switch_get() after checking the
caller's parent node.

> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index edec139b68b52..fc739fd61f033 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -157,6 +157,42 @@ usb_role_switch_is_parent(struct fwnode_handle *fwnode)
>  	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>  }
>  
> +/*
> + * Look up a role switch registered against the remote endpoint of one of the
> + * caller's OF graph endpoints.  This is how a connector endpoint that shares a
> + * USB data bus with another connector (for example two Type-C receptacles on a
> + * single DWC3 controller, selected by a GPIO mux) finds its own role switch.
> + */
> +static struct usb_role_switch *
> +usb_role_switch_at_endpoint(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *ep, *remote, *parent;
> +	struct usb_role_switch *sw;
> +	bool available;
> +
> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
> +		parent = fwnode_graph_get_remote_port_parent(ep);
> +		if (!parent)
> +			continue;
> +		available = fwnode_device_is_available(parent);
> +		fwnode_handle_put(parent);
> +		if (!available)
> +			continue;
> +
> +		remote = fwnode_graph_get_remote_endpoint(ep);
> +		if (!remote)
> +			continue;
> +		sw = usb_role_switch_match(remote, "usb-role-switch", NULL);
> +		fwnode_handle_put(remote);
> +		if (!IS_ERR_OR_NULL(sw)) {
> +			fwnode_handle_put(ep);
> +			return sw;
> +		}

[Severity: High]
Should this check be if (sw) instead of if (!IS_ERR_OR_NULL(sw)) so that
probe deferral is propagated to the caller?

In drivers/usb/roles/class.c, usb_role_switch_match() returns NULL when the
remote endpoint does not have the "usb-role-switch" property, and returns
ERR_PTR(-EPROBE_DEFER) when the property is present on the endpoint node
but the role switch device has not been registered yet:

drivers/usb/roles/class.c:usb_role_switch_match() {
	if (id && !fwnode_property_present(fwnode, id))
		return NULL;

	dev = class_find_device_by_fwnode(&role_class, fwnode);

	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
}

Because usb_role_switch_at_endpoint() checks !IS_ERR_OR_NULL(sw),
ERR_PTR(-EPROBE_DEFER) is ignored when a connector consumer calls
usb_role_switch_get() or fwnode_usb_role_switch_get() before the USB
controller driver registers the endpoint role switch.

usb_role_switch_at_endpoint() then returns NULL, and both
usb_role_switch_get() and fwnode_usb_role_switch_get() fall through to
connection matching (which checks the remote port parent node rather than
the endpoint node) and ultimately return NULL instead of
ERR_PTR(-EPROBE_DEFER), causing consumer drivers to treat the role switch
as absent rather than deferring probe.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026123211.243224-1-xiaqinghuan@mail.ustc.edu.cn?part=2