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

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

[Qianhui Xu]

v1: <20261013102927.1141696-1-xiaqinghuan@mail.ustc.edu.cn>

Changes in v2, following the review of v1:

- Drop "dt-bindings: usb: snps,dwc3: document usb-psy-name" entirely.
  Krzysztof is right: the property is not a DT convention -- a supply is
  referenced by a phandle, not by a name.  The driver does read the string
  (drivers/usb/dwc3/core.c: dwc3_vbus_draw_work() ->
  device_property_read_string(dwc->dev, "usb-psy-name", ...)), but that is a
  legacy/ACPI-shaped lookup that the board can avoid, so documenting it as a
  generic DWC3 binding property was the wrong call.  Nothing in this series
  needs it.  (For reference: on the affected board the same current limit is
  reachable through the standard "power-supplies" phandle on the connector,
  and I will follow up separately if a binding is really warranted.)

- binding: no longer touches usb-psy-name at all.  In v1 the patch that
  documented the property and the patch that described the mux raced in
  ordering, so the second patch read as deleting what the first one added.
  Krzysztof's "No, you just added it!" was a correct reading of that diff.

- driver: a failed GPIO write no longer aborts __dwc3_set_mode() half-way.
  Reported by sashiko-ai: the mode change tears down the previous host or
  gadget instance before programming the new role, so bailing out between the
  two left a torn-down role behind with a stale current_dr_role, and the next
  request would tear it down a second time (double free / use-after-free).

- driver: no more flush_work() on the freezable workqueue.  Reported by
  sashiko-ai: flushing system_freezable_wq from the role-switch path can
  deadlock against system suspend.  The mux now keeps exactly the same
  asynchronous semantics as the existing single-connector role switch --
  dwc3_set_mode() queues the work, __dwc3_set_mode() is the only place that
  programs hardware.

- driver: dropped the "mux_role" variable that was set but never used,
  reported by kernel test robot (arm-defconfig, clang, W=1).

- driver: "oldest request wins" is now actually implemented, with a
timestamp
  per connector taken at the USB_ROLE_NONE -> role transition.  v1 selected
  port 0 first, i.e. static priority, which contradicted its own commit
  message.  Reported by sashiko-ai.

- driver: the runtime-PM reference is no longer taken on a path that can
jump
  to the error label before the reference is acquired (sashiko-ai).

Konrad: to answer your question about the wiring -- this is a real board
topology, not a hub.  Lenovo Legion Y700 (4th gen, TB322FC, SM8750P) has
two
USB-C receptacles driven by a single dwc3 instance (a600000.usb).  The
Super
Speed pairs are not shared: each receptacle has its own SS PHY pair.  Only
the
High Speed D+/D- lines are routed through one 2:1 mux, selected by TLMM
GPIO
29, and each receptacle keeps its own Type-C PD/role input (both connectors
report data-role = "dual" through pmic-glink).  So the controller can be a
host on one receptacle while the other is charging-only, and plugging a
device
into the other receptacle has to be able to steal the controller.  Without
the
mux support, the second receptacle's role switch is simply not registered
and
nothing happens.

Relevant excerpt from the board DTS ("ports" endpoints carry the mux-state
level of their own connector):

  &usb {
          dr_mode = "otg";
          usb-role-switch;
          port-select-gpios = <&tlmm 29 GPIO_ACTIVE_HIGH>;
          ports {
                  port@0 {                /* long-edge receptacle */
                          reg = <0>;
                          usb_dwc3_hs: endpoint {
                                  remote-endpoint = <&pmic_glink_hs_in>;
                                  usb-role-switch;
                                  mux-state = <0>;
                          };
                  };
                  port@2 {                /* short-edge receptacle */
                          reg = <2>;
                          usb_dwc3_hs2: endpoint {
                                  remote-endpoint = <&pmic_glink_hs_c2_in>;
                                  usb-role-switch;
                                  mux-state = <1>;
                          };
                  };
          };
  };

Testing (all on the board, 7.3-rc7 based tree):
 - two receptacles: host mode on either receptacle works, and moving the
   cable from one to the other hands the controller over;
 - charging-only on one receptacle while the other has a device attached;
 - single-connector boards: the mux path is only taken when
   "port-select-gpios" is present, otherwise the existing role switch is used
   unchanged.

Qianhui Xu (2):
  dt-bindings: usb: snps,dwc3: describe the two-connector HS data path
  usb: dwc3: support two connectors sharing one HS data bus

 .../bindings/usb/snps,dwc3-common.yaml        |  63 +++++-
 drivers/usb/dwc3/core.c                       |  29 ++-
 drivers/usb/dwc3/core.h                       |  38 ++++
 drivers/usb/dwc3/debugfs.c                    |   4 +
 drivers/usb/dwc3/drd.c                        | 199 +++++++++++++++++-
 5 files changed, 330 insertions(+), 3 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] dt-bindings: usb: snps,dwc3: describe the two-connector HS data path

[Qianhui Xu]

Some designs wire the High Speed data lines of a single DWC3 controller to
two receptacles and select between them with a GPIO, while the Super Speed
lanes stay per connector.

Describe that in snps,dwc3-common.yaml:

  - port-select-gpios selects which connector drives the shared HS bus;
  - each endpoint of "ports" carries "mux-state", the logical port-select
    level of that connector, and marks itself as a role-switch input with
    "usb-role-switch".

The connector endpoints are restricted to port-base/endpoint-base so that
endpoint subnodes cannot carry unrelated properties.

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

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
index 6c0b8b653..3d012f222 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
@@ -13,6 +13,34 @@ description:
   Defines the properties of the DWC3 core as being embedded in either an
   vendor-specific implementation or as a standalone component.
 
+$defs:
+  hs-port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/graph.yaml#/$defs/endpoint-base
+        unevaluatedProperties: false
+
+        properties:
+          usb-role-switch:
+            type: boolean
+            description:
+              This connector provides its own role-switch input to the
+              controller.
+
+          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]
+
 allOf:
   - $ref: usb-drd.yaml#
   - if:
@@ -26,8 +54,37 @@ allOf:
       $ref: usb.yaml#
     else:
       $ref: usb-xhci.yaml#
+  - if:
+      required: [port-select-gpios]
+    then:
+      required: [dr_mode, usb-role-switch]
+      properties:
+        dr_mode:
+          const: otg
+        ports:
+          required: [port@0, port@2]
+          properties:
+            port@0:
+              required: [endpoint]
+              properties:
+                endpoint:
+                  required: [usb-role-switch, mux-state]
+            port@2:
+              required: [endpoint]
+              properties:
+                endpoint:
+                  required: [usb-role-switch, mux-state]
 
 properties:
+  port-select-gpios:
+    maxItems: 1
+    description:
+      Selects which of two connectors drives the controller's shared High
+      Speed data bus. Each connector is described by a "ports" endpoint that
+      carries its own role-switch input and the "mux-state" level selecting
+      it. The controller switches the data path only after it has stopped the
+      role of the previous connector.
+
   extcon:
     maxItems: 1
     deprecated: true
@@ -411,13 +468,17 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: '#/$defs/hs-port'
         description: High Speed (HS) data bus.
 
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description: Super Speed (SS) data bus.
 
+      port@2:
+        $ref: '#/$defs/hs-port'
+        description: Second connector on the multiplexed High Speed data bus.
+
   wakeup-source:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.53.0

[PATCH v2 2/2] 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 in
"ports"/"mux-state" when "port-select-gpios" is present.  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.

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>
---
 drivers/usb/dwc3/core.c    |  29 +++++-
 drivers/usb/dwc3/core.h    |  38 +++++++
 drivers/usb/dwc3/debugfs.c |   4 +
 drivers/usb/dwc3/drd.c     | 199 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 268 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index fd5c2cd36..6607e65e8 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 f3dabb37f..6312c1ed6 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1008,6 +1008,42 @@ 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 long		stamp;
+	unsigned int		state;
+};
+
+/**
+ * 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_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
+ *
+ * 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];
+	enum usb_role		desired_role;
+	int			desired_port;
+	int			active_port;
+	int			error;
+};
+
 /**
  * struct dwc3 - representation of our controller
  * @drd_work: workqueue used for role swapping
@@ -1064,6 +1100,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 +1302,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 65d909e4b..0d42f1098 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 f3e37d383..491047796 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;
+	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));
+
+	return 0;
+}
+
+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;
+	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);
+		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;
+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 +703,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 +721,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 +736,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 +793,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 v2 1/2] dt-bindings: usb: snps,dwc3: describe the two-connector HS data path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] dt-bindings: usb: snps,dwc3: board-level HS GPIO mux modeled in controller
- [Medium] dt-bindings: usb: snps,dwc3: port@2 models second connector as controller bus
- [Medium] dt-bindings: usb: snps,dwc3: usb-role-switch placed on graph endpoint node
- [Medium] dt-bindings: usb: snps,dwc3: port-select-gpios conditional omits required ports
- [Medium] dt-bindings: usb: snps,dwc3: schema allows identical mux-state on both ports
--

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

dt-bindings: usb: snps,dwc3: describe the two-connector HS data path

This patch updates Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
to describe a shared High Speed data bus connected to two receptacles via a
GPIO-controlled selector, adding port-select-gpios, port@2, and endpoint
properties mux-state and usb-role-switch.

> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> index 6c0b8b6538246..3d012f22231cb 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> @@ -13,6 +13,34 @@ description:
>    Defines the properties of the DWC3 core as being embedded in either an
>    vendor-specific implementation or as a standalone component.
>  
> +$defs:
> +  hs-port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +        unevaluatedProperties: false
> +
> +        properties:
> +          usb-role-switch:
> +            type: boolean
> +            description:
> +              This connector provides its own role-switch input to the
> +              controller.
> +
> +          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]

[Severity: Medium]
Does usb-role-switch belong on the OF graph endpoint subnode here?

In Documentation/devicetree/bindings/usb/usb-drd.yaml, usb-role-switch is a
device-level boolean property indicating that the controller node registers a
USB role switch.

When a connector driver resolves its role switch over the OF graph via:

  usb_role_switch_get() / fwnode_usb_role_switch_get()
    -> fwnode_graph_devcon_matches()
      -> fwnode_graph_get_remote_port_parent()
        -> of_fwnode_graph_get_port_parent()

the lookup walks from the remote endpoint up past port@N and ports to the
parent controller device node, and checks:

  if (!fwnode_property_present(node, "usb-role-switch"))
          continue;

on the parent device node rather than the endpoint subnode.

In addition, because dependentRequired under $defs:hs-port:properties:endpoint
requires usb-role-switch whenever mux-state is present and vice versa,
isn't usb-role-switch on the endpoint completely redundant with mux-state?

> +
>  allOf:
>    - $ref: usb-drd.yaml#
>    - if:
> @@ -26,8 +54,37 @@ allOf:
>        $ref: usb.yaml#
>      else:
>        $ref: usb-xhci.yaml#
> +  - if:
> +      required: [port-select-gpios]
> +    then:
> +      required: [dr_mode, usb-role-switch]
> +      properties:
> +        dr_mode:
> +          const: otg
> +        ports:
> +          required: [port@0, port@2]
> +          properties:
> +            port@0:
> +              required: [endpoint]
> +              properties:
> +                endpoint:
> +                  required: [usb-role-switch, mux-state]
> +            port@2:
> +              required: [endpoint]
> +              properties:
> +                endpoint:
> +                  required: [usb-role-switch, mux-state]

[Severity: Medium]
Since ports is optional at the top level of snps,dwc3-common.yaml (where
required only lists compatible and reg), will a node with port-select-gpios,
dr_mode = "otg", and usb-role-switch that omits the ports node altogether
silently pass dt_binding_check?

In JSON Schema, properties:ports: only applies if the ports child node is
present. Should ports also be listed in then:required: alongside dr_mode and
usb-role-switch?

Conversely, without an else branch or dependency, won't port@2 and the
endpoint mux-state and usb-role-switch properties remain allowed even when
port-select-gpios is absent?

[Severity: Medium]
Can a device tree specify the same mux-state value (for example, both <0> or
both <1>) on ports/port@0/endpoint and ports/port@2/endpoint and still pass
schema validation?

Both port@0 and port@2 reference #/$defs/hs-port, which independently checks
enum: [0, 1] on each endpoint, without constraining the two endpoints to use
distinct GPIO levels.

>  
>  properties:
> +  port-select-gpios:
> +    maxItems: 1
> +    description:
> +      Selects which of two connectors drives the controller's shared High
> +      Speed data bus. Each connector is described by a "ports" endpoint that
> +      carries its own role-switch input and the "mux-state" level selecting
> +      it. The controller switches the data path only after it has stopped the
> +      role of the previous connector.

[Severity: Medium]
Should a board-level GPIO multiplexer on the High Speed D+/D- lines be modeled
using the existing mux bindings instead of adding controller-specific
port-select-gpios and endpoint mux-state properties to snps,dwc3-common.yaml?

The GPIO switches external board routing between two receptacles rather than
an internal block of the Synopsys DWC3 IP core, and the description also
encodes driver switching policy ("The controller switches the data path only
after it has stopped the role of the previous connector").

Could this hardware topology be represented using
Documentation/devicetree/bindings/mux/gpio-mux.yaml (mux-gpios),
Documentation/devicetree/bindings/mux/mux-consumer.yaml (mux-controls /
mux-states), or Documentation/devicetree/bindings/usb/usb-switch.yaml?

> +
>    extcon:
>      maxItems: 1
>      deprecated: true
> @@ -411,13 +468,17 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: '#/$defs/hs-port'
>          description: High Speed (HS) data bus.
>  
>        port@1:
>          $ref: /schemas/graph.yaml#/properties/port
>          description: Super Speed (SS) data bus.
>  
> +      port@2:
> +        $ref: '#/$defs/hs-port'
> +        description: Second connector on the multiplexed High Speed data bus.
> +

[Severity: Medium]
Does adding port@2 under properties:ports conflate the DWC3 controller's own
hardware buses with remote connectors?

In snps,dwc3-common.yaml, the children of ports represent buses on the
controller itself (port@0 for the High Speed data bus and port@1 for the Super
Speed data bus). When a single High Speed bus is routed to two remote targets,
OF graph bindings normally model either an intermediate mux node in the graph
or multiple numbered endpoints (endpoint@0, endpoint@1) under port@0.

Also, by changing port@0 from /schemas/graph.yaml#/properties/port to
#/$defs/hs-port (which uses /schemas/graph.yaml#/$defs/port-base with
unevaluatedProperties: false and only defines an unnumbered endpoint node),
won't this reject numbered endpoint@N children under port@0, while leaving
port@1 (Super Speed) as a single port even though the commit message notes
that Super Speed lanes stay per connector?

>    wakeup-source:
>      $ref: /schemas/types.yaml#/definitions/flag
>      description:

Base commit: 90d9f76ccb72ec024ef476b4229f3dd1957d7b26
-- 
2.51.0

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

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

[sashiko-bot]

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

[RFC PATCH v3 0/2] USB: describe two connectors sharing one DWC3 HS data bus

[Qianhui Xu]

This is v3 of the series describing two USB-C receptacles that share one
DWC3 High Speed data bus.  It is deliberately an RFC for the shape: the
driver part is not included yet.

Changes since v2:
- Drop port@2.  The shared HS bus is now port@0 with two numbered
  endpoints, endpoint@0 and endpoint@1, so "ports" keeps describing the
  controller's own buses.
- Require "ports" when port-select-gpios is present, and reject the
  endpoint properties when it is not.
- Constrain the two mux-state values to be complementary.
- Move the per-connector role-switch lookup into a generic helper,
  usb_role_switch_at_endpoint() (new patch 2/2).  v2 relied on the
  lookup walking from the endpoint up to the parent device, which is not
  what usb_role_switch_get() does upstream, so a consumer would never
  find the switch.  With the helper the switch is registered against the
  remote endpoint and found from the connector's endpoint.
- Do not describe driver switching policy in the binding.

Two questions for you:

1. Why not the standard mux bindings?
   The GPIO only routes the HS D+/D- pair.  What the driver needs is role
   arbitration between two connectors (the oldest active request owns the
   data path, and a connector reports its role only while it owns the
   path).  The mux subsystem can select a state but has no notion of
   ownership or of USB roles, and drivers/usb/ has no mux_control
   consumer today.  gpio-mux also drives its mux-gpio to the idle state
   at probe time, which would override the level left by the bootloader
   on this board.  So the arbitration stays in the controller and the
   binding only describes the wiring.  If you would still prefer the mux
   bindings we will rework it -- that is why this is RFC.

2. Is "usb-role-switch" on a graph endpoint acceptable, given that the
   property normally means "this device registers a role switch"?  Here
   it declares that this connector endpoint has its own role switch, and
   the new helper looks it up from the connector's endpoint.  A separate
   property name is possible if you prefer.

The board is the same as in v2: two USB-C receptacles, Super Speed pairs
independent, only the HS D+/D- pair goes through a 2:1 mux controlled by
TLMM GPIO 29; both connectors have data-role = "dual" and are handled by
pmic-glink.

Once the shape is acked I will send the DWC3 driver part as v4, including
the fixes from the review of v2.

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

 .../bindings/usb/snps,dwc3-common.yaml        | 109 +++++++++++++++++-
 drivers/usb/roles/class.c                     |  40 +++++++
 2 files changed, 148 insertions(+), 1 deletion(-)

-- 
2.53.0

[RFC PATCH v3 1/2] 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 6c0b8b653..6b06b0027 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

[RFC PATCH v3 2/2] 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 edec139b6..fc739fd61 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

Re: [RFC PATCH v3 2/2] 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: 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

Re: [RFC PATCH v3 1/2] 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: hs-port rejects unnumbered endpoint in port@0
- [Medium] dt-bindings: usb: snps,dwc3: mux-state conditionals allow duplicate state 1
- [Low] dt-bindings: usb: custom mux properties and usb_role_switch_at_endpoint() ref
--

commit b06ffa61c11c4632e31e6abcd1a688344b43ffab
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 bus of a single DWC3
controller via a GPIO selector and numbered endpoints under port@0.

> 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.

[Severity: Low]
In $defs/hs-endpoint, the usb-role-switch description references the Linux
kernel internal function usb_role_switch_at_endpoint(). Since
Documentation/devicetree/bindings/writing-bindings.rst advises against
referring to Linux or driver implementation details in hardware bindings,
should this function reference be removed from the description?

Also, the GPIO-controlled switch routing the shared High-Speed D+/D- pair
between two connectors appears to be an external board-level multiplexer.
Could this use the standard multiplexer bindings from
Documentation/devicetree/bindings/mux/gpio-mux.yaml and
Documentation/devicetree/bindings/mux/mux-consumer.yaml (such as mux-controls
and mux-states) rather than introducing custom port-select-gpios and singular
mux-state properties on the DWC3 controller and its endpoints?

> +
> +    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 with #/$defs/hs-port on
ports/properties/port@0 cause a dtbs_check validation regression for existing
single-connector DWC3 device trees?

Previously, port@0 referenced /schemas/graph.yaml#/properties/port, which
allows both an unnumbered endpoint node and numbered endpoint@N child nodes.
Here, $defs/hs-port references /schemas/graph.yaml#/$defs/port-base with
unevaluatedProperties: false and only matches ^endpoint@[01]$ in
patternProperties.

When a single-connector DWC3 node without port-select-gpios defines an
unnumbered endpoint child node under port@0 (such as in
arch/arm64/boot/dts/qcom/sm8550.dtsi,
arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts, or
arch/arm64/boot/dts/apple/t8103-jxxx.dtsi), won't that unnumbered endpoint
node now be rejected as an unevaluated property?

> +
>  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

[Severity: Medium]
Do these two if-then blocks in allOf allow both endpoint@0 and endpoint@1 to
set mux-state = <1> at the same time?

Both if conditions test for const: 0 (on endpoint@0 and endpoint@1
respectively). When both endpoints set mux-state = <1>, neither if condition
matches const: 0, so neither then branch is enforced and the duplicate
mux-state = <1> configuration passes validation.

Also, because the if blocks do not include required constraints at each nested
level (ports, port@0, endpoint@0/endpoint@1, and mux-state), won't the if
conditions match vacuously when those nodes or properties are absent?

[ ... ]

> @@ -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:

[ ... ]

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