Re: [RFC PATCH v3 2/2] 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: usb_role_switch_at_endpoint() drops -EPROBE_DEFER
--

commit ed41e724f9aec8ba70adffc81a77e04c561b9932
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 USB role switches
registered on remote OF graph endpoints and calls it from
usb_role_switch_get() and fwnode_usb_role_switch_get().

> 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]
What happens in usb_role_switch_at_endpoint() if the consumer probes before
the provider has registered the role switch on the remote endpoint?

Looking at usb_role_switch_match() in drivers/usb/roles/class.c:

    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);

it already returns NULL when the "usb-role-switch" property is absent on the
remote endpoint, and returns ERR_PTR(-EPROBE_DEFER) when the property is
present but the switch device has not been registered yet.

Because usb_role_switch_at_endpoint() checks !IS_ERR_OR_NULL(sw) instead of
if (sw), ERR_PTR(-EPROBE_DEFER) is ignored and the function returns NULL.

In usb_role_switch_get() and fwnode_usb_role_switch_get(), if (!sw) then
falls through to device_connection_find_match() and
fwnode_connection_find_match(), which also return NULL because the remote
port parent and phandle properties do not carry "usb-role-switch". Callers
treat a NULL return as having no role switch configured rather than
deferring probe.

Should this check be if (sw) so that ERR_PTR(-EPROBE_DEFER) is returned to
the caller, matching fwnode_graph_devcon_matches() and
usb_role_switch_is_parent()?

> +	}
> +
> +	return NULL;
> +}

[ ... ]

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