[PATCH 0/3] usb: dwc3: two Type-C connectors sharing one HS data bus

[PATCH 0/3] usb: dwc3: two Type-C connectors sharing one HS data bus

[Qianhui Xu]

*** AI-assistance disclosure ***
This cover letter and the patches below were prepared with the help of an
LLM coding assistant.  The author reviewed every line, ran every gate
quoted in this letter himself, and takes full responsibility for the
content.

This is one complete, self-contained series of three patches: the binding,
the role-switch lookup it needs, and the DWC3 arbitration between the two
connectors.  Nothing is to be applied on top of it, and no patch of it is
posted separately.  It supersedes the v4 series
(<20261026123211.243224-1-xiaqinghuan@mail.ustc.edu.cn>) and the v5
fixups (<20261031150154.1106455-1-xiaqinghuan@mail.ustc.edu.cn>) in
full: their content is folded into the three patches below.

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

The board (Lenovo Legion Y700 4th gen, SM8750) wires the Super Speed pairs
of two USB-C receptacles to separate PHYs, while the High Speed D+/D- pair
is shared through a 2:1 mux controlled by a single GPIO.  Both connectors
are data-role dual and hand their role to the SoC through pmic-glink, so
the controller has to arbitrate between two independent role requests and
move the shared data path before it is reinitialised.

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

		ports {
			port@0 {			/* HS path, connector 0 */
				endpoint {
					remote-endpoint = <&usb_0_dwc3_hs>;
					usb-role-switch;
					mux-state = <0>;
				};
			};

			port@1 {			/* Super Speed, QMP PHY */
			};

			port@2 {			/* HS path, connector 1 */
				endpoint {
					remote-endpoint = <&usb_1_dwc3_hs>;
					usb-role-switch;
					mux-state = <1>;
				};
			};
		};
	};

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

1/3 documents that wiring: "port-select-gpios", one High Speed port per
    connector (port@0 and port@2, port@1 being the Super Speed lane), and,
    on the endpoint of each of those ports, "mux-state" (the GPIO level
    that reaches that connector) and "usb-role-switch" (this connector has
    its own role-switch input).

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

3/3 implements the arbitration in DWC3: one role switch per connector, the
    oldest pending request wins, the GPIO is flipped inside
    __dwc3_set_mode() after the previous host or gadget instance is torn
    down, and a connector reads back the role it requested.

Reply to the two procedural comments
====================================

Krishna Kurapati wrote, on the v5 posting: "If v5 is being sent, it must
not to be applied on top of v4.  It must include (v4 + any additional
changes/improvements)."  Agreed, and that is what this series is.  v4
1/3..3/3 and the v5 fixups were squashed per patch instead of stacked:
1/3 = v4 1/3 + v5 1/2, 2/3 = v4 2/3 + the usb_role_switch_at_endpoint()
hunk of v5 2/2, 3/3 = v4 3/3 + the remaining hunks of v5 2/2.  Applying
this series to a v7.3-rc7 tree gives the same three source files as
"v4 + v5", and there is no "1/2 on top of 3/3" left anywhere.

Krzysztof Kozlowski wrote: "Anyway, this is misthreaded - missing rest of
patchset.  Please read submitting patches document and switch to b4", and
separately dropped v4 1/3 ("Unanswered Sashiko comments plus some
confusion with duplicated posting of v5 without the rest, dropping").

Both points are taken.  Posting v5 as a two-patch fixup and then re-sending
only its 1/2 on 11-02 was wrong: a series has to be complete and threaded.
This posting is the complete series as one thread -- cover plus 1/3..3/3,
"base-commit" at the bottom, and In-Reply-To pointing at this cover only,
so it applies with `b4 shazam` (or `git am`) as-is.  Nothing is re-sent
piecemeal; the 11-02 1/2 has no successor and should be ignored.  On the
"git log" remark: the per-revision account now lives only here, the commit
messages describe the code as it finally is.

Dropping v4 1/3 is fine: this 1/3 replaces it, together with the schema
holes that were reported against it.  No action is needed to "un-drop" it.

Response to the review comments folded into these patches
=========================================================

Sashiko on the binding (three Medium findings):

 - "hs-port patternProperties does not match what the driver expects /
   escapes unevaluatedProperties": fixed, the endpoint name pattern is the
   one graph.yaml uses, so an unnumbered endpoint or endpoint@2+ is
   validated too (1/3).

 - "usb-role-switch should not be required only indirectly": fixed, the
   controller node must carry "usb-role-switch" whenever port-select-gpios
   is present, and dr_mode is required to be "otg".  This is a driver
   fact, not schema cosmetics: without it dwc3_get_dr_mode() forces dr_mode
   back to peripheral, dwc3_get_extcon() skips the role-switch branch and
   dwc3_drd_init() never sets the mux up, so a DT that passed the old
   schema did not describe a DT the driver could act on (1/3).

 - "mux-state complementarity is not expressed": fixed, the pair is one
   oneOf over the two assignments, so two connectors both claiming
   mux-state = <1> are rejected (1/3).

 - standing question, "model the board mux with the standard
   mux-controls/mux-states properties": kept the graph-endpoint shape.  The
   arbitration is "earliest request wins" between two connectors that must
   each be a usb role switch consumer, and the endpoint is the fwnode the
   connector is attached to; a mux_control consumer has no per-connector
   identity to hang that on, and drivers/usb/ has no mux_control consumer
   to follow as precedent.  Happy to reshape if the DT maintainers prefer
   the standard property.

Sashiko on usb: roles (one High finding):

 - "-EPROBE_DEFER is swallowed by !IS_ERR_OR_NULL(sw)": fixed, the error is
   passed up, as suggested (2/3).  This was also the root cause we hit on
   real hardware: the UCSI glue kept a NULL switch and the connectors never
   bound.

Sashiko on usb: dwc3 (five High and three Medium findings):

 - "dwc3_set_mode() under dwc->lock self-deadlocks": fixed, the request
   decision and its issue are paired by mux->lock, and dwc->lock is only
   held while individual fields are updated (3/3).

 - "glue layer is never notified": fixed, dwc3_pre_set_role() is called
   with the effective role, outside spinlocks, before dwc3_set_mode(),
   mirroring the single-connector path (3/3).

 - "same-role request that moves the data path to the other connector":
   fixed, dwc3_set_prtcap() only clears SUSPHY when PRTCAP changes, so that
   case now clears it explicitly (3/3).

 - "registration earlier than the initial mode request races with a
   connector driver": fixed, register and request the initial mode under
   mux->lock, tear down in reverse order on failure (3/3).

 - "fwnode references leak on error paths": fixed, dwc->role_mux is
   published before the error paths that used to leak them, so
   dwc3_role_mux_exit() can unwind (3/3).

 - "sticky mux->error is reported to the next caller after it already
   committed its own request": fixed, the sticky value is gone; a failed
   data-path switch is logged and counted per connector (3/3).

 - found while testing on the board, same area: dwc3_mux_role_get()
   returned USB_ROLE_NONE for a connector that did not own the data path,
   which hid its pending request; the connector driver then never sent the
   set_role(USB_ROLE_NONE) that clears it, and the stale request kept
   winning arbitration after unplug.  get() now reports the requested role
   (3/3).

 - "add dwc3_pre_set_role()" (raised against v2): this is exactly what the
   folded code does now, both in the single-connector and the mux path.

Changes relative to the previous postings
=========================================

 - v4 1/3..3/3 -> this series: 5 files changed, 143 insertions(+),
   80 deletions(-) (binding 1/3 and core.c/core.h/drd.c/class.c), which
   is the folded v5 fixup plus one documentation fix: after the reshape to
   port@0 + port@2 the "port-select-gpios" description still said
   "a numbered endpoint of port@0"; it now names port@0/port@2 and their
   endpoint.  Description only, no schema change.
 - v5 1/2..2/2 -> this series: 3 lines of that description, nothing else;
   v5's code changes are bit-identical inside 1/3, 2/3 and 3/3.

Testing
=======

Mechanical, all re-run on this exact series:

 - checkpatch.pl --strict --patch - (mbox on stdin): 0 errors, 0 warnings,
   0 checks for each of the three patches (138 / 63 / 441 lines).
 - dt-doc-validate on snps,dwc3-common.yaml: clean, exit 0.
 - make W=1 ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- for
   drivers/usb/dwc3/ and drivers/usb/roles/ with all 19 dwc3 glue drivers
   built as modules: 0 warnings, 0 errors.
 - git am of 1/3..3/3 on 24a19b953e: applies cleanly patch by patch, and the
   resulting tree is identical to the series tip.
 - scripts/check-series-selfcontained.sh on the series directory against
   the pristine base: PASS, 3/3 patches replayed in order.
 - git send-email --dry-run of the produced mbox set: 1 cover + 3 patches,
   threaded with In-Reply-To on the cover only.

Board behaviour: the same driver source runs on this board's daily kernel
and registers both per-connector switches
(a600000.usb-port0-role-switch, a600000.usb-port1-role-switch).  With
function_graph tracing, 12 consecutive plug/unplug cycles each went
usb_role_switch_set_role -> dwc3_mux_role_set -> glue notifier ->
dwc3_set_mode with zero WARNING/BUG/lockdep splats; USB data and charging
were stable throughout, and /sys/class/typec shows port0, port1 and a
bound partner.

Two honest limitations: (a) the arbitration detail -- which connector wins
a simultaneous request -- is verified by code reading and by the stale
request test above, not by a complete per-port role-value trace; (b) the
behaviour evidence comes from that daily kernel, not from a v7.3-rc7
kernel with this series applied, because pristine-lineage kernels do not
bring this board up yet for reasons unrelated to USB (no display/platform
support).  I can re-run the plug sequence on request.

Links
=====

v4: https://lore.kernel.org/linux-usb/20261026123211.243224-1-xiaqinghuan@mail.ustc.edu.cn/
v5: https://lore.kernel.org/linux-usb/20261031150154.1106455-1-xiaqinghuan@mail.ustc.edu.cn/
Krishna's reply: <ba544091-1da4-4616-b1d5-fe23f32679d9@oss.qualcomm.com>
Krzysztof's reply: <20261102-armored-arcane-serval-43faf4@quoll>
v4 1/3 dropped: <20261102-screeching-utopian-avocet-492500@quoll>

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

 .../bindings/usb/snps,dwc3-common.yaml        | 116 +++++++-
 drivers/usb/dwc3/core.c                       |  47 +++-
 drivers/usb/dwc3/core.h                       |  39 +++
 drivers/usb/dwc3/debugfs.c                    |   4 +
 drivers/usb/dwc3/drd.c                        | 254 +++++++++++++++++-
 drivers/usb/roles/class.c                     |  45 ++++
 6 files changed, 501 insertions(+), 4 deletions(-)

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

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

[Qianhui Xu]

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

Describe that in snps,dwc3-common.yaml:

  - "port-select-gpios" selects which connector drives the shared bus;
  - each connector is a port of the controller's "ports" node, "port@0"
    and "port@2", with one endpoint per port, so that "ports" keeps
    describing the controller's own buses as the graph bindings expect
    (port@1 is the Super Speed lane of the PHY, described by
    snps,dwc3.yaml);
  - the endpoint of a connector carries "mux-state", the level of the
    port-select GPIO that reaches it, and "usb-role-switch", declaring
    that this connector has its own role-switch input.

The controller node must carry "usb-role-switch" as well, and dr_mode is
required to be "otg": dwc3_get_dr_mode() otherwise forces dr_mode back to
peripheral, dwc3_get_extcon() skips the role-switch branch and
dwc3_drd_init() never sets the mux up, so a device tree that passed the
schema would not describe a device tree the driver can act on.

The two connectors must claim opposite levels of the GPIO, which is
expressed as one choice between the two assignments, and endpoint names
use the pattern of graph.yaml so that an unnumbered or third endpoint is
rejected instead of escaping "unevaluatedProperties: false".  When
port-select-gpios is absent the endpoint properties are rejected, so
existing single-connector descriptions are unaffected.

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

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
index 6c0b8b6538..0be2a6e83e 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
@@ -13,6 +13,40 @@ 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.  The two connectors of
+          one controller must use different levels; the driver rejects a
+          device tree where two connectors claim the same one.
+
+    dependentRequired:
+      usb-role-switch: [mux-state]
+      mux-state: [usb-role-switch]
+
+  hs-port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+
+    patternProperties:
+      '^endpoint(@[0-9a-f]+)?$':
+        $ref: '#/$defs/hs-endpoint'
+
 allOf:
   - $ref: usb-drd.yaml#
   - if:
@@ -26,8 +60,87 @@ allOf:
       $ref: usb.yaml#
     else:
       $ref: usb-xhci.yaml#
+  - if:
+      required: [port-select-gpios]
+    then:
+      # The mux path is only taken when the controller itself carries
+      # "usb-role-switch": dwc3_get_dr_mode() forces dr_mode back to
+      # peripheral, dwc3_get_extcon() skips the role-switch branch and
+      # dwc3_drd_init() never calls dwc3_role_mux_init() without it.
+      required: [dr_mode, ports, usb-role-switch]
+      properties:
+        dr_mode:
+          const: otg
+        ports:
+          # Each connector is one port with a single endpoint: the mux-state
+          # property on that endpoint says which level of the port-select GPIO
+          # reaches it, so two connectors are two ports, not two endpoints of
+          # one port.  port@1 is the SuperSpeed lane of the PHY and is already
+          # described by snps,dwc3.yaml, so the second High Speed connector
+          # lands on port@2.
+          required: [port@0, port@2]
+          properties:
+            port@0:
+              $ref: '#/$defs/hs-port'
+            port@2:
+              $ref: '#/$defs/hs-port'
+
+          # The two connectors must claim opposite levels of the port-select
+          # GPIO; requiring both ports in every branch is what makes the two
+          # cases mutually exclusive for oneOf.
+          oneOf:
+            - required: [port@0, port@2]
+              properties:
+                port@0:
+                  $ref: '#/$defs/hs-port'
+                  properties:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 0
+                port@2:
+                  $ref: '#/$defs/hs-port'
+                  properties:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 1
+            - required: [port@0, port@2]
+              properties:
+                port@0:
+                  $ref: '#/$defs/hs-port'
+                  properties:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 1
+                port@2:
+                  $ref: '#/$defs/hs-port'
+                  properties:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 0
+    else:
+      properties:
+        ports:
+          properties:
+            port@0:
+              patternProperties:
+                '^endpoint(@[0-9a-f]+)?$':
+                  properties:
+                    usb-role-switch: false
+                    mux-state: false
 
 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 "port@0" or "port@2",
+      and the endpoint of that port carries the connector's own role-switch
+      input and the "mux-state" level selecting it.
+
   extcon:
     maxItems: 1
     deprecated: true
@@ -411,7 +524,7 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: '#/$defs/hs-port'
         description: High Speed (HS) data bus.
 
       port@1:
@@ -429,4 +542,3 @@ required:
 
 additionalProperties: true
 ...
-

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: usb: snps,dwc3: missing endpoint/mux-state required in oneOf
- [Medium] dt-bindings: usb: snps,dwc3: port@2 missing from top-level ports schema
- [Medium] dt-bindings: usb: snps,dwc3: Linux-specific mux binding and port@2 split
--

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

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

This commit extends snps,dwc3-common.yaml to describe two USB connectors
sharing a single High-Speed bus via a port-select-gpios multiplexer, using
port@0 and port@2 with per-endpoint mux-state and usb-role-switch
properties.

> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> index 6c0b8b6538246..0be2a6e83ee48 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> @@ -13,6 +13,40 @@ 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.  The two connectors of
> +          one controller must use different levels; the driver rejects a
> +          device tree where two connectors claim the same one.
> +
> +    dependentRequired:
> +      usb-role-switch: [mux-state]
> +      mux-state: [usb-role-switch]
> +
> +  hs-port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +
> +    patternProperties:
> +      '^endpoint(@[0-9a-f]+)?$':
> +        $ref: '#/$defs/hs-endpoint'
> +
>  allOf:
>    - $ref: usb-drd.yaml#
>    - if:
> @@ -26,8 +60,87 @@ allOf:
>        $ref: usb.yaml#
>      else:
>        $ref: usb-xhci.yaml#
> +  - if:
> +      required: [port-select-gpios]
> +    then:
> +      # The mux path is only taken when the controller itself carries
> +      # "usb-role-switch": dwc3_get_dr_mode() forces dr_mode back to
> +      # peripheral, dwc3_get_extcon() skips the role-switch branch and
> +      # dwc3_drd_init() never calls dwc3_role_mux_init() without it.
> +      required: [dr_mode, ports, usb-role-switch]
> +      properties:
> +        dr_mode:
> +          const: otg
> +        ports:
> +          # Each connector is one port with a single endpoint: the mux-state
> +          # property on that endpoint says which level of the port-select GPIO
> +          # reaches it, so two connectors are two ports, not two endpoints of
> +          # one port.  port@1 is the SuperSpeed lane of the PHY and is already
> +          # described by snps,dwc3.yaml, so the second High Speed connector
> +          # lands on port@2.
> +          required: [port@0, port@2]
> +          properties:
> +            port@0:
> +              $ref: '#/$defs/hs-port'
> +            port@2:
> +              $ref: '#/$defs/hs-port'
> +
> +          # The two connectors must claim opposite levels of the port-select
> +          # GPIO; requiring both ports in every branch is what makes the two
> +          # cases mutually exclusive for oneOf.
> +          oneOf:
> +            - required: [port@0, port@2]
> +              properties:
> +                port@0:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 0
> +                port@2:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 1
> +            - required: [port@0, port@2]
> +              properties:
> +                port@0:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 1
> +                port@2:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 0

[Severity: Medium]
Does the oneOf block under allOf allow incomplete or duplicate mux-state
configurations to pass schema validation while rejecting numbered
endpoint@0 nodes?

In $defs/hs-endpoint, usb-role-switch and mux-state are only constrained
with dependentRequired, so an endpoint omitting both properties satisfies
hs-endpoint. In addition, neither port@0 nor port@2 in the allOf then
branch specifies required: [endpoint], and the endpoint subschemas inside
the two oneOf branches only specify properties: { mux-state: { const: ... } }
without required: [mux-state].

In JSON Schema, the properties keyword evaluates to true when the target
property is absent. If port@0 has an endpoint with mux-state = <0> and
usb-role-switch, while port@2 omits endpoint (or its endpoint omits both
mux-state and usb-role-switch), the first branch of oneOf evaluates to true
and the second branch evaluates to false, allowing an incomplete device
tree that fails at runtime in dwc3_role_mux_init() to pass validation.

Furthermore, $defs/hs-port defines patternProperties matching
^endpoint(@[0-9a-f]+)?$, whereas the oneOf branches only check the literal
unnumbered property name endpoint:

1. If port@0 uses endpoint with mux-state = <0> and port@2 uses endpoint@0
   with mux-state = <0>, port@2 has no unnumbered endpoint property and
   vacuously matches const: 1 in the first oneOf branch, allowing duplicate
   mux-state levels to pass.
2. If both port@0 and port@2 use endpoint@0 with opposite mux-state values
   0 and 1, neither port has an unnumbered endpoint property, so both
   branches of oneOf evaluate to true and oneOf fails.
3. If a port defines both endpoint and endpoint@1, hs-port accepts both and
   oneOf ignores endpoint@1, even though the commit message states that a
   third endpoint is rejected.

Should port@0 and port@2 require a single unnumbered endpoint (or constrain
endpoint count and naming consistently between hs-port and oneOf) and
require mux-state and usb-role-switch on each endpoint when
port-select-gpios is present?

> +    else:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              patternProperties:
> +                '^endpoint(@[0-9a-f]+)?$':
> +                  properties:
> +                    usb-role-switch: false
> +                    mux-state: false
>  
>  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 "port@0" or "port@2",
> +      and the endpoint of that port carries the connector's own role-switch
> +      input and the "mux-state" level selecting it.

[Severity: Medium]
Is defining custom port-select-gpios and mux-state properties on the DWC3
controller and splitting the shared High-Speed bus across port@0 and port@2
the right hardware representation for this board topology?

There are three concerns with this binding design in
Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml:

1. The DWC3 controller itself has a single High-Speed D+/D- pair routed
   through an external GPIO-controlled multiplexer to two connectors.
   Instead of introducing custom port-select-gpios and endpoint mux-state
   properties on the DWC3 binding, could this use the existing multiplexer
   bindings in Documentation/devicetree/bindings/mux/gpio-mux.yaml and
   Documentation/devicetree/bindings/mux/mux-consumer.yaml (such as
   mux-controls or mux-states)?
2. Under ports in snps,dwc3-common.yaml, port@0 represents the controller's
   High-Speed data bus and port@1 represents the SuperSpeed data bus. Since
   the controller still has only one physical High-Speed bus, why is the
   second connector modeled as a separate controller port (port@2) rather
   than two endpoints (endpoint@0 and endpoint@1) under port@0 or via an
   intermediate mux node?
3. Documentation/devicetree/bindings/writing-bindings.rst states that
   bindings should describe hardware rather than Linux driver
   implementation details. Here, $defs/hs-endpoint and the allOf conditional
   for port-select-gpios reference Linux kernel functions
   (usb_role_switch_at_endpoint(), dwc3_get_dr_mode(), dwc3_get_extcon(),
   dwc3_drd_init(), and dwc3_role_mux_init()) and force dr_mode and
   controller-level usb-role-switch into required to satisfy current driver
   control flow, even though usb-drd.yaml already defines dr_mode as
   optional with a default of otg.

> @@ -411,7 +524,7 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: '#/$defs/hs-port'
>          description: High Speed (HS) data bus.
>  
>        port@1:

[Severity: Medium]
Will dtschema reject every device tree that sets port-select-gpios and
defines port@2 because port@2 is missing from the top-level
properties/ports block in snps,dwc3-common.yaml?

In snps,dwc3-common.yaml, the top-level properties/ports schema references
/schemas/graph.yaml#/properties/ports and only lists port@0 and port@1:

    ports:
      $ref: /schemas/graph.yaml#/properties/ports
      ...
      properties:
        port@0:
          $ref: '#/$defs/hs-port'
          description: High Speed (HS) data bus.

        port@1:
          $ref: /schemas/graph.yaml#/properties/port
          description: Super Speed (SS) data bus.

(Note also that the comment in allOf and the commit message state that
port@1 is described by snps,dwc3.yaml, whereas port@1 is defined here in
snps,dwc3-common.yaml.)

When dtschema processes a node referencing graph.yaml#/properties/ports
that defines properties, it automatically injects unevaluatedProperties:
false into that properties/ports subschema. In JSON Schema,
unevaluatedProperties: false on the top-level properties/ports block does
not see port@2 evaluated inside the separate allOf then branch, so any
device tree containing port@2 is rejected as having an unevaluated property.

Conversely, in the allOf else branch when port-select-gpios is absent, only
port@0 is restricted and port@2 is not disallowed with port@2: false.

Should port@2 be declared under the top-level properties/ports block and
disallowed with port@2: false in the else branch when port-select-gpios is
absent?

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

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

[Qianhui Xu]

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

Add usb_role_switch_at_endpoint(), which walks the caller's OF graph
endpoints and matches a switch registered on the remote endpoint, and try
it after the existing parent lookup.  A switch that is described by the
graph but not registered yet comes back as -EPROBE_DEFER, as the rest of
the role-switch API does, so the caller retries instead of concluding that
the connector has no role switch at all.  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 | 45 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index edec139b68..ee186d542a 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -157,6 +157,47 @@ 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);
+		/*
+		 * ERR_PTR(-EPROBE_DEFER) means the switch is described by the graph
+		 * but not registered yet: pass it up so the caller defers instead of
+		 * concluding that this connector has no role switch at all.
+		 */
+		if (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 +210,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 +235,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);

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

[sashiko-bot]

> 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.  A switch that is described by the
> graph but not registered yet comes back as -EPROBE_DEFER, as the rest of
> the role-switch API does, so the caller retries instead of concluding that
> the connector has no role switch at all.  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>

Sashiko has reviewed this patch and found no issues.

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

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

[Qianhui Xu]

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

Register one usb_role_switch per connector endpoint described by
"port-select-gpios" and the per-endpoint "mux-state" property.  Requests
from the two switches are serialised by the mux lock, 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 same-role request that moves the data path to
the other connector reinitialises the controller as well and starts it
from the same SUSPHY state as a role change.

A connector reads back the role it requested, not the role currently on the
bus: hiding a pending request behind the active port would leave the
connector driver unable to release it, and the stale request would keep
winning the arbitration after unplug.  The data-path selection and the mode
request it implies are decided under one lock, and the glue layer is
notified with dwc3_pre_set_role() before dwc3_set_mode(), outside
spinlocks, as the single-connector path does.

A failed port-select write is logged and counted per connector; it cannot
leave a torn-down role behind, because __dwc3_set_mode() is the only place
that programs hardware and it never aborts half-way.

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    |  47 ++++++-
 drivers/usb/dwc3/core.h    |  39 ++++++
 drivers/usb/dwc3/debugfs.c |   4 +
 drivers/usb/dwc3/drd.c     | 254 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 342 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index fd5c2cd36c..da609a011d 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,10 @@ 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;
+	bool port_switch;
+	int mux_port = 0;
 	int ret;
 	u32 reg;
 	u32 desired_dr_role;
@@ -174,6 +178,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 +190,14 @@ static void __dwc3_set_mode(struct work_struct *work)
 	if (!desired_dr_role)
 		goto out;
 
-	if (desired_dr_role == dwc->current_dr_role)
+	/*
+	 * Moving the shared data path to the other connector is a mode switch even
+	 * when the role does not change: the running host or gadget instance has to
+	 * be torn down while its own data lines are still selected.
+	 */
+	port_switch = mux && mux_port != mux->active_port;
+
+	if (desired_dr_role == dwc->current_dr_role && !port_switch)
 		goto out;
 
 	if (desired_dr_role == DWC3_GCTL_PRTCAP_OTG && dwc->edev)
@@ -233,8 +246,40 @@ 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 (port_switch) {
+		ret = gpiod_direction_output(mux->select, mux_port);
+		if (ret)
+			dev_err_ratelimited(dwc->dev,
+					    "failed to select USB port %d: %d\n",
+					    mux_port, ret);
+
+		spin_lock_irqsave(&dwc->lock, flags);
+		if (ret)
+			mux->ports[mux_port].errors++;
+		else
+			mux->active_port = mux_port;
+		spin_unlock_irqrestore(&dwc->lock, flags);
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	/*
+	 * dwc3_set_prtcap() only clears GUSB3PIPECTL.SUSPENDENABLE and
+	 * GUSB2PHYCFG.SUSPHY when PRTCAP actually changes, but a same-role port
+	 * switch reinitialises the controller too and must start from the same
+	 * state: the instance created below would otherwise inherit the SUSPHY
+	 * setting of the one torn down above.
+	 */
+	if (port_switch)
+		dwc3_enable_susphy(dwc, false);
+
 	dwc3_set_prtcap(dwc, desired_dr_role, false);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f3dabb37f8..eae321e2d7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1008,6 +1008,43 @@ struct dwc3_glue_ops {
 	void	(*pre_run_stop)(struct dwc3 *dwc, bool is_on);
 };
 
+/* Per-connector state of the optional shared-HS-data-bus mux. */
+struct dwc3_role_port {
+	struct dwc3		*dwc;
+	struct usb_role_switch	*sw;
+	struct fwnode_handle	*fwnode;
+	enum usb_role		role;
+	unsigned int		state;
+	/* request order, see dwc3_role_mux.seq */
+	u64			seq;
+	/* data-path switch failures, for diagnostics only */
+	unsigned int		errors;
+};
+
+/**
+ * struct dwc3_role_mux - optional data-path mux between two connectors
+ * @select: GPIO selecting which connector drives the shared HS data lines
+ * @lock: serialises role requests coming from both connectors
+ * @ports: per-connector state, indexed by the endpoint's "mux-state"
+ * @desired_port: index of the connector that currently wins
+ * @active_port: index of the connector currently driving the data lines
+ * @seq: monotonic request counter used to order pending requests
+ *
+ * Two connectors can share the High Speed data lines of one controller while
+ * keeping their Super Speed lanes separate.  Each connector then needs its own
+ * role-switch input, and the shared data path has to be moved before the
+ * controller is reinitialised for the requested role.
+ */
+struct dwc3_role_mux {
+	struct gpio_desc	*select;
+	/* serialises role requests coming from both connectors */
+	struct mutex		lock;
+	struct dwc3_role_port	ports[2];
+	int			desired_port;
+	int			active_port;
+	u64			seq;
+};
+
 /**
  * struct dwc3 - representation of our controller
  * @drd_work: workqueue used for role swapping
@@ -1064,6 +1101,7 @@ struct dwc3_glue_ops {
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
  * @role_sw: usb_role_switch handle
+ * @role_mux: optional shared data-path mux plus per-connector role switches
  * @role_switch_default_mode: default operation mode of controller while
  *			usb role is USB_ROLE_NONE.
  * @usb_psy: pointer to power supply interface.
@@ -1265,6 +1303,7 @@ struct dwc3 {
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
 	struct usb_role_switch	*role_sw;
+	struct dwc3_role_mux	*role_mux;
 	enum usb_dr_mode	role_switch_default_mode;
 
 	struct power_supply	*usb_psy;
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 65d909e4b7..0d42f10988 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 f3e37d3836..53106bed3e 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,254 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
 	return role;
 }
 
+/*
+ * Index of the connector that should own the shared data path: the oldest
+ * pending request wins, and the last selection is kept when neither connector
+ * requests a role.  Requests are ordered by a monotonic counter rather than by
+ * a timestamp, so a wrapped counter cannot be mistaken for an old request.
+ */
+static int dwc3_role_mux_pick(struct dwc3_role_mux *mux)
+{
+	int sel = mux->desired_port;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mux->ports); i++) {
+		if (mux->ports[i].role == USB_ROLE_NONE)
+			continue;
+		if (mux->ports[sel].role == USB_ROLE_NONE ||
+		    mux->ports[i].seq < mux->ports[sel].seq)
+			sel = i;
+	}
+
+	return sel;
+}
+
+static u32 dwc3_role_mux_mode(struct dwc3 *dwc, enum usb_role role)
+{
+	if (role == USB_ROLE_HOST)
+		return DWC3_GCTL_PRTCAP_HOST;
+	if (role == USB_ROLE_DEVICE)
+		return DWC3_GCTL_PRTCAP_DEVICE;
+	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
+		return DWC3_GCTL_PRTCAP_HOST;
+
+	return DWC3_GCTL_PRTCAP_DEVICE;
+}
+
+static int dwc3_mux_role_set(struct usb_role_switch *sw, enum usb_role role)
+{
+	struct dwc3_role_port *port = usb_role_switch_get_drvdata(sw);
+	struct dwc3 *dwc = port->dwc;
+	struct dwc3_role_mux *mux = dwc->role_mux;
+	unsigned long flags;
+	enum usb_role selected_role;
+	int selected;
+
+	if (role > USB_ROLE_DEVICE)
+		return -EINVAL;
+
+	/*
+	 * The request order and the mode request it implies are decided under one
+	 * lock, so a second connector cannot change desired_port between the
+	 * selection and the dwc3_set_mode() call that consumes it.
+	 */
+	mutex_lock(&mux->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
+	/*
+	 * A role change between two non-NONE roles of the same connector must not
+	 * give that connector a new request ticket: it would otherwise steal the
+	 * data path from the connector that asked first.
+	 */
+	if (port->role == USB_ROLE_NONE && role != USB_ROLE_NONE)
+		port->seq = ++mux->seq;
+	port->role = role;
+	selected = dwc3_role_mux_pick(mux);
+	mux->desired_port = selected;
+	selected_role = mux->ports[selected].role;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/*
+	 * Notify the glue layer before touching the controller, in the same order
+	 * as the single-connector path.  It may sleep (the Qualcomm glue calls
+	 * pm_runtime_resume_and_get()), so it runs outside of dwc->lock -
+	 * dwc3_set_mode() takes that lock itself and a recursive take deadlocks
+	 * the controller on the very first role change.
+	 *
+	 * Pairing with the selection above is provided by mux->lock: a second
+	 * connector blocks in mutex_lock() until this request has been issued.
+	 *
+	 * Same asynchronous semantics as the single-connector role switch: the
+	 * data path and the controller are reprogrammed from __dwc3_set_mode().
+	 */
+	dwc3_pre_set_role(dwc, selected_role);
+	dwc3_set_mode(dwc, dwc3_role_mux_mode(dwc, selected_role));
+
+	mutex_unlock(&mux->lock);
+
+	/*
+	 * A data-path switch failure is reported through the console and counted
+	 * per connector; it is not returned here.  This call has already updated
+	 * the request state, so a stale error from another connector must not be
+	 * reported as the result of this (successful) request.
+	 */
+	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;
+	unsigned long flags;
+	enum usb_role role;
+
+	/*
+	 * Report the role this connector itself requested, whether or not it owns
+	 * the shared data path right now.  Hiding a pending request behind
+	 * active_port makes connector drivers read back USB_ROLE_NONE, so they
+	 * never send the set_role(USB_ROLE_NONE) that would release it; the stale
+	 * request then keeps winning dwc3_role_mux_pick() after the connector has
+	 * been unplugged and starves the other one.
+	 */
+	spin_lock_irqsave(&dwc->lock, flags);
+	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);
+
+	/*
+	 * Publish the mux before the first endpoint reference is taken:
+	 * dwc3_role_mux_exit() is a no-op while dwc->role_mux is NULL, so every
+	 * error path below would otherwise leak the fwnode references already
+	 * acquired.
+	 */
+	dwc->role_mux = mux;
+
+	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;
+		}
+	}
+
+	/*
+	 * Registering the switches makes them reachable from the connector
+	 * drivers, so the initial mode request has to be issued inside the same
+	 * critical section: a set() arriving in between would be silently
+	 * overwritten by it while port->role keeps the requested value.
+	 * dwc3_mux_role_set() serialises on the same lock.
+	 */
+	mutex_lock(&mux->lock);
+
+	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;
+			mutex_unlock(&mux->lock);
+			goto err;
+		}
+	}
+
+	/*
+	 * The controller is only started in its default role once both switches
+	 * are registered: before that a connector driver cannot request a role,
+	 * so nothing can race with this initial mode request.
+	 */
+	dwc3_set_mode(dwc, mode);
+
+	mutex_unlock(&mux->lock);
+
+	return 1;
+
+err:
+	/* A mode request queued before the failure must not run on freed state. */
+	cancel_work_sync(&dwc->drd_work);
+	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 +758,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 +776,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 +791,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 +848,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);
 

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc3: race and reversed teardown order in dwc3_role_mux_init()
- [High] usb: dwc3: torn port and role state in dwc3_mux_role_set()
--

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

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

This commit adds support for DWC3 controllers where two USB connectors share
a single High Speed data bus via a GPIO-controlled mux while using separate
Super Speed PHYs. It registers a usb_role_switch per connector endpoint and
arbitrates role requests and GPIO port selection between the two ports.

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

[ ... ]

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

[ ... ]

> +static int dwc3_mux_role_set(struct usb_role_switch *sw, enum usb_role role)
> +{
> +	struct dwc3_role_port *port = usb_role_switch_get_drvdata(sw);
> +	struct dwc3 *dwc = port->dwc;
> +	struct dwc3_role_mux *mux = dwc->role_mux;
> +	unsigned long flags;
> +	enum usb_role selected_role;
> +	int selected;
> +
> +	if (role > USB_ROLE_DEVICE)
> +		return -EINVAL;
> +
> +	/*
> +	 * The request order and the mode request it implies are decided under one
> +	 * lock, so a second connector cannot change desired_port between the
> +	 * selection and the dwc3_set_mode() call that consumes it.
> +	 */
> +	mutex_lock(&mux->lock);
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	/*
> +	 * A role change between two non-NONE roles of the same connector must not
> +	 * give that connector a new request ticket: it would otherwise steal the
> +	 * data path from the connector that asked first.
> +	 */
> +	if (port->role == USB_ROLE_NONE && role != USB_ROLE_NONE)
> +		port->seq = ++mux->seq;
> +	port->role = role;
> +	selected = dwc3_role_mux_pick(mux);
> +	mux->desired_port = selected;
> +	selected_role = mux->ports[selected].role;
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	/*
> +	 * Notify the glue layer before touching the controller, in the same order
> +	 * as the single-connector path.  It may sleep (the Qualcomm glue calls
> +	 * pm_runtime_resume_and_get()), so it runs outside of dwc->lock -
> +	 * dwc3_set_mode() takes that lock itself and a recursive take deadlocks
> +	 * the controller on the very first role change.
> +	 *
> +	 * Pairing with the selection above is provided by mux->lock: a second
> +	 * connector blocks in mutex_lock() until this request has been issued.
> +	 *
> +	 * Same asynchronous semantics as the single-connector role switch: the
> +	 * data path and the controller are reprogrammed from __dwc3_set_mode().
> +	 */
> +	dwc3_pre_set_role(dwc, selected_role);
> +	dwc3_set_mode(dwc, dwc3_role_mux_mode(dwc, selected_role));

[Severity: High]
Can __dwc3_set_mode() observe a torn update between mux->desired_port and
dwc->desired_dr_role here?

In dwc3_mux_role_set(), mux->desired_port is updated under dwc->lock, and
then dwc->lock is dropped across dwc3_pre_set_role() (which may sleep) before
dwc3_set_mode() re-acquires dwc->lock to update dwc->desired_dr_role.

Meanwhile, __dwc3_set_mode() in drivers/usb/dwc3/core.c does not acquire
mux->lock when snapshotting those two fields:

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

If dwc->drd_work is already pending on system_freezable_wq from an earlier
dwc3_set_mode() call and starts executing __dwc3_set_mode() while
dwc3_mux_role_set() is sleeping in dwc3_pre_set_role(), wouldn't
__dwc3_set_mode() read the new mux->desired_port paired with the previous
dwc->desired_dr_role, switching the port-select GPIO to the new connector
while initializing the controller in the old role before dwc3_pre_set_role()
has completed?

> +
> +	mutex_unlock(&mux->lock);
> +
> +	/*
> +	 * A data-path switch failure is reported through the console and counted
> +	 * per connector; it is not returned here.  This call has already updated
> +	 * the request state, so a stale error from another connector must not be
> +	 * reported as the result of this (successful) request.
> +	 */
> +	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)
> +{

[ ... ]

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

[Severity: High]
What happens if usb_role_switch_register() succeeds for port 0 and fails for
port 1 in dwc3_role_mux_init()?

Once mux->ports[0].sw is registered, port 0's connector driver can immediately
look it up and call usb_role_switch_set_role() -> dwc3_mux_role_set(). While
dwc3_role_mux_init() holds mux->lock, that call blocks on
mutex_lock(&mux->lock).

When registration of port 1 fails, dwc3_role_mux_init() unlocks mux->lock
while mux->ports[0].sw is still registered, and at the err label calls
cancel_work_sync(&dwc->drd_work) before dwc3_role_mux_exit(dwc) unregisters
mux->ports[0].sw.

Doesn't that allow a blocked or concurrent dwc3_mux_role_set() on port 0 to:

1. Queue dwc->drd_work via dwc3_set_mode() after cancel_work_sync() has
already returned, leaving __dwc3_set_mode() queued or running on devm-managed
struct dwc3 and struct dwc3_role_mux after probe fails and frees them?

2. Enter dwc3_mux_role_set() just before usb_role_switch_unregister() in
dwc3_role_mux_exit() (which does not synchronize with mux->lock) and read
dwc->role_mux after err sets dwc->role_mux = NULL, dereferencing a NULL mux
pointer in mutex_lock(&mux->lock)?

3. Run __dwc3_set_mode() before cancel_work_sync() finishes and initialize
host or gadget mode (dwc3_host_init() / dwc3_gadget_init()), which is never
cleaned up because dwc3_core_probe() -> dwc3_core_init_mode() jumps to
err_exit_debugfs on failure without calling dwc3_core_exit_mode() or
dwc3_drd_exit()?

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