Re: [PATCH v5 8/9] arm64: dts: qcom: kodiak: Describe EUD UTMI path using child node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch creates a broken (asymmetric) OF graph connection by modifying `kodiak.dtsi` without updating dependent board files, which breaks USB role switching for EUD.
- [Medium] Missing mutex lock in `port_store` allows a race condition with `enable_store` and role switch callbacks, potentially corrupting EUD hardware state.
--

--- Patch [8]: [PATCH v5 8/9] arm64: dts: qcom: kodiak: Describe EUD UTMI path using child node ---
commit 6a20db16eb123d18691f3748fc7fc0dc057b3d4c
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

arm64: dts: qcom: kodiak: Describe EUD UTMI path using child node

This patch updates the EUD description to explicitly model the topology
using a child node and adds a role-switch declaration. It removes the
USB endpoint mapping to the secondary controller, associating it with
the primary controller, and enables the EUD by default.

> diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> index fa540d8c2615d..be9d02760a985 100644
> --- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
[ ... ]
> @@ -4736,16 +4730,28 @@ eud: eud@88e0000 {
[ ... ]
> +			eud0: eud-path@0 {
> +				reg = <0>;
> +				usb-role-switch;
>  
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +						eud_usb: endpoint {
> +							remote-endpoint = <&usb_1_dwc3_hs>;
> +						};
> +					};

[Severity: High]
Does this create an asymmetric OF graph connection?

If downstream board files (like qcs6490-rb3gen2.dts) override usb_1_dwc3_hs
to point directly to their Type-C connectors (e.g., pmic_glink_hs_in),
wouldn't this leave eud_usb as a dangling one-way link?

This would violate the OF graph specification which strictly requires
symmetric bidirectional links.

[ ... ]
> @@ -4972,6 +4978,7 @@ port@0 {
>  					reg = <0>;
>  
>  					usb_1_dwc3_hs: endpoint {
> +						remote-endpoint = <&eud_usb>;
>  					};
>  				};

[Severity: High]
Will Type-C role switch events bypass the EUD driver entirely on boards
that override this endpoint?

If the OF graph connection is broken by downstream overrides, could this
leave the enabled EUD hardware permanently waiting for role change callbacks
that never arrive?

Additionally, looking at drivers/usb/misc/qcom_eud.c:port_store():

	/* Port selection must be done before enabling EUD */
	if (chip->enabled)
		return -EBUSY;

	writel(port, chip->base + EUD_REG_PORT_SEL);
	chip->port_idx = port;

[Severity: Medium]
Does this missing mutex lock in port_store() allow a race condition with
enable_store()?

If a concurrent sysfs write to enable and port occurs, port_store() can
interleave with enable_store() since it doesn't hold chip->state_lock.

Could this allow enable_store() to perform role checks using one port but
configure and enable the PHY for another port, misaligning the internal
driver state with the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803031534.2942628-1-elson.serrao@oss.qualcomm.com?part=8