Re: [PATCH v9 4/8] usb: misc: qcom_eud: add per-path role switch support

[Elson Serrao]

On 10/15/2026 7:44 AM, Konrad Dybcio wrote:
> On 9/29/26 11:35 PM, Elson Serrao wrote:
>> The EUD hardware can support multiple High-Speed USB paths, each connected
>> to different USB controllers. The current implementation uses a single
>> chip-level role switch, which cannot properly handle multi-path
>> configurations where each path needs independent role management. Since
>> EUD is physically present between the USB connector and the controller,
>> it should also relay the role change requests from the connector.
>>
>> Restructure the driver to support per-path role switches and remove the
>> chip-level role switch. Additionally, as EUD need not modify the USB
>> role upon enabling, remove the unnecessary role switch call from
>> enable_eud().
>>
>> Reviewed-by: Peter Chen <peter.chen@xxxxxxxxxx>
>> Signed-off-by: Elson Serrao <elson.serrao@xxxxxxxxxxxxxxxx>
>> ---
> 
> [...]
> 
>> -static void eud_role_switch_release(void *data)
>> +static int eud_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
>>  {
>> -	struct eud_chip *chip = data;
>> +	struct eud_path *path = usb_role_switch_get_drvdata(sw);
>> +	int ret;
>> +
>> +	/* curr_role tracks the role from EUD's point of view */
>> +	path->curr_role = role;
> 
> This should be assigned after the fallible call below
> 
My thinking here was that `curr_role` should track the role requested by
TCPM, rather than the outcome of the downstream controller role switch.

When TCPM requests `USB_ROLE_DEVICE` (e.g. cable insertion), EUD should be
enabled regardless of whether `usb_role_switch_set_role()` eventually
succeeds or fails. EUD access is tied to the requested device-mode
operation, not to the controller's ability to complete the transition.
Likewise, when the port exits `USB_ROLE_DEVICE`, EUD should be disabled
irrespective of the controller's state, since device-mode EUD access is no
longer needed.

If `curr_role` is updated based on the controller's outcome, EUD state and
`curr_role` can become inconsistent. For example, TCPM may request
`USB_ROLE_DEVICE` and we enable_eud() as per Patch6, while the downstream
controller subsequently fails to switch roles. In that case EUD remains
enabled, but `curr_role` would still be `USB_ROLE_NONE` (see also Sashiko's
observation with this approach 
https://lore.kernel.org/all/20260811215406.7FD161F00A3A@xxxxxxxxxxxxxxx/).

We could keep them synchronized by rolling back the EUD state when the
controller role switch fails. However, I don't see a strong reason to tie
EUD state to the controller outcome. EUD is intended to operate
independently of the controller in device mode. Preserving EUD access in
this scenario also provides a means to investigate why the downstream
controller role switch failed.

Thanks,
Elson

>> +
>> +	/* Forward the role request to the USB controller */
>> +	ret = usb_role_switch_set_role(path->controller_sw, role);
>> +	if (ret) {
>> +		dev_err(path->chip->dev, "failed to set role %s for port %u: %d\n",
>> +			usb_role_string(role), path->num, ret);
>> +		return ret;
>> +	}
>>  
>> -	usb_role_switch_put(chip->role_sw);
>> +	return 0;
>> +}
>> +

[...]