Re: [PATCH v2 2/2] usb: dwc3: support two connectors sharing one HS data bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] usb: dwc3: uncanceled drd_work on error in dwc3_role_mux_init()
- [High] usb: dwc3: unlocked dwc3_set_mode() race in dwc3_mux_role_set()
- [High] usb: dwc3: role swap overwrites port stamp in dwc3_mux_role_set()
- [High] usb: dwc3: registering role switches on endpoint fwnodes breaks lookup
- [Medium] usb: dwc3: fwnode reference leak on error in dwc3_role_mux_init()
- [Medium] usb: dwc3: unread error and desired_role fields in dwc3_role_mux
- [Medium] usb: dwc3: missing dwc3_pre_set_role() call in dwc3_mux_role_set()
--

commit 5d2d5ebbc56e4b5ec53011cd4401439a8ee2fdcf
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

usb: dwc3: support two connectors sharing one HS data bus

This patch adds support for DWC3 controllers where two USB connectors share a
single High Speed data bus switched via a port-select GPIO. It registers a
usb_role_switch per connector endpoint, arbitrates requests between the two
switches, and flips the GPIO in __dwc3_set_mode() after tearing down the
previous role.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index fd5c2cd36c595..6607e65e8433d 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c

[ ... ]

> @@ -233,6 +239,27 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		dwc3_writel(dwc, DWC3_GCTL, reg);
>  	}
>  
> +	/*
> +	 * The previous host or gadget instance has been torn down above, so the
> +	 * shared High Speed data path can now be moved to the requesting
> +	 * connector.  A failure here must not abort the mode change half-way:
> +	 * the controller would be left with a torn-down role and a stale
> +	 * current_dr_role, which the next request would tear down a second time.
> +	 */
> +	if (mux && mux_port != mux->active_port) {
> +		ret = gpiod_direction_output(mux->select, mux_port);
> +		if (ret) {
> +			WRITE_ONCE(mux->error, ret);
> +			dev_err_ratelimited(dwc->dev,
> +					    "failed to select USB port %d: %d\n",
> +					    mux_port, ret);
> +		} else {
> +			spin_lock_irqsave(&dwc->lock, flags);
> +			mux->active_port = mux_port;
> +			spin_unlock_irqrestore(&dwc->lock, flags);
> +		}
> +	}

[Severity: Medium]
In struct dwc3_role_mux (in drivers/usb/dwc3/core.h), desired_role and error
are documented as:

 * @desired_role: role requested by the connector that currently wins
 * @desired_port: index of that connector
 * @active_port: index of the connector currently driving the data lines
 * @error: sticky data-path switch error, reported to the role-switch users

In __dwc3_set_mode(), GPIO failures are stored via WRITE_ONCE(mux->error, ret),
and dwc3_mux_role_set() stores mux->desired_role = selected_role before
returning 0, but neither mux->error nor mux->desired_role is ever read
anywhere in the driver. Additionally, when gpiod_direction_output() fails,
__dwc3_set_mode() leaves mux->active_port on the previous connector while
proceeding to initialize desired_dr_role for mux_port.

Were mux->error and mux->desired_role intended to be read and reported back to
the role-switch callers, or are they unused state?

> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index f3e37d383627a..49104779600be 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/extcon.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> @@ -501,10 +502,199 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
>  	return role;
>  }
>  
> +/*
> + * Index of the connector that should own the shared data path: the oldest
> + * pending request wins, and the last selection is kept when neither connector
> + * requests a role.
> + */
> +static int dwc3_role_mux_pick(struct dwc3_role_mux *mux)
> +{
> +	int sel = mux->desired_port;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mux->ports); i++) {
> +		if (mux->ports[i].role == USB_ROLE_NONE)
> +			continue;
> +		if (mux->ports[sel].role == USB_ROLE_NONE ||
> +		    time_before(mux->ports[i].stamp, mux->ports[sel].stamp))
> +			sel = i;
> +	}
> +
> +	return sel;
> +}
> +
> +static u32 dwc3_role_mux_mode(struct dwc3 *dwc, enum usb_role role)
> +{
> +	if (role == USB_ROLE_HOST)
> +		return DWC3_GCTL_PRTCAP_HOST;
> +	if (role == USB_ROLE_DEVICE)
> +		return DWC3_GCTL_PRTCAP_DEVICE;
> +	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
> +		return DWC3_GCTL_PRTCAP_HOST;
> +
> +	return DWC3_GCTL_PRTCAP_DEVICE;
> +}
> +
> +static int dwc3_mux_role_set(struct usb_role_switch *sw, enum usb_role role)
> +{
> +	struct dwc3_role_port *port = usb_role_switch_get_drvdata(sw);
> +	struct dwc3 *dwc = port->dwc;
> +	struct dwc3_role_mux *mux = dwc->role_mux;
> +	unsigned long flags;
> +	enum usb_role selected_role;
> +	int selected;
> +
> +	if (role > USB_ROLE_DEVICE)
> +		return -EINVAL;
> +
> +	mutex_lock(&mux->lock);
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	if (role != port->role && role != USB_ROLE_NONE)
> +		port->stamp = jiffies;

[Severity: High]
When an already-active connector performs an in-place role swap (such as a USB
PD Data Role Swap between USB_ROLE_DEVICE and USB_ROLE_HOST without
disconnecting), won't updating port->stamp here cause it to lose ownership of
the shared HS data bus to the other connector?

If port 0 connects at t0 and port 1 connects later at t1 > t0, port 0 retains
the mux because time_before(t0, t1) is true in dwc3_role_mux_pick(). If port 0
then swaps directly between DEVICE and HOST at t2 > t1, role != port->role is
true, so port 0's stamp is overwritten with t2. dwc3_role_mux_pick() then sees
time_before(t1, t2) and switches the data path away from port 0 to port 1
mid-session. Also, if a connection stays active longer than MAX_LONG jiffies,
time_before() will wrap around.

Should port->stamp only be updated when transitioning out of USB_ROLE_NONE
(port->role == USB_ROLE_NONE && role != USB_ROLE_NONE)?

> +	port->role = role;
> +	selected = dwc3_role_mux_pick(mux);
> +	mux->desired_port = selected;
> +	selected_role = mux->ports[selected].role;
> +	mux->desired_role = selected_role;
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +	mutex_unlock(&mux->lock);
> +
> +	/*
> +	 * Same asynchronous semantics as the single-connector role switch: the
> +	 * data path and the controller are reprogrammed from __dwc3_set_mode().
> +	 */
> +	dwc3_set_mode(dwc, dwc3_role_mux_mode(dwc, selected_role));

[Severity: High]
Can releasing mux->lock and dwc->lock before calling dwc3_set_mode() desynchronize
mux->desired_port and dwc->desired_dr_role when both connectors change roles
concurrently?

Each port has its own struct usb_role_switch with a separate sw->lock mutex,
so two threads can enter dwc3_mux_role_set() in parallel:

Thread A (port 0)                      Thread B (port 1)
-----------------                      -----------------
mutex_lock(&mux->lock)
spin_lock_irqsave(&dwc->lock)
mux->desired_port = 0
selected_role = HOST
spin_unlock_irqrestore(&dwc->lock)
mutex_unlock(&mux->lock)
                                       mutex_lock(&mux->lock)
                                       spin_lock_irqsave(&dwc->lock)
                                       mux->desired_port = 1
                                       selected_role = DEVICE
                                       spin_unlock_irqrestore(&dwc->lock)
                                       mutex_unlock(&mux->lock)
                                       dwc3_set_mode(dwc, PRTCAP_DEVICE)
dwc3_set_mode(dwc, PRTCAP_HOST)

When __dwc3_set_mode() runs, it snapshots both fields under dwc->lock:

	spin_lock_irqsave(&dwc->lock, flags);
	desired_dr_role = dwc->desired_dr_role;
	if (mux)
		mux_port = mux->desired_port;
	spin_unlock_irqrestore(&dwc->lock, flags);

In the interleaving above (or if __dwc3_set_mode() runs between updating
mux->desired_port in dwc3_mux_role_set() and updating dwc->desired_dr_role in
dwc3_set_mode()), __dwc3_set_mode() switches the HS GPIO mux to port 1 while
programming the controller role for port 0.

[Severity: Medium]
Also in dwc3_mux_role_set(), should this path call
dwc3_pre_set_role(dwc, selected_role) before dwc3_set_mode() like the
single-connector dwc3_usb_role_switch_set() does?

	dwc3_pre_set_role(dwc, role);
	dwc3_set_mode(dwc, mode);

Without dwc3_pre_set_role(), glue drivers implementing glue_ops->pre_set_role
(such as dwc3-imx, dwc3-imx8mp, and dwc3-qcom) will not be notified of role
changes when port-select-gpios is used.

> +
> +	return 0;
> +}

[ ... ]

> +static void dwc3_role_mux_exit(struct dwc3 *dwc)
> +{
> +	struct dwc3_role_mux *mux = dwc->role_mux;
> +	unsigned int i;
> +
> +	if (!mux)
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(mux->ports); i++) {
> +		if (mux->ports[i].sw)
> +			usb_role_switch_unregister(mux->ports[i].sw);
> +		mux->ports[i].sw = NULL;
> +		fwnode_handle_put(mux->ports[i].fwnode);
> +		mux->ports[i].fwnode = NULL;
> +	}
> +}
> +
> +/* Returns 1 when the optional two-connector mux has been set up. */
> +static int dwc3_role_mux_init(struct dwc3 *dwc, u32 mode)
> +{
> +	struct usb_role_switch_desc desc = {};
> +	struct dwc3_role_mux *mux;
> +	struct fwnode_handle *ep;
> +	struct gpio_desc *select;
> +	char name[64];
> +	u32 state;
> +	int ret, i;
> +
> +	select = devm_gpiod_get_optional(dwc->dev, "port-select", GPIOD_ASIS);
> +	if (IS_ERR(select))
> +		return PTR_ERR(select);
> +	if (!select)
> +		return 0;
> +
> +	mux = devm_kzalloc(dwc->dev, sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return -ENOMEM;
> +
> +	ret = gpiod_get_value_cansleep(select);
> +	if (ret < 0)
> +		return ret;
> +
> +	mux->select = select;
> +	mux->desired_port = ret;
> +	mux->active_port = -1;
> +	mutex_init(&mux->lock);
> +
> +	fwnode_graph_for_each_endpoint(dev_fwnode(dwc->dev), ep) {
> +		if (!fwnode_property_present(ep, "usb-role-switch"))
> +			continue;
> +
> +		ret = fwnode_property_read_u32(ep, "mux-state", &state);
> +		if (ret || state >= ARRAY_SIZE(mux->ports)) {
> +			ret = -EINVAL;
> +			goto err_put_ep;
> +		}
> +		if (mux->ports[state].fwnode) {
> +			ret = -EINVAL;
> +			goto err_put_ep;
> +		}
> +
> +		mux->ports[state].fwnode = fwnode_handle_get(ep);
> +		mux->ports[state].state = state;
> +		mux->ports[state].dwc = dwc;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(mux->ports); i++) {
> +		if (!mux->ports[i].fwnode) {
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +	}

[Severity: Medium]
Does dwc3_role_mux_init() leak the mux->ports[].fwnode references if an error
occurs before dwc->role_mux = mux is assigned?

In the endpoint loop above, each valid endpoint has its reference count
incremented with mux->ports[state].fwnode = fwnode_handle_get(ep). If a later
endpoint jumps to err_put_ep, or if the check for !mux->ports[i].fwnode jumps
to err, dwc->role_mux is still NULL. In dwc3_role_mux_exit(dwc):

	struct dwc3_role_mux *mux = dwc->role_mux;
	...
	if (!mux)
		return;

it returns early without calling fwnode_handle_put() on the previously
populated mux->ports[].fwnode entries.

> +
> +	dwc->role_mux = mux;
> +	dwc3_set_mode(dwc, mode);
> +
> +	for (i = 0; i < ARRAY_SIZE(mux->ports); i++) {
> +		snprintf(name, sizeof(name), "%s-port%u", dev_name(dwc->dev), i);
> +		desc.name = name;
> +		desc.fwnode = mux->ports[i].fwnode;
> +		desc.set = dwc3_mux_role_set;
> +		desc.get = dwc3_mux_role_get;
> +		desc.driver_data = &mux->ports[i];
> +		mux->ports[i].sw = usb_role_switch_register(dwc->dev, &desc);

[Severity: High]
Will connector drivers be able to look up these role switches via
usb_role_switch_get() or fwnode_usb_role_switch_get() when desc.fwnode is set
to the endpoint fwnode mux->ports[i].fwnode?

When a connector driver calls usb_role_switch_get() or
fwnode_usb_role_switch_get(), fwnode_connection_find_match() walks the OF
graph via fwnode_graph_devcon_matches():

	fwnode_graph_for_each_endpoint(fwnode, ep) {
		node = fwnode_graph_get_remote_port_parent(ep);
		if (!fwnode_device_is_available(node)) {
			fwnode_handle_put(node);
			continue;
		}

		ret = match(node, con_id, data);

fwnode_graph_get_remote_port_parent(ep) resolves to the parent DWC3 device
fwnode (dev_fwnode(dwc->dev)), not the remote endpoint fwnode. Then
usb_role_switch_match() checks dev_fwnode(dwc->dev):

	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 dwc3_drd_init() requires "usb-role-switch" to be present on
dev_fwnode(dwc->dev) in order to call dwc3_setup_role_switch(),
fwnode_property_present(dev_fwnode(dwc->dev), "usb-role-switch") is true,
while class_find_device_by_fwnode(&role_class, dev_fwnode(dwc->dev)) returns
NULL because the switches were registered on the endpoint fwnodes instead.
Won't every consumer lookup return ERR_PTR(-EPROBE_DEFER) indefinitely?

> +		if (IS_ERR(mux->ports[i].sw)) {
> +			ret = PTR_ERR(mux->ports[i].sw);
> +			mux->ports[i].sw = NULL;
> +			goto err;
> +		}
> +	}
> +
> +	return 1;
> +
> +err:
> +	dwc3_role_mux_exit(dwc);
> +	dwc->role_mux = NULL;
> +	return ret;

[Severity: Critical]
If usb_role_switch_register() fails and jumps to err, can the queued
dwc->drd_work cause a use-after-free?

dwc3_set_mode(dwc, mode) is called before the registration loop, queueing
dwc->drd_work (__dwc3_set_mode) on system_freezable_wq. In addition, if port 0
registers and port 1 fails, a consumer on port 0 could call
dwc3_mux_role_set() and queue dwc->drd_work before port 0 is unregistered.

The err path calls dwc3_role_mux_exit(dwc) and sets dwc->role_mux = NULL
without cancelling dwc->drd_work or tearing down any role already started by
__dwc3_set_mode(). When the error propagates out of dwc3_drd_init() ->
dwc3_core_init_mode() to dwc3_core_probe(), probe failure disables clocks and
frees the devm-managed mux and dwc while __dwc3_set_mode() may still be
running or pending.

Should dwc3_set_mode(dwc, mode) be deferred until after both switches are
successfully registered, and should the error path cancel dwc->drd_work if any
switch was registered?

> +err_put_ep:
> +	fwnode_handle_put(ep);
> +	goto err;
> +}

[ ... ]

>
>
>

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