[PATCH v4 0/3] usb: dwc3: support two connectors sharing one HS data bus

[PATCH v4 0/3] usb: dwc3: support two connectors sharing one HS data bus

[Qianhui Xu]

This is a formal submission, not an RFC: it supersedes the two-patch v2
series (<20261017081320.220525-1-xiaqinghuan@mail.ustc.edu.cn>) and the
shape-only v3 RFC (<20261019131848.344689-1-xiaqinghuan@mail.ustc.edu.cn>),
and it contains the driver that uses the binding it describes.  Sorry for the confusion caused by sending v3 as an
RFC threaded under v2; the intent was to settle the binding shape first,
but that is not how it should have been posted.  The series is complete
below, with the disposition of every review comment listed.

Two connectors on one DWC3 controller
=====================================

The board (Lenovo Legion Y700 Gen4, SM8750) wires the Super Speed pairs of
two USB-C receptacles to separate PHYs, while the High Speed D+/D- pair is
shared through a 2:1 mux controlled by a single GPIO:

	usb@a600000 {
		port-select-gpios = <&tlmm 29 GPIO_ACTIVE_HIGH>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;

				endpoint@0 {
					reg = <0>;
					remote-endpoint = <&usb_0_dwc3_hs>;
					usb-role-switch;
					mux-state = <0>;
				};

				endpoint@1 {
					reg = <1>;
					remote-endpoint = <&usb_1_dwc3_hs>;
					usb-role-switch;
					mux-state = <1>;
				};
			};

			port@1 {
				/* Super Speed of connector 0, separate PHY */
			};

			port@2 {
				/* Super Speed of connector 1, separate PHY */
			};
		};
	};

Both connectors are data-role dual and hand their role to the SoC through
pmic-glink, so the controller has to arbitrate between two independent role
requests and move the shared data path before it is reinitialised.

Series layout
=============

1/3 documents the two-connector HS data path: "port-select-gpios" plus two
    numbered endpoints of "port@0" carrying "mux-state" (which GPIO level
    connects that connector) and "usb-role-switch" (this connector has its
    own role-switch input).

2/3 adds usb_role_switch_at_endpoint(), so a role switch registered against
    an OF graph endpoint can be found by the consumer that owns the other
    end of that link.  Without it the endpoint registration in 1/3 would
    have no lookup path.

3/3 implements the arbitration in DWC3: one role switch per connector, the
    oldest pending request wins, the GPIO is flipped inside __dwc3_set_mode()
    after the previous host/gadget instance is torn down, and a connector
    only reports a role while it owns the data path.

Disposition of the review comments
==================================

sashiko, binding (v2 1/2):

 - "model the board mux with the standard mux subsystem": not adopted.
   drivers/usb/ has no mux_control consumer, mux only selects a channel and
   cannot express "oldest requester wins" role arbitration, and gpio-mux
   drives its idle level at probe, which would override the selection left
   by the bootloader.  The mux is therefore described where it is used, as
   a GPIO plus per-endpoint level.

 - "port@2 models the second connector as a controller bus": fixed.
   The second connector is now endpoint@1 of port@0, so "ports" keeps
   describing the controller's own buses.

 - "usb-role-switch does not belong on a graph endpoint": kept, but the
   missing lookup path is fixed by 2/3 instead of relying on a downstream
   only helper.

 - "conditionals do not require ports": fixed, "ports" and "port@0" are
   required when "port-select-gpios" is present, and the endpoint
   properties are rejected when it is absent.

 - "two ports may carry the same mux-state": fixed, the values are
   constrained to be complementary.

sashiko, driver (v2 2/2):

 - Critical, "the init error path leaves drd_work queued": fixed.
   Both switches are now registered before the initial dwc3_set_mode(),
   and the error path cancels the queued role work.

 - High, "dwc3_set_mode() is called after dropping the lock": fixed.
   The request order and the resulting mode request are taken in one
   critical section; dwc3_set_mode() only queues work, so holding the mux
   lock across it is safe.

 - High, "an in-place role swap overwrites the request timestamp": fixed.
   A request ticket is only taken on a NONE -> role transition, so a
   connector that is already using the bus cannot steal it.

 - High, "registering on the endpoint breaks the lookup": addressed by 2/3.

 - Medium, "fwnode leak on the error path": fixed, the endpoint reference
   is dropped on every exit path.

 - Medium, "error and desired_role are never read": fixed, desired_role is
   gone and the sticky mux error is reported to the connector on its next
   role request.

 - Medium, "add dwc3_pre_set_role()": not adopted in this series; the role
   is set from the same place as the single-connector switch, and adding a
   second entry point would duplicate that path.  If you prefer the helper,
   I will restructure the series that way.

Testing
=======

Mechanical, re-run on v4:

 - checkpatch --strict: 0 errors, 0 warnings, 0 checks on all three patches.
 - dt-doc-validate on snps,dwc3-common.yaml: clean.
 - W=1 compiles clean for drivers/usb/dwc3/ and drivers/usb/roles/.
 - Full arm64 Image.gz + dtbs build of a v7.3-rc7 tree with the series
   applied.

Board behaviour: this board's daily kernel carries this series, and it
registers both per-connector switches, visible as
a600000.usb-port0-role-switch and a600000.usb-port1-role-switch.  In the
earlier revisions the mux logic was exercised plug-by-plug: host mode on
either receptacle, moving the cable from one receptacle to the other hands
the controller over, and a charging-only connector next to an active one
does not disturb it.

The v4 changes are the review fixes listed above: registering the switches
before the initial mode request, taking the request order and the mode
request in one critical section, ticket handling on NONE -> role only, the
monotonic counter instead of jiffies, and dropping the unused field.  Those
were reviewed and built but not re-exercised plug-by-plug, because the
author's test setup talks to this board through the very USB link being
switched; I would rather say that than claim a test I did not run.  I can
re-test on request if a maintainer wants the swap sequence confirmed before
this goes in.

Qianhui Xu (3):
  dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS bus
  usb: roles: look up role switches attached to a graph endpoint
  usb: dwc3: support two connectors sharing one HS data bus


Qianhui Xu (3):
  dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS
    bus
  usb: roles: look up role switches attached to a graph endpoint
  usb: dwc3: support two connectors sharing one HS data bus

 .../bindings/usb/snps,dwc3-common.yaml        | 109 ++++++++-
 drivers/usb/dwc3/core.c                       |  29 ++-
 drivers/usb/dwc3/core.h                       |  39 ++++
 drivers/usb/dwc3/debugfs.c                    |   4 +
 drivers/usb/dwc3/drd.c                        | 219 +++++++++++++++++-
 drivers/usb/roles/class.c                     |  40 ++++
 6 files changed, 437 insertions(+), 3 deletions(-)

-- 
2.53.0

[PATCH v4 1/3] dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS bus

[Qianhui Xu]

Two connectors can share the High Speed D+/D- pair of a single DWC3
controller, with a GPIO selecting which one is connected.  Super Speed
lanes stay per connector.

Describe that in snps,dwc3-common.yaml:

  - port-select-gpios selects which connector drives the shared HS bus;
  - the shared HS bus is port@0 with two numbered endpoints, endpoint@0
    and endpoint@1, so that "ports" keeps describing the controller's own
    buses as the graph bindings expect;
  - each endpoint carries "mux-state", the port-select level of that
    connector, and "usb-role-switch" to declare that this connector has
    its own role switch input.

When port-select-gpios is absent the endpoint properties are rejected,
so existing single-connector descriptions are unaffected.  The two
mux-state values are constrained to be complementary.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 .../bindings/usb/snps,dwc3-common.yaml        | 109 +++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
index 6c0b8b6..6b06b00 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
@@ -13,6 +13,38 @@ description:
   Defines the properties of the DWC3 core as being embedded in either an
   vendor-specific implementation or as a standalone component.
 
+$defs:
+  hs-endpoint:
+    $ref: /schemas/graph.yaml#/$defs/endpoint-base
+    unevaluatedProperties: false
+
+    properties:
+      usb-role-switch:
+        type: boolean
+        description:
+          This connector endpoint has its own USB role switch. The switch is
+          registered against this endpoint and looked up with
+          usb_role_switch_at_endpoint().
+
+      mux-state:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1]
+        description:
+          Logical level of the port-select GPIO which connects this
+          connector to the shared High Speed data bus.
+
+    dependentRequired:
+      usb-role-switch: [mux-state]
+      mux-state: [usb-role-switch]
+
+  hs-port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+
+    patternProperties:
+      '^endpoint@[01]$':
+        $ref: '#/$defs/hs-endpoint'
+
 allOf:
   - $ref: usb-drd.yaml#
   - if:
@@ -26,8 +58,83 @@ allOf:
       $ref: usb.yaml#
     else:
       $ref: usb-xhci.yaml#
+  - if:
+      required: [port-select-gpios]
+    then:
+      required: [dr_mode, ports]
+      properties:
+        dr_mode:
+          const: otg
+        ports:
+          required: [port@0]
+          properties:
+            port@0:
+              required: ['#address-cells', '#size-cells', endpoint@0, endpoint@1]
+              properties:
+                endpoint@0:
+                  required: [usb-role-switch, mux-state]
+                endpoint@1:
+                  required: [usb-role-switch, mux-state]
+    else:
+      properties:
+        ports:
+          properties:
+            port@0:
+              patternProperties:
+                '^endpoint@[01]$':
+                  properties:
+                    usb-role-switch: false
+                    mux-state: false
+  - if:
+      properties:
+        ports:
+          properties:
+            port@0:
+              properties:
+                endpoint@0:
+                  properties:
+                    mux-state:
+                      const: 0
+    then:
+      properties:
+        ports:
+          properties:
+            port@0:
+              properties:
+                endpoint@1:
+                  properties:
+                    mux-state:
+                      const: 1
+  - if:
+      properties:
+        ports:
+          properties:
+            port@0:
+              properties:
+                endpoint@1:
+                  properties:
+                    mux-state:
+                      const: 0
+    then:
+      properties:
+        ports:
+          properties:
+            port@0:
+              properties:
+                endpoint@0:
+                  properties:
+                    mux-state:
+                      const: 1
 
 properties:
+  port-select-gpios:
+    maxItems: 1
+    description:
+      Selects which of the two connectors drives the controller's shared High
+      Speed data bus. Each connector is described by a numbered endpoint of
+      "port@0" that carries its own role-switch input and the "mux-state"
+      level selecting it.
+
   extcon:
     maxItems: 1
     deprecated: true
@@ -411,7 +518,7 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: '#/$defs/hs-port'
         description: High Speed (HS) data bus.
 
       port@1:
-- 
2.53.0

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

[Qianhui Xu]

usb_role_switch_get() and fwnode_usb_role_switch_get() look for a switch
on the caller's parent device node or through device_connection.  That
does not work when one controller exposes a role switch per connector
endpoint: the switch is registered against the remote graph endpoint and
the consumer (the connector) has no phandle to it.

Add usb_role_switch_at_endpoint(), which walks the caller's OF graph
endpoints and matches a switch registered on the remote endpoint, and try
it after the existing parent lookup.  There is no behaviour change for
existing users: if nothing is registered against an endpoint the lookup
falls through to the previous paths.

This is needed by DWC3 designs where two Type-C receptacles share one
High Speed data bus.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 drivers/usb/roles/class.c | 40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index edec139..fc739fd 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;
+		}
+	}
+
+	return NULL;
+}
+
 /**
  * usb_role_switch_get - Find USB role switch linked with the caller
  * @dev: The caller device
@@ -169,6 +205,8 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 	struct usb_role_switch *sw;
 
 	sw = usb_role_switch_is_parent(dev_fwnode(dev));
+	if (!sw)
+		sw = usb_role_switch_at_endpoint(dev_fwnode(dev));
 	if (!sw)
 		sw = device_connection_find_match(dev, "usb-role-switch", NULL,
 						  usb_role_switch_match);
@@ -192,6 +230,8 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
 	struct usb_role_switch *sw;
 
 	sw = usb_role_switch_is_parent(fwnode);
+	if (!sw)
+		sw = usb_role_switch_at_endpoint(fwnode);
 	if (!sw)
 		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
 						  NULL, usb_role_switch_match);
-- 
2.53.0

[PATCH v4 3/3] usb: dwc3: support two connectors sharing one HS data bus

[Qianhui Xu]

On some boards a single DWC3 controller drives two receptacles: the Super
Speed pairs are wired to separate PHYs, while the High Speed data lines
are muxed to whichever connector is in use.  Such a controller has to
service two independent role-switch inputs, and the shared data path has
to be moved before the controller is reinitialised for the requested role.

Register one usb_role_switch per connector endpoint described by
"port-select-gpios" and the per-endpoint "mux-state" property.  Requests
from the two switches are serialised, the oldest pending request wins, and
the port-select GPIO is only changed once the previous host or gadget
instance has been torn down, which is where __dwc3_set_mode() runs.  A
connector only reports its own role while it owns the data path, so the
connector drivers see a consistent view of a shared controller.

The role change keeps the asynchronous semantics of the existing
single-connector role switch: __dwc3_set_mode() is the only place that
programs hardware, and it never aborts half-way, so a failed GPIO write
cannot leave a torn-down role behind.  The error is kept in the mux state
and reported back to the connector on its next role request.

While the two-connector mux owns role selection, the debugfs "mode" write
used to force a specific role is refused instead of racing with it.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
Changes in v4:
 - register both role switches before the initial dwc3_set_mode(), and
   cancel the queued role work on the init error path (Critical)
 - take the request order and the resulting mode request in one critical
   section instead of dropping the mux lock around dwc3_set_mode() (High)
 - only take a request ticket on a NONE -> role transition, so an in-place
   role swap cannot steal the shared data path from the connector that
   asked first (High)
 - order requests with a monotonic counter instead of jiffies, removing
   the time_before() wraparound (High)
 - drop the unused desired_role field and report the sticky mux error to
   the connector instead of keeping it unread (Medium)

Changes in v3:
 - binding moved to "port-select-gpios" plus numbered endpoints carrying
   "mux-state" and "usb-role-switch"; the endpoint lookup helper is part of
   the series
---
 drivers/usb/dwc3/core.c    |  29 ++++-
 drivers/usb/dwc3/core.h    |  39 +++++++
 drivers/usb/dwc3/debugfs.c |   4 +
 drivers/usb/dwc3/drd.c     | 219 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 289 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index fd5c2cd..6607e65 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/list.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/dma-mapping.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
@@ -165,7 +166,9 @@ EXPORT_SYMBOL_GPL(dwc3_set_prtcap);
 static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
+	struct dwc3_role_mux *mux = dwc->role_mux;
 	unsigned long flags;
+	int mux_port = 0;
 	int ret;
 	u32 reg;
 	u32 desired_dr_role;
@@ -174,6 +177,8 @@ static void __dwc3_set_mode(struct work_struct *work)
 	mutex_lock(&dwc->mutex);
 	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);
 
 	pm_runtime_get_sync(dwc->dev);
@@ -184,7 +189,8 @@ static void __dwc3_set_mode(struct work_struct *work)
 	if (!desired_dr_role)
 		goto out;
 
-	if (desired_dr_role == dwc->current_dr_role)
+	if (desired_dr_role == dwc->current_dr_role &&
+	    (!mux || mux_port == mux->active_port))
 		goto out;
 
 	if (desired_dr_role == DWC3_GCTL_PRTCAP_OTG && dwc->edev)
@@ -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);
+		}
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 
 	dwc3_set_prtcap(dwc, desired_dr_role, false);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f3dabb3..1238376 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1008,6 +1008,43 @@ struct dwc3_glue_ops {
 	void	(*pre_run_stop)(struct dwc3 *dwc, bool is_on);
 };
 
+/* Per-connector state of the optional shared-HS-data-bus mux. */
+struct dwc3_role_port {
+	struct dwc3		*dwc;
+	struct usb_role_switch	*sw;
+	struct fwnode_handle	*fwnode;
+	enum usb_role		role;
+	unsigned int		state;
+	/* request order, see dwc3_role_mux.seq */
+	u64			seq;
+};
+
+/**
+ * struct dwc3_role_mux - optional data-path mux between two connectors
+ * @select: GPIO selecting which connector drives the shared HS data lines
+ * @lock: serialises role requests coming from both connectors
+ * @ports: per-connector state, indexed by the endpoint's "mux-state"
+ * @desired_port: index of the connector that currently wins
+ * @active_port: index of the connector currently driving the data lines
+ * @seq: monotonic request counter used to order pending requests
+ * @error: sticky data-path switch error, reported to the role-switch users
+ *
+ * Two connectors can share the High Speed data lines of one controller while
+ * keeping their Super Speed lanes separate.  Each connector then needs its own
+ * role-switch input, and the shared data path has to be moved before the
+ * controller is reinitialised for the requested role.
+ */
+struct dwc3_role_mux {
+	struct gpio_desc	*select;
+	/* serialises role requests coming from both connectors */
+	struct mutex		lock;
+	struct dwc3_role_port	ports[2];
+	int			desired_port;
+	int			active_port;
+	u64			seq;
+	int			error;
+};
+
 /**
  * struct dwc3 - representation of our controller
  * @drd_work: workqueue used for role swapping
@@ -1064,6 +1101,7 @@ struct dwc3_glue_ops {
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
  * @role_sw: usb_role_switch handle
+ * @role_mux: optional shared data-path mux plus per-connector role switches
  * @role_switch_default_mode: default operation mode of controller while
  *			usb role is USB_ROLE_NONE.
  * @usb_psy: pointer to power supply interface.
@@ -1265,6 +1303,7 @@ struct dwc3 {
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
 	struct usb_role_switch	*role_sw;
+	struct dwc3_role_mux	*role_mux;
 	enum usb_dr_mode	role_switch_default_mode;
 
 	struct power_supply	*usb_psy;
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 65d909e..0d42f10 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -438,6 +438,10 @@ static ssize_t dwc3_mode_write(struct file *file,
 	u32			mode = 0;
 	char			buf[32];
 
+	/* The connector providers own both the role and the data-path selection. */
+	if (dwc->role_mux)
+		return -EBUSY;
+
 	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
 
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index f3e37d3..20821d6 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,219 @@ 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.  Requests are ordered by a monotonic counter rather than by
+ * a timestamp, so a wrapped counter cannot be mistaken for an old request.
+ */
+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 ||
+		    mux->ports[i].seq < mux->ports[sel].seq)
+			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, ret;
+
+	if (role > USB_ROLE_DEVICE)
+		return -EINVAL;
+
+	/*
+	 * The request order and the mode request it implies are decided under one
+	 * lock, so a second connector cannot change desired_port between the
+	 * selection and the dwc3_set_mode() call that consumes it.
+	 */
+	mutex_lock(&mux->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
+	/*
+	 * A role change between two non-NONE roles of the same connector must not
+	 * give that connector a new request ticket: it would otherwise steal the
+	 * data path from the connector that asked first.
+	 */
+	if (port->role == USB_ROLE_NONE && role != USB_ROLE_NONE)
+		port->seq = ++mux->seq;
+	port->role = role;
+	selected = dwc3_role_mux_pick(mux);
+	mux->desired_port = selected;
+	selected_role = mux->ports[selected].role;
+	/*
+	 * Same asynchronous semantics as the single-connector role switch: the
+	 * data path and the controller are reprogrammed from __dwc3_set_mode().
+	 * dwc3_set_mode() only queues that work, so the mux lock may be held.
+	 */
+	dwc3_set_mode(dwc, dwc3_role_mux_mode(dwc, selected_role));
+	ret = mux->error;
+	mux->error = 0;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	mutex_unlock(&mux->lock);
+
+	return ret;
+}
+
+static enum usb_role dwc3_mux_role_get(struct usb_role_switch *sw)
+{
+	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 role = USB_ROLE_NONE;
+
+	/*
+	 * A connector only reports its own role while it owns the shared data
+	 * path, so the connector drivers never fight over the controller.
+	 */
+	spin_lock_irqsave(&dwc->lock, flags);
+	if (mux->active_port == port->state)
+		role = port->role;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return role;
+}
+
+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;
+		}
+	}
+
+	dwc->role_mux = mux;
+
+	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);
+		if (IS_ERR(mux->ports[i].sw)) {
+			ret = PTR_ERR(mux->ports[i].sw);
+			mux->ports[i].sw = NULL;
+			goto err;
+		}
+	}
+
+	/*
+	 * The controller is only started in its default role once both switches
+	 * are registered: before that a connector driver cannot request a role,
+	 * so nothing can race with this initial mode request.
+	 */
+	dwc3_set_mode(dwc, mode);
+
+	return 1;
+
+err:
+	dwc3_role_mux_exit(dwc);
+	dwc->role_mux = NULL;
+	/* A mode request queued before the failure must not run on freed state. */
+	cancel_work_sync(&dwc->drd_work);
+	return ret;
+err_put_ep:
+	fwnode_handle_put(ep);
+	goto err;
+}
+
 static int dwc3_setup_role_switch(struct dwc3 *dwc)
 {
 	struct usb_role_switch_desc dwc3_role_switch = {NULL};
 	u32 mode;
+	int ret;
 
 	dwc->role_switch_default_mode = usb_get_role_switch_default_mode(dwc->dev);
 	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) {
@@ -513,6 +723,11 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 		dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
 		mode = DWC3_GCTL_PRTCAP_DEVICE;
 	}
+
+	ret = dwc3_role_mux_init(dwc, mode);
+	if (ret)
+		return ret < 0 ? ret : 0;
+
 	dwc3_set_mode(dwc, mode);
 
 	dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
@@ -526,7 +741,7 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 
 	if (dwc->dev->of_node) {
 		/* populate connector entry */
-		int ret = devm_of_platform_populate(dwc->dev);
+		ret = devm_of_platform_populate(dwc->dev);
 
 		if (ret) {
 			usb_role_switch_unregister(dwc->role_sw);
@@ -541,6 +756,7 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 #else
 #define ROLE_SWITCH 0
 #define dwc3_setup_role_switch(x) 0
+static inline void dwc3_role_mux_exit(struct dwc3 *dwc) {}
 #endif
 
 int dwc3_drd_init(struct dwc3 *dwc)
@@ -597,6 +813,7 @@ void dwc3_drd_exit(struct dwc3 *dwc)
 {
 	unsigned long flags;
 
+	dwc3_role_mux_exit(dwc);
 	if (dwc->role_sw)
 		usb_role_switch_unregister(dwc->role_sw);
 
-- 
2.53.0

Re: [PATCH v4 1/3] dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS bus

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: usb: snps,dwc3: mux-state if-then blocks allow duplicate 1
- [Medium] dt-bindings: usb: snps,dwc3: missing endpoint validation in port@0 hs-port
- [Medium] dt-bindings: usb: snps,dwc3: custom mux properties and incomplete constraints
--

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

dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS bus

This commit updates Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
to describe two connectors sharing the High Speed D+/D- bus of a single DWC3
controller via a GPIO selector. It adds port-select-gpios on the controller
node and endpoint@0/endpoint@1 under port@0 with mux-state and
usb-role-switch properties.

> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> index 6c0b8b6538246..6b06b0027b393 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> @@ -13,6 +13,38 @@ description:
>    Defines the properties of the DWC3 core as being embedded in either an
>    vendor-specific implementation or as a standalone component.
>  
> +$defs:
> +  hs-endpoint:
> +    $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +    unevaluatedProperties: false
> +
> +    properties:
> +      usb-role-switch:
> +        type: boolean
> +        description:
> +          This connector endpoint has its own USB role switch. The switch is
> +          registered against this endpoint and looked up with
> +          usb_role_switch_at_endpoint().
> +
> +      mux-state:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1]
> +        description:
> +          Logical level of the port-select GPIO which connects this
> +          connector to the shared High Speed data bus.
> +
> +    dependentRequired:
> +      usb-role-switch: [mux-state]
> +      mux-state: [usb-role-switch]
> +
> +  hs-port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +
> +    patternProperties:
> +      '^endpoint@[01]$':
> +        $ref: '#/$defs/hs-endpoint'

[Severity: Medium]
Does replacing /schemas/graph.yaml#/properties/port on properties/ports/port@0
with #/$defs/hs-port drop unevaluatedProperties: false validation for
unnumbered port@0/endpoint nodes and port@0/endpoint@[2-9a-f]+ nodes?

Previously, port@0 referenced /schemas/graph.yaml#/properties/port, which
applies /schemas/graph.yaml#/properties/endpoint (including
unevaluatedProperties: false) to all child nodes matching
^endpoint(@[0-9a-f]+)?$.

Because /schemas/graph.yaml#/$defs/port-base only validates
^endpoint(@[0-9a-f]+)?$ against /schemas/graph.yaml#/$defs/endpoint-base
(which leaves unevaluatedProperties open for extension), and $defs/hs-port
only applies #/$defs/hs-endpoint to ^endpoint@[01]$, any existing
single-connector DWC3 node using an unnumbered port@0/endpoint node (or
endpoint@2 and higher) will no longer have unevaluatedProperties: false
enforced on that endpoint.

In addition, in allOf below, the else branch when port-select-gpios is absent
only prohibits usb-role-switch and mux-state under patternProperties
^endpoint@[01]$, so placing usb-role-switch, mux-state, or arbitrary
undocumented properties inside an unnumbered port@0/endpoint node will
silently pass schema validation.

>  allOf:
>    - $ref: usb-drd.yaml#
>    - if:
> @@ -26,8 +58,83 @@ allOf:
>        $ref: usb.yaml#
>      else:
>        $ref: usb-xhci.yaml#
> +  - if:
> +      required: [port-select-gpios]
> +    then:
> +      required: [dr_mode, ports]
> +      properties:
> +        dr_mode:
> +          const: otg
> +        ports:
> +          required: [port@0]
> +          properties:
> +            port@0:
> +              required: ['#address-cells', '#size-cells', endpoint@0, endpoint@1]
> +              properties:
> +                endpoint@0:
> +                  required: [usb-role-switch, mux-state]
> +                endpoint@1:
> +                  required: [usb-role-switch, mux-state]

[Severity: Medium]
When port-select-gpios is present in allOf, should the then branch also
require top-level usb-role-switch on the controller node and disallow the
mutually exclusive single-connector port and connector properties?

In the DWC3 driver (even with the follow-up commit "usb: dwc3: support two
connectors sharing one HS data bus"), role-switch initialization in
dwc3_get_dr_mode(), dwc3_get_extcon(), and dwc3_drd_init() is gated on
device_property_read_bool(dwc->dev, "usb-role-switch") on the controller node
itself:

  dwc3_drd_init()
    if (device_property_read_bool(dwc->dev, "usb-role-switch"))
      dwc3_setup_role_switch()
        dwc3_role_mux_init()

A device tree node that only specifies usb-role-switch inside endpoint@0 and
endpoint@1 as required by this schema will pass validation, but
dwc3_setup_role_switch() -> dwc3_role_mux_init() will never be called at
runtime.

Also, should this binding use the standard multiplexer consumer properties
(mux-controls / mux-states from Documentation/devicetree/bindings/mux/mux-consumer.yaml)
instead of introducing custom unprefixed port-select-gpios and mux-state
properties, and avoid referencing the Linux-specific helper
usb_role_switch_at_endpoint() in $defs/hs-endpoint?

> +    else:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              patternProperties:
> +                '^endpoint@[01]$':
> +                  properties:
> +                    usb-role-switch: false
> +                    mux-state: false
> +  - if:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@0:
> +                  properties:
> +                    mux-state:
> +                      const: 0
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@1:
> +                  properties:
> +                    mux-state:
> +                      const: 1
> +  - if:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@1:
> +                  properties:
> +                    mux-state:
> +                      const: 0
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@0:
> +                  properties:
> +                    mux-state:
> +                      const: 1

[Severity: Medium]
Can a device tree node set mux-state = <1> on both endpoint@0 and endpoint@1
and still pass validation?

Both if-then blocks in allOf check for const: 0 in their if condition:
- if endpoint@0/mux-state is 0, then endpoint@1/mux-state must be 1
- if endpoint@1/mux-state is 0, then endpoint@0/mux-state must be 1

Over enum: [0, 1], the second rule is the contrapositive of the first, so
both rules only reject the case where both endpoints set mux-state = <0>.
When both endpoint@0 and endpoint@1 specify mux-state = <1>, both if clauses
evaluate to false and the duplicate mux-state = <1> configuration passes
dt_binding_check.

Additionally, because these if schemas do not include required guards along
the ports -> port@0 -> endpoint@0/1 -> mux-state path, an absent property
causes the if block to match vacuously. Would checking endpoint@0 for
const: 0 (requiring endpoint@1 const: 1) and const: 1 (requiring endpoint@1
const: 0), or using oneOf under the port-select-gpios then block, avoid this?

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

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

[sashiko-bot]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] usb: dwc3: recursive spinlock deadlock on dwc->lock in dwc3_mux_role_set()
- [High] usb: dwc3: role mux unreachable without controller usb-role-switch property
- [High] usb: dwc3: init race and missing mode cleanup in dwc3_role_mux_init()
- [High] usb: dwc3: SUSPHY not cleared in dwc3_set_prtcap() on same-role mux switch
- [High] usb: dwc3: dwc3_mux_role_get() hides pending role on inactive port
- [Medium] usb: dwc3: fwnode_handle leak on error path in dwc3_role_mux_init()
- [Medium] usb: dwc3: broken and racy mux->error reporting in dwc3_mux_role_set()
- [Medium] usb: dwc3: missing dwc3_pre_set_role() call in dwc3_mux_role_set()
--

commit 39b70abd190f294a50ef9e95121a523aa5bca7ff
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

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

This commit adds support for a single DWC3 controller servicing two
connectors that share a muxed High Speed data bus via a port-select GPIO.
It registers a separate usb_role_switch for each connector endpoint and
serializes role and data-path transitions through __dwc3_set_mode().

> 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

[ ... ]

> @@ -184,7 +189,8 @@ static void __dwc3_set_mode(struct work_struct *work)
>  	if (!desired_dr_role)
>  		goto out;
>  
> -	if (desired_dr_role == dwc->current_dr_role)
> +	if (desired_dr_role == dwc->current_dr_role &&
> +	    (!mux || mux_port == mux->active_port))
>  		goto out;
>  
>  	if (desired_dr_role == DWC3_GCTL_PRTCAP_OTG && dwc->edev)
> @@ -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);
> +		}
> +	}
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
>  	dwc3_set_prtcap(dwc, desired_dr_role, false);

[Severity: High]
When switching between the two mux ports while keeping the same DRD role
(desired_dr_role == dwc->current_dr_role and mux_port != mux->active_port),
__dwc3_set_mode() tears down the previous role via dwc3_gadget_exit() or
dwc3_host_exit(), both of which call dwc3_enable_susphy(dwc, true) to set
GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY.

However, dwc3_set_prtcap() only clears SUSPHY when the PRTCAP mode actually
changes:

drivers/usb/dwc3/core.c:dwc3_set_prtcap() {
    ...
	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD && !ignore_susphy) {
		if (DWC3_GCTL_PRTCAP(reg) != mode)
			dwc3_enable_susphy(dwc, false);
	}
    ...
}

Because DWC3_GCTL_PRTCAP(reg) already equals mode during a same-role port
switch, dwc3_enable_susphy(dwc, false) is skipped, and __dwc3_set_mode()
proceeds to run dwc3_core_soft_reset() and dwc3_gadget_init() or
dwc3_host_init() with PHY suspend still enabled.  Won't leaving SUSPHY
enabled across soft reset and re-initialization cause PHY clock
synchronization and DCTL.CSFTRST timeouts on same-role connector switches?

> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index f3e37d383627a..20821d6505114 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c

[ ... ]

> @@ -501,10 +502,219 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)

[ ... ]

> +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, ret;
> +
> +	if (role > USB_ROLE_DEVICE)
> +		return -EINVAL;
> +
> +	/*
> +	 * The request order and the mode request it implies are decided under one
> +	 * lock, so a second connector cannot change desired_port between the
> +	 * selection and the dwc3_set_mode() call that consumes it.
> +	 */
> +	mutex_lock(&mux->lock);
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	/*
> +	 * A role change between two non-NONE roles of the same connector must not
> +	 * give that connector a new request ticket: it would otherwise steal the
> +	 * data path from the connector that asked first.
> +	 */
> +	if (port->role == USB_ROLE_NONE && role != USB_ROLE_NONE)
> +		port->seq = ++mux->seq;
> +	port->role = role;
> +	selected = dwc3_role_mux_pick(mux);
> +	mux->desired_port = selected;
> +	selected_role = mux->ports[selected].role;
> +	/*
> +	 * Same asynchronous semantics as the single-connector role switch: the
> +	 * data path and the controller are reprogrammed from __dwc3_set_mode().
> +	 * dwc3_set_mode() only queues that work, so the mux lock may be held.
> +	 */
> +	dwc3_set_mode(dwc, dwc3_role_mux_mode(dwc, selected_role));

[Severity: High]
Doesn't calling dwc3_set_mode() while holding dwc->lock deadlock on the
first role switch request?

dwc3_mux_role_set() acquires spin_lock_irqsave(&dwc->lock, flags) above and
then calls dwc3_set_mode(), which acquires dwc->lock again:

drivers/usb/dwc3/core.c:dwc3_set_mode() {
	unsigned long flags;

	if (dwc->dr_mode != USB_DR_MODE_OTG)
		return;

	spin_lock_irqsave(&dwc->lock, flags);
	dwc->desired_dr_role = mode;
	spin_unlock_irqrestore(&dwc->lock, flags);

	queue_work(system_freezable_wq, &dwc->drd_work);
}

[Severity: Medium]
Should dwc3_mux_role_set() also notify glue drivers via
dwc3_pre_set_role(dwc, selected_role) before calling dwc3_set_mode(), just
as dwc3_usb_role_switch_set() does?

Without dwc3_pre_set_role(), glue drivers implementing glue_ops->pre_set_role
(such as dwc3-qcom, dwc3-imx, and dwc3-imx8mp) will not update their VBUS
override or runtime PM autosuspend state when roles change through the mux.
Note that dwc3_pre_set_role() must be called outside dwc->lock because
dwc3_qcom_set_role_notifier() calls pm_runtime_resume_and_get().

> +	ret = mux->error;
> +	mux->error = 0;
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +	mutex_unlock(&mux->lock);
> +
> +	return ret;
> +}

[Severity: Medium]
Does returning a previous request's mux->error from dwc3_mux_role_set()
work as expected with usb_role_switch_set_role()?

In dwc3_mux_role_set(), port->role, mux->desired_port, and dwc3_set_mode()
have already been updated and queued for the new request, but if mux->error
was set by an earlier gpiod_direction_output() failure in __dwc3_set_mode(),
returning that non-zero value causes usb_role_switch_set_role() to treat
the newly queued transition as failed:

drivers/usb/roles/class.c:usb_role_switch_set_role() {
    ...
	ret = sw->set(sw, role);
	if (!ret) {
		sw->role = role;
		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
	}
    ...
}

In addition, because mux->error is a single field shared by both ports, an
asynchronous GPIO failure caused by one port is reported back to whichever
port happens to call usb_role_switch_set_role() next, and __dwc3_set_mode()
writes mux->error without holding dwc->lock or mux->lock, which can race
with the non-atomic read-and-clear here.

> +
> +static enum usb_role dwc3_mux_role_get(struct usb_role_switch *sw)
> +{
> +	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 role = USB_ROLE_NONE;
> +
> +	/*
> +	 * A connector only reports its own role while it owns the shared data
> +	 * path, so the connector drivers never fight over the controller.
> +	 */
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	if (mux->active_port == port->state)
> +		role = port->role;
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	return role;
> +}

[Severity: High]
What happens when a connector driver queries usb_role_switch_get_role() on
disconnect while the other port owns the mux (or while __dwc3_set_mode() is
still queued)?

For example, if port 0 is active (mux->active_port == 0) and port 1 connects
and requests a role, dwc3_mux_role_set() records mux->ports[1].role and
assigns mux->ports[1].seq while mux->active_port remains 0.  If port 1 is
then unplugged before port 0, connector drivers that check the current role
before clearing it—such as cd321x_update_work() in
drivers/usb/typec/tipd/core.c:

drivers/usb/typec/tipd/core.c:cd321x_update_work() {
    ...
	enum usb_role old_role = usb_role_switch_get_role(tps->role_sw);
    ...
	if (old_role != USB_ROLE_NONE && (new_role != old_role || was_disconnected))
		usb_role_switch_set_role(tps->role_sw, USB_ROLE_NONE);
    ...
	if (!new_connected)
		return;
}

will receive USB_ROLE_NONE from dwc3_mux_role_get() and skip calling
usb_role_switch_set_role(sw, USB_ROLE_NONE).  Won't that leave
mux->ports[1].role and mux->ports[1].seq permanently set to the unplugged
request, causing dwc3_role_mux_pick() to switch to the disconnected port 1
once port 0 disconnects and starve any subsequent connection on port 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)
> +{

[ ... ]

> +	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;
> +		}
> +	}
> +
> +	dwc->role_mux = mux;

[Severity: Medium]
If the first endpoint succeeds and takes a reference via
mux->ports[state].fwnode = fwnode_handle_get(ep), but a subsequent endpoint
fails validation (or one of the ports is missing) before dwc->role_mux = mux
is assigned here, doesn't jumping to err_put_ep or err leak
mux->ports[state].fwnode because dwc3_role_mux_exit(dwc) sees
dwc->role_mux == NULL and returns early?

> +
> +	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);
> +		if (IS_ERR(mux->ports[i].sw)) {
> +			ret = PTR_ERR(mux->ports[i].sw);
> +			mux->ports[i].sw = NULL;
> +			goto err;
> +		}
> +	}
> +
> +	/*
> +	 * The controller is only started in its default role once both switches
> +	 * are registered: before that a connector driver cannot request a role,
> +	 * so nothing can race with this initial mode request.
> +	 */
> +	dwc3_set_mode(dwc, mode);
> +
> +	return 1;
> +
> +err:
> +	dwc3_role_mux_exit(dwc);
> +	dwc->role_mux = NULL;
> +	/* A mode request queued before the failure must not run on freed state. */
> +	cancel_work_sync(&dwc->drd_work);
> +	return ret;
> +err_put_ep:
> +	fwnode_handle_put(ep);
> +	goto err;
> +}

[Severity: High]
Can a connector driver request a role as soon as usb_role_switch_register()
publishes mux->ports[0].sw or mux->ports[1].sw, before dwc3_set_mode(dwc, mode)
runs?

If a connector calls dwc3_mux_role_set() right after usb_role_switch_register()
returns, two problems can occur:

1. On the normal path, the unlocked dwc3_set_mode(dwc, mode) call here can
overwrite the connector's dwc->desired_dr_role back to the default mode while
port->role remains set to the connector's requested role.

2. On the error path (if registering ports[0].sw succeeds, port 0 queues
drd_work, and registering ports[1].sw fails), err sets dwc->role_mux = NULL
before cancel_work_sync(&dwc->drd_work) (racing with __dwc3_set_mode()), and
if __dwc3_set_mode() already ran and initialized host or gadget mode via
dwc3_host_init() or dwc3_gadget_init(), dwc3_core_probe() jumps to
err_exit_debugfs without calling dwc3_core_exit_mode(), leaving the xHCI
device or gadget registered after probe fails.

> +
>  static int dwc3_setup_role_switch(struct dwc3 *dwc)
>  {
>  	struct usb_role_switch_desc dwc3_role_switch = {NULL};
>  	u32 mode;
> +	int ret;
>  
>  	dwc->role_switch_default_mode = usb_get_role_switch_default_mode(dwc->dev);
>  	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) {
> @@ -513,6 +723,11 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
>  		dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
>  		mode = DWC3_GCTL_PRTCAP_DEVICE;
>  	}
> +
> +	ret = dwc3_role_mux_init(dwc, mode);
> +	if (ret)
> +		return ret < 0 ? ret : 0;

[Severity: High]
How is dwc3_role_mux_init() reached when the Device Tree follows the
two-connector binding?

In Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml (and as checked
in dwc3_role_mux_init() above), when port-select-gpios is present, the
usb-role-switch property is placed on the child graph endpoints
(ports/port@0/endpoint@0 and endpoint@1) rather than on the controller node.

However, dwc3_setup_role_switch() is only called from dwc3_drd_init() when
the controller node itself has the usb-role-switch property:

drivers/usb/dwc3/drd.c:dwc3_drd_init() {
	if (ROLE_SWITCH &&
	    device_property_read_bool(dwc->dev, "usb-role-switch"))
		return dwc3_setup_role_switch(dwc);
    ...
}

And both dwc3_get_dr_mode() and dwc3_get_extcon() in drivers/usb/dwc3/core.c
likewise only check device_property_read_bool(dwc->dev, "usb-role-switch")
on the controller device (causing dwc3_get_dr_mode() to force dr_mode from
USB_DR_MODE_OTG to USB_DR_MODE_PERIPHERAL on DWC_usb3 >= 3.30a and
DWC_usb31/32).  Without top-level checks for port-select-gpios (or endpoint
usb-role-switch properties), won't dwc3_role_mux_init() be unreachable on
boards using a schema-compliant Device Tree?

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