On 2/3/2025 6:40 AM, Heikki Krogerus wrote:
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;
Thank you for the feedback.
Yes that works as well. Wasn't entirely sure if we can set that flag from the get-go before calling device_register(). But guess we can reset the flag if device_register fails since that is the only failure path in role_switch_register().
I will upload v2 with this modification.
Best Regards, Elson