[PATCH] usb: roles: cache usb roles received during switch registration

[PATCH] usb: roles: cache usb roles received during switch registration

[Elson Roy Serrao]

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.

Fixes: b787a3e78175 ("usb: roles: don't get/set_role() when usb_role_switch is unregistered")
cc: stable@vger.kernel.org
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/roles/class.c | 45 ++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index c58a12c147f4..c0149c31c01b 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -26,6 +26,8 @@ struct usb_role_switch {
 	struct mutex lock; /* device lock*/
 	struct module *module; /* the module this device depends on */
 	enum usb_role role;
+	enum usb_role cached_role;
+	bool cached;
 	bool registered;
 
 	/* From descriptor */
@@ -65,6 +67,20 @@ static const struct component_ops connector_ops = {
 	.unbind = connector_unbind,
 };
 
+static int __usb_role_switch_set_role(struct usb_role_switch *sw,
+				      enum usb_role role)
+{
+	int ret;
+
+	ret = sw->set(sw, role);
+	if (!ret) {
+		sw->role = role;
+		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
+	}
+
+	return ret;
+}
+
 /**
  * usb_role_switch_set_role - Set USB role for a switch
  * @sw: USB role switch
@@ -79,17 +95,21 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
 	if (IS_ERR_OR_NULL(sw))
 		return 0;
 
-	if (!sw->registered)
-		return -EOPNOTSUPP;
-
+	/*
+	 * Since we have a valid sw struct here, role switch registration might
+	 * be in progress. Hence cache the role here and send it out once
+	 * registration is complete.
+	 */
 	mutex_lock(&sw->lock);
-
-	ret = sw->set(sw, role);
-	if (!ret) {
-		sw->role = role;
-		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
+	if (!sw->registered) {
+		sw->cached = true;
+		sw->cached_role = role;
+		mutex_unlock(&sw->lock);
+		return 0;
 	}
 
+	ret = __usb_role_switch_set_role(sw, role);
+
 	mutex_unlock(&sw->lock);
 
 	return ret;
@@ -399,8 +419,14 @@ usb_role_switch_register(struct device *parent,
 			dev_warn(&sw->dev, "failed to add component\n");
 	}
 
+	mutex_lock(&sw->lock);
 	sw->registered = true;
 
+	if (sw->cached)
+		__usb_role_switch_set_role(sw, sw->cached_role);
+
+	mutex_unlock(&sw->lock);
+
 	/* TODO: Symlinks for the host port and the device controller. */
 
 	return sw;
@@ -417,7 +443,10 @@ void usb_role_switch_unregister(struct usb_role_switch *sw)
 {
 	if (IS_ERR_OR_NULL(sw))
 		return;
+	mutex_lock(&sw->lock);
 	sw->registered = false;
+	sw->cached = false;
+	mutex_unlock(&sw->lock);
 	if (dev_fwnode(&sw->dev))
 		component_del(&sw->dev, &connector_ops);
 	device_unregister(&sw->dev);
-- 
2.17.1

Re: [PATCH] usb: roles: cache usb roles received during switch registration

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH] usb: roles: cache usb roles received during switch registration

[Elson Serrao]

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