On Mon, Jan 27, 2025 at 03:07:15PM -0800, Elson Roy Serrao wrote:
The role switch registration and set_role() can happen in parallel as they are invoked independent of each other. There is a possibility that a driver might spend significant amount of time in usb_role_switch_register() API due to the presence of time intensive operations like component_add() which operate under common mutex. This leads to a time window after allocating the switch and before setting the registered flag where the set role notifications are dropped. Below timeline summarizes this behavior
Thread1 | Thread2 usb_role_switch_register() | | | ---> allocate switch | | | ---> component_add() | usb_role_switch_set_role() | | | | | --> Drop role notifications | | since sw->registered | | flag is not set. | | --->Set registered flag.|
To avoid this, cache the last role received and set it once the switch registration is complete. Since we are now caching the roles based on registered flag, protect this flag with the switch mutex.
Instead, why not just mark the switch registered from the get-go?
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c index c58a12c147f4..cf38be82d397 100644 --- a/drivers/usb/roles/class.c +++ b/drivers/usb/roles/class.c @@ -387,6 +387,8 @@ usb_role_switch_register(struct device *parent, dev_set_name(&sw->dev, "%s-role-switch", desc->name ? desc->name : dev_name(parent));
+ sw->registered = true; + ret = device_register(&sw->dev); if (ret) { put_device(&sw->dev); @@ -399,8 +401,6 @@ usb_role_switch_register(struct device *parent, dev_warn(&sw->dev, "failed to add component\n"); }
- sw->registered = true; - /* TODO: Symlinks for the host port and the device controller. */
return sw;
thanks,