[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

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

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