usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED

From: Michal Nazarewicz <[email protected]>

This commit removes the use of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. Those were selected by UDC drivers which supported respective speeds.

However, since kernel now allows multiple UDC drivers to be compiled, the options in question may no longer reflect the state of all gadgets.

For instance, if one driver that supports dual speed is selected and another that does not, the USB_GADGE_DUALSPEED will be set "for both".

This commit replaces all the #ifdefs by a run-time checks made by calling gadget_is_dualspeed().

drivers/usb/gadget/file_storage.c +0 -0

@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
 /*-------------------------------------------------------------------------*/

drivers/usb/gadget/inode.c +0 -0

index 1b24099..1869478 100644
@@ -1015,9 +1013,8 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
 			if (dev->current_config) {

Quoted 8 lines

drivers/usb/gadget/composite.c +1 -2

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct  
 usb_composite_driver *driver,
 		driver->iProduct = driver->name;
 	composite_driver.function =  (char *) driver->name;
 	composite_driver.driver.name = driver->name;
-	composite_driver.speed = min((u8)composite_driver.speed,
-				     (u8)driver->max_speed);
+	composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);

I wonder if the min is needed here at all. It doesn't seem to be doing any good.

Quoted 5 lines

drivers/usb/gadget/file_storage.c +0 -0

@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
 /*-------------------------------------------------------------------------*/
 

drivers/usb/gadget/file_storage.c static void fsg_resume(struct usb_gadget *gadget) · line 3568

Quoted 9 lines

drivers/usb/gadget/file_storage.c +0 -4

 static struct usb_gadget_driver		fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
 	.speed		= USB_SPEED_HIGH,
-#else
-	.speed		= USB_SPEED_FULL,
-#endif
 	.function	= (char *) fsg_string_product,
 	.unbind		= fsg_unbind,
 	.disconnect	= fsg_disconnect,

You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH. The same goes for the other drivers below.

Quoted 10 lines

drivers/usb/gadget/inode.c +0 -0

index 1b24099..1869478 100644
@@ -1015,9 +1013,8 @@ ep0_read (struct file *fd, char __user *buf,  
 size_t len, loff_t *ptr)
 			if (dev->current_config) {
 				unsigned power;
 

Quoted 8 lines

drivers/usb/gadget/inode.c +2 -3

-				if (gadget_is_dualspeed(dev->gadget)
-						&& (dev->gadget->speed
-							== USB_SPEED_HIGH))
+				if (gadget_is_dualspeed(dev->gadget) &&
+				    (dev->gadget->speed == USB_SPEED_HIGH))
 					power = dev->hs_config->bMaxPower;
 				else

power = dev->config->bMaxPower;

Please don't mix functional changes with cosmetic whitespace changes.

Quoted 9 lines · Michal Nazarewicz

drivers/usb/gadget/composite.c int usb_composite_probe(struct · line 1607

Quoted 8 lines

drivers/usb/gadget/composite.c +1 -2

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct  
 usb_composite_driver *driver,
 		driver->iProduct = driver->name;
 	composite_driver.function =  (char *) driver->name;
 	composite_driver.driver.name = driver->name;
-	composite_driver.speed = min((u8)composite_driver.speed,
-				     (u8)driver->max_speed);
+	composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);

On Wed, 17 Aug 2011 16:20:15 +0200, Alan Stern <[email protected]> wrote:

Quoted 2 lines

I wonder if the min is needed here at all. It doesn't seem to be doing any good.

Do you mean just doing “composite_driver.speed = driver->max_speed”?

Quoted 5 lines

Quoted 5 lines

drivers/usb/gadget/file_storage.c +0 -0

@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
 /*-------------------------------------------------------------------------*/
 

Quoted 12 lines

drivers/usb/gadget/file_storage.c static void fsg_resume(struct usb_gadget *gadget) · line 3568

Quoted 9 lines

drivers/usb/gadget/file_storage.c +0 -4

 static struct usb_gadget_driver		fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
 	.speed		= USB_SPEED_HIGH,
-#else
-	.speed		= USB_SPEED_FULL,
-#endif
 	.function	= (char *) fsg_string_product,
 	.unbind		= fsg_unbind,
 	.disconnect	= fsg_disconnect,

You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH. The same goes for the other drivers below.

Please note that the field is not in fact removed.

Quoted 12 lines

Quoted 9 lines · Michal Nazarewicz

Quoted 8 lines

drivers/usb/gadget/composite.c +1 -2

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
 usb_composite_driver *driver,
 		driver->iProduct = driver->name;
 	composite_driver.function =  (char *) driver->name;
 	composite_driver.driver.name = driver->name;
-	composite_driver.speed = min((u8)composite_driver.speed,
-				     (u8)driver->max_speed);
+	composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);

Quoted 1 line · Alan Stern

I wonder if the min is needed here at all. It doesn't seem to be doing any good.

Quoted 1 line · Michal Nazarewicz

Do you mean just doing “composite_driver.speed = driver->max_speed”?

Yes. After all, there aren't any usb_device_speed values larger than USB_SPEED_SUPER.

Quoted 15 lines

Quoted 15 lines

drivers/usb/gadget/file_storage.c static void fsg_resume(struct usb_gadget · line 3567

Quoted 13 lines

drivers/usb/gadget/file_storage.c +0 -4

@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget  
 *gadget)
 static struct usb_gadget_driver		fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
 	.speed		= USB_SPEED_HIGH,
-#else
-	.speed		= USB_SPEED_FULL,
-#endif
 	.function	= (char *) fsg_string_product,
 	.unbind		= fsg_unbind,
 	.disconnect	= fsg_disconnect,

You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH.

Quoted 1 line

Please note that the field is not in fact removed.

No, but you removed its initializer and as a result, it will be set to 0 whereas it should be set to USB_SPEED_HIGH.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

drivers/usb/gadget/file_storage.c static void fsg_resume(struct usb_gadget · line 3567

Quoted 13 lines

drivers/usb/gadget/file_storage.c +0 -4

@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget  
 *gadget)
 static struct usb_gadget_driver		fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
 	.speed		= USB_SPEED_HIGH,
-#else
-	.speed		= USB_SPEED_FULL,
-#endif
 	.function	= (char *) fsg_string_product,
 	.unbind		= fsg_unbind,
 	.disconnect	= fsg_disconnect,

You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH.

Quoted 4 lines

Quoted 1 line

Please note that the field is not in fact removed.

No, but you removed its initializer and as a result, it will be set to 0 whereas it should be set to USB_SPEED_HIGH.

The initializer is still there. ;) Only the “#” lines and the initializer in the “#else” part are removed.

From: Michal Nazarewicz <[email protected]>

This commit removes the use of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. Those were selected by UDC drivers which supported respective speeds.

However, since kernel now allows multiple UDC drivers to be compiled, the options in question may no longer reflect the state of all gadgets.

For instance, if one driver that supports dual speed is selected and another that does not, the USB_GADGE_DUALSPEED will be set "for both".

This commit replaces all the #ifdefs by a run-time checks made by calling gadget_is_dualspeed().

Signed-off-by Michal Nazarewicz <[email protected]>

drivers/usb/gadget/inode.c +0 -0

index 1b24099..1869478 100644

drivers/usb/gadget/inode.c +0 -0

 					power = dev->hs_config->bMaxPower;
 				else
 					power = dev->config->bMaxPower;

@@ -1354,10 +1350,8 @@ static void make_qualifier (struct dev_data *dev)
 

Quoted 10 lines · Michal Nazarewicz

drivers/usb/gadget/inode.c +1 -2

@@ -1354,10 +1350,8 @@ static void make_qualifier (struct dev_data *dev)
  	memcpy (dev->rbuf, &qual, sizeof qual);
 }
-#endif
 -static int
-config_buf (struct dev_data *dev, u8 type, unsigned index)
+static int config_buf (struct dev_data *dev, u8 type, unsigned index)
 {
 	int		len;
 	int		hs = 0;

WARNING: space prohibited between function name and open parenthesis '(' #291: FILE: drivers/usb/gadget/inode.c:1354:
+static int config_buf (struct dev_data *dev, u8 type, unsigned index)

From: Michal Nazarewicz <[email protected]>

This commit removes the use of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. Those were selected by UDC drivers which supported respective speeds.

However, since kernel now allows multiple UDC drivers to be compiled, the options in question may no longer reflect the state of all gadgets.

For instance, if one driver that supports dual speed is selected and another that does not, the USB_GADGE_DUALSPEED will be set "for both".

This commit replaces all the #ifdefs by a run-time checks made by calling gadget_is_dualspeed().

drivers/usb/gadget/composite.c +0 -6

@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
 /*-------------------------------------------------------------------------*/
 
 static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
-	.speed		= USB_SPEED_SUPER,
-#else
-	.speed		= USB_SPEED_HIGH,
-#endif
-
 	.unbind		= composite_unbind,
 
 	.setup		= composite_setup,

Quoted 25 lines

drivers/usb/gadget/composite.c +1 -8

@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
 /*-------------------------------------------------------------------------*/
 
 static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
-	.speed		= USB_SPEED_SUPER,
-#else
-	.speed		= USB_SPEED_HIGH,
-#endif
-
 	.unbind		= composite_unbind,
 
 	.setup		= composite_setup,

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct  
 usb_composite_driver *driver,
 		driver->iProduct = driver->name;
 	composite_driver.function =  (char *) driver->name;
 	composite_driver.driver.name = driver->name;
-	composite_driver.speed = min((u8)composite_driver.speed,
-				     (u8)driver->max_speed);
+	composite_driver.speed = driver->max_speed;
 	composite = driver;
 	composite_gadget_bind = bind;

I see what's going on here. Your original patch was wrong and then my correction was wrong as well.

This line has to remain the way it was (although those (u8) typecasts don't seem to be necessary). Above, you have to initialize composite_driver.speed to an appropriate value, probably USB_SPEED_SUPER.

What you didn't realize in your original patch is that usb_composite_probe() gets called more than once. Each time it is called, it has to adjust composite_driver.speed.

Quoted 8 lines · Alan Stern

I see what's going on here. Your original patch was wrong and then my correction was wrong as well.

This line has to remain the way it was (although those (u8) typecasts don't seem to be necessary). Above, you have to initialize composite_driver.speed to an appropriate value, probably USB_SPEED_SUPER.

Quoted 3 lines

What you didn't realize in your original patch is that usb_composite_probe() gets called more than once. Each time it is called, it has to adjust composite_driver.speed.

That's sneaky of composite.c... But is it desired behaviour?

I cannot
came up with a situation where that's what we want. I would imagine that usb_composite_probe() should assume the same speed for given usb_composite_driver regardless if some other slower usb_composite_driver was loaded.

Quoted 10 lines · Michal Nazarewicz

Quoted 8 lines · Alan Stern

I see what's going on here. Your original patch was wrong and then my correction was wrong as well.

This line has to remain the way it was (although those (u8) typecasts don't seem to be necessary). Above, you have to initialize composite_driver.speed to an appropriate value, probably USB_SPEED_SUPER.

Quoted 5 lines

Quoted 3 lines

What you didn't realize in your original patch is that usb_composite_probe() gets called more than once. Each time it is called, it has to adjust composite_driver.speed.

That's sneaky of composite.c... But is it desired behaviour?

Yes, it is.

Quoted 5 lines

I cannot
came up with a situation where that's what we want. I would imagine that usb_composite_probe() should assume the same speed for given usb_composite_driver regardless if some other slower usb_composite_driver was loaded.

The speed being calculated isn't the speed of the usb_composite_driver; it's the speed of the usb_gadget_driver. The gadget itself cannot be allowed to run faster than its internal drivers can handle.

For example, if you have a composite gadget where one of the function drivers can handle SuperSpeed and the other can't go beyond high speed, the overall gadget must never run faster than high speed.

For the most part, usb_composite_probe() is called only once in module's init function. As far as I know, only g_ffs calls it several times. So in all cases expect for g_ffs, composite_driver.speed = min(composite_driver.speed,
driver->max_speed) should have the same effect as composite_driver.speed = driver->max_speed.

Quoted 3 lines

For example, if you have a composite gadget where one of the function drivers can handle SuperSpeed and the other can't go beyond high speed, the overall gadget must never run faster than high speed.

Shouldn't that be dealt in usb_add_function()? I cannot see any code that would do that here atm though.

Quoted 9 lines · Michal Nazarewicz

For the most part, usb_composite_probe() is called only once in module's init function. As far as I know, only g_ffs calls it several times. So in all cases expect for g_ffs, composite_driver.speed = min(composite_driver.speed,
driver->max_speed) should have the same effect as composite_driver.speed = driver->max_speed.

Quoted 2 lines

For example, if you have a composite gadget where one of the function drivers can handle SuperSpeed and the other can't go beyond high

Quoted 4 lines

speed,

Quoted 1 line

the overall gadget must never run faster than high speed.

Shouldn't that be dealt in usb_add_function()? I cannot see any code that would do that here atm though.

Maybe you're right. But isn't that too late? The UDC driver has to know the gadget driver's limitations before it can connect to the host.

usb_add_function() is called before usb_composite_probe() which only then calls usb_gadget_probe_driver().

What's more, if I understand it correctly, comment in usb_add_function() says that we support all the speeds usb_composite_driver structure claims we do but then return different sets of functions depending on speed. Ie. if a function is only full speed and host requests high speed, configuration
will lack that function. The comment in question is:

/* We allow configurations that don't work at both speeds.
* If we run into a lowspeed Linux system, treat it the same
* as full speed ... it's the function drivers that will need
* to avoid bulk and ISO transfers. */

usb_add_function() then proceeds to set config->fullspeed, config->highspeed
and config->superspeed depending on what descriptors function provides.

Those are later used in config_desc() where it iterates over all configurations checking which have functions supporting given speed:

	list_for_each_entry(c, &cdev->configs, list) {
		switch (speed) {
		case USB_SPEED_SUPER: if (!c->superspeed) continue; break;
		case USB_SPEED_HIGH: if (!c->highspeed) continue; break;
		default: if (!c->fullspeed) continue;
		}
		if (w_value == 0)
			return config_buf(c, speed, cdev->req->buf, type);
		w_value--;
	}

config_buf() at the same time, iterates over all functions and skips the ones
that do not provide descriptors for given speed:

	list_for_each_entry(f, &config->functions, list) {
		switch (speed) {
		case USB_SPEED_SUPER: descriptors = f->ss_descriptors; break;
		case USB_SPEED_HIGH: descriptors = f->hs_descriptors; break;
		default: descriptors = f->descriptors;
		}
		if (!descriptors)
			continue;
[...]
	}

So I think that your suggestion to use composite_driver.speed = driver->max_speed was by all means correct.

Quoted 16 lines

usb_add_function() is called before usb_composite_probe() which only then calls usb_gadget_probe_driver().

What's more, if I understand it correctly, comment in usb_add_function()
says that we support all the speeds usb_composite_driver structure claims we do but then return different sets of functions depending
on speed. Ie. if a function is only full speed and host requests
high speed, configuration will lack that function. The comment in
question is:

/* We allow configurations that don't work at both speeds.
* If we run into a lowspeed Linux system, treat it the same
* as full speed ... it's the function drivers that will need
* to avoid bulk and ISO transfers. */

usb_add_function() then proceeds to set config->fullspeed, config->highspeed and config->superspeed depending on what descriptors function provides.

Have you misinterpreted that comment?

Quoted 2 lines

So I think that your suggestion to use composite_driver.speed = driver->max_speed was by all means correct.

Okay, that line was probably all right, but it seems that something else needs to be changed. In a composite gadget, if none of the function drivers support SuperSpeed then we don't want composite_driver.speed to be set to USB_SPEED_SUPER. It would be foolish to allow the UDC to connect at a speed which would make the gadget useless.

Okay, that line was probably all right, but it seems that something else needs to be changed. In a composite gadget, if none of the function drivers support SuperSpeed then we don't want composite_driver.speed to be set to USB_SPEED_SUPER. It would be foolish to allow the UDC to connect at a speed which would make the gadget useless.

Quoted 6 lines · Alan Stern

Okay, that line was probably all right, but it seems that something else needs to be changed. In a composite gadget, if none of the function drivers support SuperSpeed then we don't want composite_driver.speed to be set to USB_SPEED_SUPER. It would be foolish to allow the UDC to connect at a speed which would make the gadget useless.

OK, so it seems I was misunderstanding your comment all along, but that was nothing a good night sleep couldn't fix. :P

So, what you are saying is that when passed to the UDC driver (ie. by calling usb_gadget_probe_driver()) the usb_gadget_driver structure must have speed no higher then what the UDC supports, right? And in that case, it's not composite specific but affects all the gadgets.

In that case, I think that usb_gadget_probe_driver() itself needs to be changed since prior to this function we don't know which gadget the gadget driver will be bound to. Ie. something as follows could be added somewhere there:

driver->speed = min(driver->speed, udc->gadget->speed)

Does that sound like it makes sense?

At this point though, gadget drivers need to be aware that usb_gadget_probe_driver() may change the speed field. It should not be a problem since, as far as I can tell, all gadgets expect for g_ffs call usb_gadget_probe_driver() only once and in case of g_ffs this can be solved at composite level.

There are three speeds:
- the max speed the UDC supports i.e. HS (no field for that?)

- the max speed the gadget / function driver support i.e. SS (driver->max_speed ?)

- the speed the UDC is connected at the moment i.e. FS. (gadget->speed)

Quoted 2 lines · Sebastian Andrzej Siewior

There are three speeds:
- the max speed the UDC supports i.e. HS (no field for that?)

Yep, the field seems to be missing for that one. We have is_dualspeed and comment in gadget_is_superspeed() says about is_superspeed but that does not exist.

Quoted 2 lines

- the max speed the gadget / function driver support i.e. SS (driver->max_speed ?)

Actually “max_speed” is only in usb_composite_driver structure. usb_gadget_driver has only “speed” and it's description says that it is the “highest speed the driver handles.” Because of that description I've assumed that it is UDC driver's responsibility to figure out maximum speed it can use.

Quoted 1 line

- the speed the UDC is connected at the moment i.e. FS. (gadget->speed)

OK, I see my mistake. I misinterpreted gadget->speed as the max speed gadget supports.

So in the end, it would seem to me that we need to do the following is usb_gadget_probe_direvr() before passing the usb_gadget_driver to the usb_gadegt's bind:

if (driver->speed == USB_SPEED_SUPER && !gadget_is_superspeed(udc->gadget))

	driver->speed = USB_SPEED_HIGH;
if (driver->speed == USB_SPEED_HIGH && !gadget_is_dualspeed(udc->gadget))
	driver->speed = USB_SPEED_FULL;

(Although I'm not sure about USB_SPEED_FULL vs. USB_SPEED_LOW.)

Either than, or make sure that all UDC drivers handle correctly gadgets that declare speed as highest of what the UDC can handle.

At the moment however, gadget_is_superspeed() is implemented using #ifdefs (which I missed before) so we need to add a is_superspeed to usb_gadget structure. Or maybe better yet get rid of is_dualspeed and add max_speed?

Quoted 11 lines

OK, I see my mistake. I misinterpreted gadget->speed as the max speed gadget supports.

So in the end, it would seem to me that we need to do the following is usb_gadget_probe_direvr() before passing the usb_gadget_driver to the usb_gadegt's bind:

if (driver->speed == USB_SPEED_SUPER && !gadget_is_superspeed(udc->gadget))

	driver->speed = USB_SPEED_HIGH;
if (driver->speed == USB_SPEED_HIGH && !gadget_is_dualspeed(udc->gadget))
	driver->speed = USB_SPEED_FULL;

No, the usb_gadget_driver structure is read-only to the UDC driver (except for the .driver field). .speed is the maximum speed supported by the gadget driver -- it doesn't change at runtime. If the driver was written to support speeds up to high speed, then those are the speeds it supports, regardless of the UDC's capabilities.

Maybe if would help if the .speed field in struct usb_gadget_driver was renamed to max_speed.

Quoted 4 lines

(Although I'm not sure about USB_SPEED_FULL vs. USB_SPEED_LOW.)

Either than, or make sure that all UDC drivers handle correctly gadgets that declare speed as highest of what the UDC can handle.

I don't quite understand that sentence. However, it is definitely true that all UDC drivers _must_ avoid connecting at speeds that are faster than the usb_gadget_driver's .speed field.

Quoted 3 lines

At the moment however, gadget_is_superspeed() is implemented using #ifdefs (which I missed before) so we need to add a is_superspeed to usb_gadget structure. Or maybe better yet get rid of
is_dualspeed and add max_speed?

Either one would be okay. max_speed is simpler, although it would require some work to change the existing usages of is_dualspeed.

Quoted 3 lines

I don't quite understand that sentence. However, it is definitely true that all UDC drivers _must_ avoid connecting at speeds that are faster than the usb_gadget_driver's .speed field.

Yang has brought up an issue that musb_gadget_start() checks if driver->speed
equals USB_SPEED_HIGH. This means that if a gadget driver supports super speed
it'll set driver->speed to USB_SPEED_SUPER and musb_gadget_start() will complain.

My understanding is that this is a bug in musb driver as it should allow the
gadget driver to start but never set super speed.

Quoted 29 lines · Alan Stern

Quoted 11 lines · Michal Nazarewicz

Quoted 8 lines · Alan Stern

I see what's going on here. Your original patch was wrong and then my correction was wrong as well.

This line has to remain the way it was (although those (u8) typecasts don't seem to be necessary). Above, you have to initialize composite_driver.speed to an appropriate value, probably USB_SPEED_SUPER.

Quoted 5 lines

Quoted 3 lines

What you didn't realize in your original patch is that usb_composite_probe() gets called more than once. Each time it is called, it has to adjust composite_driver.speed.

That's sneaky of composite.c... But is it desired behaviour?

Yes, it is.

Quoted 7 lines

I cannot
came up with a situation where that's what we want. I would imagine that
usb_composite_probe() should assume the same speed for given usb_composite_driver regardless if some other slower usb_composite_driver
was loaded.

Quoted 3 lines

The speed being calculated isn't the speed of the usb_composite_driver; it's the speed of the usb_gadget_driver. The gadget itself cannot be allowed to run faster than its internal drivers can handle.

Yeah, that's why you set usb_gadget_driver's speed to the speed declared in usb_composite_driver.

For the most part, usb_composite_probe() is called only once in module's init function. As far as I know, only g_ffs calls it several times. So in all cases expect for g_ffs, composite_driver.speed = min(composite_driver.speed,
driver->max_speed) should have the same effect as composite_driver.speed = driver->max_speed.

Quoted 4 lines

For example, if you have a composite gadget where one of the function drivers can handle SuperSpeed and the other can't go beyond high speed, the overall gadget must never run faster than high speed.

Shouldn't that be dealt in usb_add_function()? I cannot see any code that would do that here atm though.

Quoted 6 lines

For the most part, usb_composite_probe() is called only once in module's init function. As far as I know, only g_ffs calls it several times. So in all cases expect for g_ffs, composite_driver.speed = min(composite_driver.speed,
driver->max_speed) should have the same effect as composite_driver.speed = driver->max_speed.

For original code, ifndef CONFIG_USB_GADGET_SUPERSPEED and max_speed = USB_SPEED_SUPER then the result will be:

composite_driver.speed = min(USB_SPEED_HIGH, USB_SPEED_SUPER)

For patched code, composite_driver.speed = USB_SPEED_SUPER

From: Michal Nazarewicz<[email protected]>

This commit removes the use of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. Those were selected by UDC drivers which supported respective speeds.

However, since kernel now allows multiple UDC drivers to be compiled, the options in question may no longer reflect the state of all gadgets.

For instance, if one driver that supports dual speed is selected and another that does not, the USB_GADGE_DUALSPEED will be set "for both".

This commit replaces all the #ifdefs by a run-time checks made by calling gadget_is_dualspeed().

---
  drivers/usb/gadget/Kconfig        |   30 -------------------------
  drivers/usb/gadget/composite.c    |    9 +-------
  drivers/usb/gadget/file_storage.c |    4 ---
  drivers/usb/gadget/inode.c        |   10 --------
  drivers/usb/gadget/printer.c      |   43 ++++++-------------------------------
  drivers/usb/gadget/u_ether.c      |    7 ------
  6 files changed, 8 insertions(+), 95 deletions(-)

Like before, this has been only compile-tested (make allmodconfig bzImage modules on x86_64).

Compared to v1:
* Removed unrelated whitespace changes
* Removed the use of min() in usb_composite_probe()

drivers/usb/gadget/Kconfig +0 -30

index 5a084b9..c60c167 100644
@@ -133,7 +133,6 @@ config USB_AT91
 
  config USB_ATMEL_USBA
         tristate "Atmel USBA"
-       select USB_GADGET_DUALSPEED
         depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
         help
           USBA is the integrated high-speed USB Device controller on

@@ -142,7 +141,6 @@ config USB_ATMEL_USBA
  config USB_FSL_USB2
         tristate "Freescale Highspeed USB DR Peripheral Controller"
         depends on FSL_SOC || ARCH_MXC
-       select USB_GADGET_DUALSPEED
         select USB_FSL_MPH_DR_OF if OF
         help
            Some of Freescale PowerPC processors have a High Speed

@@ -158,7 +156,6 @@ config USB_FSL_USB2
  config USB_FUSB300
         tristate "Faraday FUSB300 USB Peripheral Controller"
         depends on !PHYS_ADDR_T_64BIT
-       select USB_GADGET_DUALSPEED
         help
            Faraday usb device controller FUSB300 driver
 

@@ -206,7 +203,6 @@ config USB_PXA25X_SMALL
 
  config USB_R8A66597
         tristate "Renesas R8A66597 USB Peripheral Controller"
-       select USB_GADGET_DUALSPEED
         help
            R8A66597 is a discrete USB host and peripheral controller chip that
            supports both full and high speed USB 2.0 data transfers.

@@ -220,7 +216,6 @@ config USB_RENESAS_USBHS_UDC
         tristate 'Renesas USBHS controller'
         depends on SUPERH || ARCH_SHMOBILE
         depends on USB_RENESAS_USBHS
-       select USB_GADGET_DUALSPEED
         help
            Renesas USBHS is a discrete USB host and peripheral controller chip
            that supports both full and high speed USB 2.0 data transfers.

@@ -249,7 +244,6 @@ config USB_S3C_HSOTG
         tristate "S3C HS/OtG USB Device controller"
         depends on S3C_DEV_USB_HSOTG
         select USB_GADGET_S3C_HSOTG_PIO
-       select USB_GADGET_DUALSPEED
         help
           The Samsung S3C64XX USB2.0 high-speed gadget controller
           integrated into the S3C64XX series SoC.

@@ -287,7 +281,6 @@ config USB_S3C2410_DEBUG
  config USB_S3C_HSUDC
         tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
         depends on ARCH_S3C2410
-       select USB_GADGET_DUALSPEED
         help
           Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
           integrated with dual speed USB 2.0 device controller. It has

@@ -298,7 +291,6 @@ config USB_S3C_HSUDC
  config USB_PXA_U2O
         tristate "PXA9xx Processor USB2.0 controller"
         depends on ARCH_MMP
-       select USB_GADGET_DUALSPEED
         help
           PXA9xx Processor series include a high speed USB2.0 device
           controller, which support high speed and full speed USB peripheral.

@@ -311,14 +303,12 @@ config USB_PXA_U2O
  config USB_GADGET_MUSB_HDRC
         tristate "Inventra HDRC USB Peripheral (TI, ADI, ...)"
         depends on USB_MUSB_HDRC
-       select USB_GADGET_DUALSPEED
         help
           This OTG-capable silicon IP is used in dual designs including
           the TI DaVinci, OMAP 243x, OMAP 343x, TUSB 6010, and ADI Blackfin
 
  config USB_M66592
         tristate "Renesas M66592 USB Peripheral Controller"
-       select USB_GADGET_DUALSPEED
         help
            M66592 is a discrete USB peripheral controller chip that
            supports both full and high speed USB 2.0 data transfers.

@@ -335,7 +325,6 @@ config USB_M66592
  config USB_AMD5536UDC
         tristate "AMD5536 UDC"
         depends on PCI
-       select USB_GADGET_DUALSPEED
         help
            The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
            It is a USB Highspeed DMA capable USB device controller. Beside ep0

@@ -363,7 +352,6 @@ config USB_FSL_QE
  config USB_CI13XXX_PCI
         tristate "MIPS USB CI13xxx PCI UDC"
         depends on PCI
-       select USB_GADGET_DUALSPEED
         help
           MIPS USB IP core family device controller
           Currently it only supports IP part number CI13412

@@ -374,7 +362,6 @@ config USB_CI13XXX_PCI
 
  config USB_NET2272
         tristate "PLX NET2272"
-       select USB_GADGET_DUALSPEED
         help
           PLX NET2272 is a USB peripheral controller which supports
           both full and high speed USB 2.0 data transfers.

@@ -398,7 +385,6 @@ config USB_NET2272_DMA
  config USB_NET2280
         tristate "NetChip 228x"
         depends on PCI
-       select USB_GADGET_DUALSPEED
         help
            NetChip 2280 / 2282 is a PCI based USB peripheral controller which
            supports both full and high speed USB 2.0 data transfers.

@@ -429,7 +415,6 @@ config USB_LANGWELL
         tristate "Intel Langwell USB Device Controller"
         depends on PCI
         depends on !PHYS_ADDR_T_64BIT
-       select USB_GADGET_DUALSPEED
         help
            Intel Langwell USB Device Controller is a High-Speed USB
            On-The-Go device controller.

@@ -444,7 +429,6 @@ config USB_LANGWELL
  config USB_EG20T
         tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH UDC"
         depends on PCI
-       select USB_GADGET_DUALSPEED
         help
           This is a USB device driver for EG20T PCH.
           EG20T PCH is the platform controller hub that is used in Intel's

@@ -466,7 +450,6 @@ config USB_EG20T
  config USB_CI13XXX_MSM
         tristate "MIPS USB CI13xxx for MSM"
         depends on ARCH_MSM
-       select USB_GADGET_DUALSPEED
         select USB_MSM_OTG
         help
           MSM SoC has chipidea USB controller.  This driver uses

@@ -487,8 +470,6 @@ config USB_CI13XXX_MSM
  config USB_DUMMY_HCD
         tristate "Dummy HCD (DEVELOPMENT)"
         depends on USB=y || (USB=m&&  USB_GADGET=m)
-       select USB_GADGET_DUALSPEED
-       select USB_GADGET_SUPERSPEED
         help
           This host controller driver emulates USB, looping all data transfer
           requests back to a USB "gadget driver" in the same host.  The host

@@ -513,17 +494,6 @@ config USB_DUMMY_HCD
 
  endchoice
 
-# Selected by UDC drivers that support high-speed operation.
-config USB_GADGET_DUALSPEED
-       bool
-       depends on USB_GADGET
-
-# Selected by UDC drivers that support super-speed opperation
-config USB_GADGET_SUPERSPEED
-       bool
-       depends on USB_GADGET
-       depends on USB_GADGET_DUALSPEED
-
  #
  # USB Gadget Drivers
  #

drivers/usb/gadget/composite.c +0 -6

index 5a3461e..c5249b3 100644
@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
  /*-------------------------------------------------------------------------*/
 
  static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
-       .speed          = USB_SPEED_SUPER,
-#else
-       .speed          = USB_SPEED_HIGH,
-#endif
-
         .unbind         = composite_unbind,
 
         .setup          = composite_setup,

         composite = driver;
         composite_gadget_bind = bind;

drivers/usb/gadget/file_storage.c +0 -4

index cf875be..61ed4ec 100644
@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
  /*-------------------------------------------------------------------------*/
 
  static struct usb_gadget_driver                fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
         .speed          = USB_SPEED_HIGH,
-#else
-       .speed          = USB_SPEED_FULL,
-#endif
         .function       = (char *) fsg_string_product,
         .unbind         = fsg_unbind,
         .disconnect     = fsg_disconnect,

drivers/usb/gadget/inode.c +0 -10

index 1b24099..dddb3e4 100644
@@ -837,7 +837,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
                 if (value == 0)
                         data->state = STATE_EP_ENABLED;
                 break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
         case USB_SPEED_HIGH:
                 /* fails if caller didn't provide that descriptor... */
                 ep->desc =&data->hs_desc;

@@ -845,7 +844,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
                 if (value == 0)
                         data->state = STATE_EP_ENABLED;
                 break;
-#endif
         default:
                 DBG(data->dev, "unconnected, %s init abandoned\n",
                                 data->name);

@@ -1331,7 +1329,6 @@ static const struct file_operations ep0_io_operations = {
   * Unrecognized ep0 requests may be handled in user space.
   */
 
-#ifdef CONFIG_USB_GADGET_DUALSPEED
  static void make_qualifier (struct dev_data *dev)
  {
         struct usb_qualifier_descriptor         qual;

@@ -1354,7 +1351,6 @@ static void make_qualifier (struct dev_data *dev)
 
         memcpy (dev->rbuf,&qual, sizeof qual);
  }
-#endif
 
  static int
  config_buf (struct dev_data *dev, u8 type, unsigned index)

@@ -1434,7 +1430,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
                         dev->dev->bMaxPacketSize0 = dev->gadget->ep0->maxpacket;
                         req->buf = dev->dev;
                         break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
                 case USB_DT_DEVICE_QUALIFIER:
                         if (!dev->hs_config)
                                 break;

@@ -1444,7 +1439,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
                         break;
                 case USB_DT_OTHER_SPEED_CONFIG:
                         // FALLTHROUGH
-#endif
                 case USB_DT_CONFIG:
                         value = config_buf (dev,
                                         w_value>>  8,

@@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
  }
 
  static struct usb_gadget_driver gadgetfs_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
         .speed          = USB_SPEED_HIGH,
-#else
-       .speed          = USB_SPEED_FULL,
-#endif
         .function       = (char *) driver_desc,
         .unbind         = gadgetfs_unbind,
         .setup          = gadgetfs_setup,

drivers/usb/gadget/printer.c +7 -36

index a341dde..901b7ba 100644
@@ -164,12 +164,6 @@ module_param(qlen, uint, S_IRUGO|S_IWUSR);
 
  #define QLEN   qlen
 
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-#define DEVSPEED       USB_SPEED_HIGH
-#else   /* full speed (low speed doesn't do bulk) */
-#define DEVSPEED        USB_SPEED_FULL
-#endif
-
  /*-------------------------------------------------------------------------*/
 
  #define xprintk(d, level, fmt, args...) \

@@ -288,8 +282,6 @@ static const struct usb_descriptor_header *fs_printer_function [11] = {
         NULL
  };
 
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
  /*
   * usb 2.0 devices need to expose both high speed and full speed
   * descriptors, unless they only run at full speed.

@@ -328,13 +320,6 @@ static const struct usb_descriptor_header *hs_printer_function [11] = {
  /* maxpacket and other transfer characteristics vary by speed. */
  #define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
 
-#else
-
-/* if there's no high speed support, maxpacket doesn't change. */
-#define ep_desc(g, hs, fs) (((void)(g)), (fs))
-
-#endif /* !CONFIG_USB_GADGET_DUALSPEED */
-
  /*-------------------------------------------------------------------------*/
 
  /* descriptors that are built on-demand */

@@ -979,9 +964,7 @@ printer_set_config(struct printer_dev *dev, unsigned number)
 
                 switch (gadget->speed) {
                 case USB_SPEED_FULL:    speed = "full"; break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
                 case USB_SPEED_HIGH:    speed = "high"; break;
-#endif
                 default:                speed = "?"; break;
                 }
 

@@ -998,23 +981,15 @@ config_buf(enum usb_device_speed speed, u8 *buf, u8 type, unsigned index,
  {
         int                                     len;
         const struct usb_descriptor_header      **function;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
         int                                     hs = (speed == USB_SPEED_HIGH);
 
+       if (index>= device_desc.bNumConfigurations)
+               return -EINVAL;
+
         if (type == USB_DT_OTHER_SPEED_CONFIG)
                 hs = !hs;
 
-       if (hs) {
-               function = hs_printer_function;
-       } else {
-               function = fs_printer_function;
-       }
-#else
-       function = fs_printer_function;
-#endif
-
-       if (index>= device_desc.bNumConfigurations)
-               return -EINVAL;
+       function = hs ? hs_printer_function : fs_printer_function;
 
         /* for now, don't advertise srp-only devices */
         if (!is_otg)

@@ -1156,9 +1131,8 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
                                 value = min(wLength, (u16) sizeof device_desc);
                                 memcpy(req->buf,&device_desc, value);
                                 break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
                         case USB_DT_DEVICE_QUALIFIER:
-                               if (!gadget->is_dualspeed)
+                               if (!gadget_is_dualspeed(gadget))
                                         break;
                                 /*
                                  * assumes ep0 uses the same value for both

@@ -1172,10 +1146,9 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
                                 break;
 
                         case USB_DT_OTHER_SPEED_CONFIG:
-                               if (!gadget->is_dualspeed)
+                               if (!gadget_is_dualspeed(gadget))
                                         break;
                                 /* FALLTHROUGH */
-#endif /* CONFIG_USB_GADGET_DUALSPEED */
                         case USB_DT_CONFIG:
                                 value = config_buf(gadget->speed, req->buf,
                                                 wValue>>  8,

@@ -1460,11 +1433,9 @@ autoconf_fail:
                 goto autoconf_fail;
         out_ep->driver_data = out_ep;   /* claim */
 
-#ifdef CONFIG_USB_GADGET_DUALSPEED
         /* assumes that all endpoints are dual-speed */
         hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
         hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
-#endif /* DUALSPEED */
 
         usb_gadget_set_selfpowered(gadget);
 

@@ -1552,7 +1523,7 @@ fail:
  /*-------------------------------------------------------------------------*/
 
  static struct usb_gadget_driver printer_driver = {
-       .speed          = DEVSPEED,
+       .speed          = USB_SPEED_HIGH,
 
         .function       = (char *) driver_desc,
         .unbind         = printer_unbind,

drivers/usb/gadget/u_ether.c +0 -8

index dfed4c1..8642abb 100644
@@ -92,17 +92,10 @@ struct eth_dev {
 
  #define DEFAULT_QLEN   2       /* double buffering by default */
 
-
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
  static unsigned qmult = 5;
  module_param(qmult, uint, S_IRUGO|S_IWUSR);
  MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
 
-#else  /* full speed (low speed doesn't do bulk) */
-#define qmult          1
-#endif
-
  /* for dual-speed hardware, use deeper queues at high/super speed */
  static inline int qlen(struct usb_gadget *gadget)
  {
--

1.7.3.1

I did not see the include/linux/usb/gadget.h changes, did you have another patch? There's CONFIG_USB_GADGET_DUALSPEED ifdefs in gadget_is_dualspeed function of the gadget.h If so, I will get oops as before.

Quoted 14 lines

drivers/usb/gadget/composite.c +0 -6

@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
  /*-------------------------------------------------------------------------*/
 
  static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
-       .speed          = USB_SPEED_SUPER,
-#else
-       .speed          = USB_SPEED_HIGH,
-#endif
-
         .unbind         = composite_unbind,
 

drivers/usb/gadget/composite.c int usb_composite_probe(struct · line 1607

Quoted 9 lines

drivers/usb/gadget/composite.c +1 -2

         .setup          = composite_setup,

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct  
 usb_composite_driver *driver,
                 driver->iProduct = driver->name;
         composite_driver.function =  (char *) driver->name;
         composite_driver.driver.name = driver->name;
-       composite_driver.speed = min((u8)composite_driver.speed,
-                                    (u8)driver->max_speed);
+       composite_driver.speed = driver->max_speed;

This does not works, many drivers will check the driver->speed, ie in musg_gadget.c

static int musb_gadget_start(struct usb_gadget *g,
struct usb_gadget_driver *driver)

{
         struct musb             *musb = gadget_to_musb(g);
         unsigned long           flags;
         int                     retval = -EINVAL;

         if (driver->speed != USB_SPEED_HIGH)
                 goto err0;

[snip]

Quoted 2 lines

         composite = driver;
         composite_gadget_bind = bind;

Quoted 6 lines · Yang Rui Rui

I did not see the include/linux/usb/gadget.h changes, did you have another patch?
There's  CONFIG_USB_GADGET_DUALSPEED ifdefs in gadget_is_dualspeed function of the gadget.h
If so, I will get oops as before.

Thanks, fixed.  I did “git add -u” while I was in drivers/usb/gadget so gadget.h did not get added and I've overlooked it.  Sorry about that.

Quoted 17 lines · Michal Nazarewicz

Quoted 16 lines

drivers/usb/gadget/composite.c +0 -6

@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
 
  /*-------------------------------------------------------------------------*/
 
  static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
-       .speed          = USB_SPEED_SUPER,
-#else
-       .speed          = USB_SPEED_HIGH,
-#endif
-
         .unbind         = composite_unbind,
 

Quoted 12 lines

drivers/usb/gadget/composite.c int usb_composite_probe(struct usb_composite_driver · line 1607

Quoted 9 lines

        .setup          = composite_setup,

drivers/usb/gadget/composite.c +1 -2

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver
 *driver,
                 driver->iProduct = driver->name;
         composite_driver.function =  (char *) driver->name;
         composite_driver.driver.name = driver->name;
-       composite_driver.speed = min((u8)composite_driver.speed,
-                                    (u8)driver->max_speed);
+       composite_driver.speed = driver->max_speed;

This does not works, many drivers will check the driver->speed, ie in musg_gadget.c

Uh?  Not sure what you mean here.  Note that driver above is not the same structure as driver below.  composite_driver is what musb_gadget_start() will get.

Quoted 8 lines

static int musb_gadget_start(struct usb_gadget *g, struct usb_gadget_driver *driver) {
        struct musb             *musb = gadget_to_musb(g); unsigned long           flags;
        int                     retval = -EINVAL;

Quoted 6 lines

        if (driver->speed != USB_SPEED_HIGH)
                goto err0;
[snip]

Quoted 2 lines

drivers/usb/gadget/composite.c +0 -0

         composite = driver;
         composite_gadget_bind = bind;

--
Best regards,                                         _     _ .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o ..o | Computer Science,  Michal "mina86" Nazarewicz    (o o) ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo-- --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 30 lines

Quoted 26 lines · Michal Nazarewicz

Quoted 25 lines

drivers/usb/gadget/composite.c +0 -6

@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
 
 /*-------------------------------------------------------------------------*/
 
 static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
-       .speed          = USB_SPEED_SUPER,
-#else
-       .speed          = USB_SPEED_HIGH,
-#endif
-
        .unbind         = composite_unbind,
 

.setup = composite_setup,

drivers/usb/gadget/composite.c +0 -0

@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct  

usb_composite_driver
*driver,

                driver->iProduct = driver->name;
        composite_driver.function =  (char *) driver->name;
        composite_driver.driver.name = driver->name;

drivers/usb/gadget/composite.c +1 -2

-       composite_driver.speed = min((u8)composite_driver.speed,
-                                    (u8)driver->max_speed);
+       composite_driver.speed = driver->max_speed;

Quoted 2 lines · Yang Rui Rui

This does not works, many drivers will check the driver->speed, ie in musg_gadget.c

Quoted 3 lines · Michal Nazarewicz

Uh? Not sure what you mean here. Note that driver above is not the same structure as driver below. composite_driver is what musb_gadget_start() will get.

For example I got -EINVAL in musb_gadget_start in case composite_driver.speed = USB_SPEED_SUPER

the driver param is the composite_driver in composite.c, isn't it?

Quoted 13 lines

Quoted 13 lines

static int musb_gadget_start(struct usb_gadget *g,
struct usb_gadget_driver *driver)

{
        struct musb             *musb = gadget_to_musb(g);
        unsigned long           flags;
        int                     retval = -EINVAL;

        if (driver->speed != USB_SPEED_HIGH)
                goto err0;

[snip]

Quoted 2 lines

        composite = driver;
        composite_gadget_bind = bind;

From: Michal Nazarewicz <[email protected]>

In a few places kernel wants to print a human-readable USB device speed name (eg. "high") instead of a raw number (eg. 3). Usually a switch is introduced in those places leading to code repetition. To mitigate this issue, this commit introduces usb_device_speed_name() function, which returns a human-readable name of provided speed.

Signed-off-by Michal Nazarewicz <[email protected]>

---
 drivers/usb/Makefile          |    3 +++
 drivers/usb/common.c          |   24 ++++++++++++++++++++++++
 drivers/usb/gadget/udc-core.c |   19 ++-----------------
 include/linux/usb/ch9.h       |    8 ++++++++
 4 files changed, 37 insertions(+), 17 deletions(-)
 create mode 100644 drivers/usb/common.c

drivers/usb/Makefile +3 -0

index 30ddf8d..a3167cc 100644
@@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC)	+= musb/
 obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
 obj-$(CONFIG_USB_OTG_UTILS)	+= otg/
 obj-$(CONFIG_USB_GADGET)	+= gadget/
+
+obj-$(CONFIG_USB)		+= common.o
+obj-$(CONFIG_USB_GADGET)	+= common.o

drivers/usb/common.c new file +0 -0

new file mode 100644
index 0000000..e9d7141

drivers/usb/gadget/udc-core.c +0 -11

index 05ba472..e1ecdbc 100644
@@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
-	struct usb_gadget	*gadget = udc->gadget;
-
-	switch (gadget->speed) {
-	case USB_SPEED_LOW:
-		return snprintf(buf, PAGE_SIZE, "low-speed\n");
-	case USB_SPEED_FULL:
-		return snprintf(buf, PAGE_SIZE, "full-speed\n");
-	case USB_SPEED_HIGH:
-		return snprintf(buf, PAGE_SIZE, "high-speed\n");
-	case USB_SPEED_WIRELESS:
-		return snprintf(buf, PAGE_SIZE, "wireless\n");

Quoted 38 lines · Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

In a few places kernel wants to print a human-readable USB device speed name (eg. "high") instead of a raw number (eg. 3). Usually a switch is introduced in those places leading to code repetition. To mitigate this issue, this commit introduces usb_device_speed_name() function, which returns a human-readable name of provided speed.

Signed-off-by Michal Nazarewicz <[email protected]>

---
 drivers/usb/Makefile          |    3 +++
 drivers/usb/common.c          |   24 ++++++++++++++++++++++++
 drivers/usb/gadget/udc-core.c |   19 ++-----------------
 include/linux/usb/ch9.h       |    8 ++++++++
 4 files changed, 37 insertions(+), 17 deletions(-)
 create mode 100644 drivers/usb/common.c

drivers/usb/Makefile +3 -0

index 30ddf8d..a3167cc 100644
@@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC)	+= musb/
 obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
 obj-$(CONFIG_USB_OTG_UTILS)	+= otg/
 obj-$(CONFIG_USB_GADGET)	+= gadget/
+
+obj-$(CONFIG_USB)		+= common.o
+obj-$(CONFIG_USB_GADGET)	+= common.o

drivers/usb/common.c new file +3 -0

new file mode 100644
index 0000000..e9d7141
@@ -0,0 +1,24 @@
+/*
+ * Provides code common for host and device side USB.
+ */

drivers/usb/common.c line 9

Quoted 6 lines

drivers/usb/common.c new file +6 -0

+
+#include <linux/kernel.h>  /* for ARRAY_SIZE() */
+#include <linux/module.h>  /* for EXPORT_SYMBOL_GPL() */
+#include <linux/usb/ch9.h>
+
+const char *usb_device_speed_name(enum usb_device_speed speed)

somehow usb_speed_string() sounds better to me.

drivers/usb/common.c line 21

Quoted 12 lines

drivers/usb/common.c new file +12 -0

+{
+	static const char *const names[] = {
+		"UNKNOWN",
+		"low-speed",
+		"full-speed",
+		"high-speed",
+		"wireless",
+		"super-speed",
+	};
+
+	if (speed < 0 || speed >= ARRAY_SIZE(names))
+		speed = USB_SPEED_UNKNOWN;

I generally dislike depending on a particular ordering of enumerations. While's it's highly unlikely that anyone will change that, if someone does change, then it'll take a long time to actually find the bug on the print here :-)

I woud rather use a switch in this case.

Quoted 25 lines

drivers/usb/gadget/udc-core.c +0 -16

index 05ba472..e1ecdbc 100644
@@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device  
 *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
-	struct usb_gadget	*gadget = udc->gadget;
-
-	switch (gadget->speed) {
-	case USB_SPEED_LOW:
-		return snprintf(buf, PAGE_SIZE, "low-speed\n");
-	case USB_SPEED_FULL:
-		return snprintf(buf, PAGE_SIZE, "full-speed\n");
-	case USB_SPEED_HIGH:
-		return snprintf(buf, PAGE_SIZE, "high-speed\n");
-	case USB_SPEED_WIRELESS:
-		return snprintf(buf, PAGE_SIZE, "wireless\n");
-	case USB_SPEED_SUPER:
-		return snprintf(buf, PAGE_SIZE, "super-speed\n");
-	case USB_SPEED_UNKNOWN:	/* FALLTHROUGH */
-	default:
-		return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");

drivers/usb/gadget/udc-core.c static ssize_t usb_udc_speed_show(struct device *dev, · line 381

Quoted 5 lines

drivers/usb/gadget/udc-core.c +2 -1

-	}
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			usb_device_speed_name(udc->gadget->speed));
 }
 static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);

you said 'several locations', but there's only one ? No other driver is using something similar ?

From: Michal Nazarewicz <[email protected]>

This commit replaces usb_gadget's is_dualspeed field with a max_speed field.

This change is made so that one will be able to check at run-time if given gadget supports super speed.

drivers/usb/gadget/udc-core.c +1 -1

index e1ecdbc..25058b4 100644
@@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
 }
 static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
 
-static ssize_t usb_udc_speed_show(struct device *dev,
+#define USB_UDC_SPEED_ATTR(name)					\

Quoted 5 lines

This commit replaces usb_gadget's is_dualspeed field with a max_speed field.

This change is made so that one will be able to check at run-time if given gadget supports super speed.

IMHO the logic is inverted here. It should start from the function drivers. They should say which USB speeds they support, that would go up to composite layer and composite would call e.g. usb_gadget_set_speed(gadget, maximum_speed);

Quoted 10 lines

drivers/usb/gadget/udc-core.c +0 -0

index e1ecdbc..25058b4 100644
@@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct  
 device *dev,
 }
 static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,  
 usb_udc_softconn_store);
 

drivers/usb/gadget/udc-core.c static ssize_t usb_udc_softconn_store(struct device *dev, · line 384

Quoted 12 lines

drivers/usb/gadget/udc-core.c +11 -1

-static ssize_t usb_udc_speed_show(struct device *dev,
+#define USB_UDC_SPEED_ATTR(name)					\
+ssize_t usb_udc_##name##_show(struct device *dev,			\
+		struct device_attribute *attr, char *buf)		\
+{									\
+	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);	\
+	return snprintf(buf, PAGE_SIZE, "%s\n",				\
+			usb_device_speed_name(udc->gadget->name));	\
+}									\
+static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
+
+static USB_UDC_SPEED_ATTR(speed);

how about "current_speed" ?

drivers/usb/gadget/udc-core.c static ssize_t usb_udc_softconn_store(struct device *dev, · line 385

Quoted 1 line

drivers/usb/gadget/udc-core.c +1 -0

+static USB_UDC_SPEED_ATTR(max_speed);

in this case, humans will be reading sysfs, so maximum_speed will look nicer, IMHO.

Quoted 8 lines

drivers/usb/gadget/udc-core.c +3 -3

+/* Provide "is_dualspeed" for backward compatibility. */
+static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
-	return snprintf(buf, PAGE_SIZE, "%s\n",
-			usb_device_speed_name(udc->gadget->speed));
+	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);

drivers/usb/gadget/udc-core.c static ssize_t usb_udc_softconn_store(struct device *dev,

Quoted 6 lines

drivers/usb/gadget/udc-core.c +3 -1

+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			gadget_is_dualspeed(udc->gadget));
 }
-static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
+static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show,  
 NULL);

maybe deprecate this one on feature-removal-schedule ??

Sorry, I somehow missed this mail before.

Quoted 6 lines · Michal Nazarewicz

Quoted 5 lines

This commit replaces usb_gadget's is_dualspeed field with a max_speed field.

This change is made so that one will be able to check at run-time if given gadget supports super speed.

Quoted 4 lines · Felipe Balbi

IMHO the logic is inverted here. It should start from the function drivers. They should say which USB speeds they support, that would go up to composite layer and composite would call e.g. usb_gadget_set_speed(gadget, maximum_speed);

This is actually not how composite works at the moment. Currently,

a composite gadget can declare a maximum speed of say “high” even if all the functions do not support that speed. Of course when host asks about descriptors for given speed, only functions that support that speed will be returned (and hence only configurations that have at least one function supporting that speed).

Whether the behaviour should be changed is, in my opinion, issue separate from the patchset that I'm sending.

Quoted 11 lines

Quoted 11 lines

drivers/usb/gadget/udc-core.c +0 -0

index e1ecdbc..25058b4 100644
@@ -371,14 +371,28 @@ static ssize_t
 usb_udc_softconn_store(struct device *dev,
 }
 static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
 usb_udc_softconn_store);
 

Quoted 14 lines

drivers/usb/gadget/udc-core.c static ssize_t usb_udc_softconn_store(struct device *dev, · line 384

Quoted 12 lines

drivers/usb/gadget/udc-core.c +11 -1

-static ssize_t usb_udc_speed_show(struct device *dev,
+#define USB_UDC_SPEED_ATTR(name)					\
+ssize_t usb_udc_##name##_show(struct device *dev,			\
+		struct device_attribute *attr, char *buf)		\
+{									\
+	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);	\
+	return snprintf(buf, PAGE_SIZE, "%s\n",				\
+			usb_device_speed_name(udc->gadget->name));	\
+}									\
+static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
+
+static USB_UDC_SPEED_ATTR(speed);

how about "current_speed" ?

Is there a big advantage? That would change external interface and I don't see reason to do so. Of course, udc class is quite recent so if you feel we can
ignore this issue I can go forward with that change.

Quoted 8 lines

Quoted 8 lines

drivers/usb/gadget/udc-core.c +3 -3

+/* Provide "is_dualspeed" for backward compatibility. */
+static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
-	return snprintf(buf, PAGE_SIZE, "%s\n",
-			usb_device_speed_name(udc->gadget->speed));
+	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);

Quoted 8 lines

drivers/usb/gadget/udc-core.c static ssize_t usb_udc_softconn_store(struct device *dev,

Quoted 6 lines

drivers/usb/gadget/udc-core.c +3 -1

+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			gadget_is_dualspeed(udc->gadget));
 }
-static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
+static DEVICE_ATTR(is_dualspeed, S_IRUSR,
 usb_udc_is_dualspeed_show, NULL);

maybe deprecate this one on feature-removal-schedule ??

Sure.

Also, if we decide to change “speed” to “current_speed”, we could just drop this without the announcement.

Quoted 6 lines · Felipe Balbi

Quoted 3 lines

IMHO the logic is inverted here. It should start from the function
drivers. They should say which USB speeds they support, that would go up to composite layer and composite would call e.g. usb_gadget_set_speed(gadget, maximum_speed);

This is actually not how composite works at the moment. Currently,

my suggestion was exactly to change that :-) Speed is something functions support. composite.c shouldn't dictate which speed functions should use, rather composite.c should use the maximum speed which all functions support.

Quoted 6 lines

a composite gadget can declare a maximum speed of say “high” even if all the functions do not support that speed. Of course when host asks about descriptors for given speed, only functions that support that speed will be returned (and hence only configurations that have at least one function supporting that speed).

Quoted 2 lines

Whether the behaviour should be changed is, in my opinion, issue separate from the patchset that I'm sending.

I wouldn't say that, actually. Just replacing is_dualspeed with max_speed isn't changing much and if you want to make that part of the code better, why not doing The Right Thing(TM) ?

All of the speed negotiation between composite.c and f_*.c should happen before even connecting to host

(before attaching data pullups, enabling IRQs, etc), that's exactly why me and Sebastian have decided (at that time off list) to add udc_start()/udc_stop() methods.

One of the reason was that it would be a quite intrusive change to all UDC drivers, second we wanted to give maintainers/authors of those UDC drivers some grace period for the change, third when all UDCs are converted, it allow us to do the speed negotiation before connecting to host.

Quoted 4 lines

Quoted 1 line

how about "current_speed" ?

Is there a big advantage? That would change external interface and I don't see reason to do so. Of course, udc class is quite recent so if you feel we can ignore this issue I can go forward with that change.

you already maximum_speed (below) and speed alone looses some extra hint of what kind of information will be there. I think it's better to change this to current_speed and make a symbolic link called 'speed' which we can keep for the next 5 years and remove it in e.g. Linux v5.0

My understanding was that one could consciously create a composite gadget such that not all of the functions support all of the speeds.

Quoted 2 lines · Michal Nazarewicz

My understanding was that one could consciously create a composite gadget such that not all of the functions support all of the speeds.

That's an important point. If somebody wants to build a composite gadget that includes both midi and storage support, the storage driver would be able to go all the way up to SuperSpeed (or rather, it will once Felipe's patch is finished and merged) but the midi driver would only support full speed. So what speed should the UDC connect at?

In cases like this there's really no way to know. Ultimately the best approach would be to update the midi driver, but that's a long-term sort of thing.

Usually when the kernel can't decide something, it asks the user for help (by allowing the user to specify the speed via a module parameter, for instance). In this case even that wouldn't work, because the gadget's userspace isn't aware of which functions the host's user will want to utilize.

At the moment I don't know the best answer. This is something we should all think about.

I stand with the fact that if they are part of the same configuration, there's no way around it, storage will have to come down to full speed. Unless I'm missing something.

Quoted 3 lines · Felipe Balbi

I stand with the fact that if they are part of the same configuration, there's no way around it, storage will have to come down to full speed. Unless I'm missing something.

If you look at config_buf() in composite.c, you'll notice that composite just skips functions that do not support current speed. This means, that if a connection uses high speed but given function can handle only full speed, it won't be included in the descriptors.

config_buf() is called from config_desc() which similarly, iterates over configurations skipping the ones that do not support given speed.

Configuration is defined to support given speed if at least one function that is added to that configuration supports that speed (see usb_add_function()).

Quoted 10 lines

If you look at config_buf() in composite.c, you'll notice that composite
just skips functions that do not support current speed. This means, that if a connection uses high speed but given function can handle only full speed, it won't be included in the descriptors.

config_buf() is called from config_desc() which similarly, iterates over configurations skipping the ones that do not support given speed.

Configuration is defined to support given speed if at least one function that is added to that configuration supports that speed (see usb_add_function()).

I'm trying to think if this is really what we want. This might mean $current_config will be different depending if we connect it to SS, HS or FS hubs.

Quoted 7 lines · Michal Nazarewicz

Quoted 6 lines

If you look at config_buf() in composite.c, you'll notice that composite just skips functions that do not support current speed. This means, that if a connection uses high speed but given function can handle only
full speed, it won't be included in the descriptors.

Quoted 6 lines

Quoted 6 lines

config_buf() is called from config_desc() which similarly, iterates over configurations skipping the ones that do not support given speed.

Configuration is defined to support given speed if at least one function that is added to that configuration supports that speed (see usb_add_function()).

Quoted 3 lines · Felipe Balbi

I'm trying to think if this is really what we want. This might mean $current_config will be different depending if we connect it to SS, HS or FS hubs.

What do you mean by $current_config? The bConfigurationValue will be

the same for "the same" configuration regardless of speed but different configurations may be available for different speeds and "the same" configuration in various speeds might have different functions.

So just for the sake of having something concrete, let's say we have:

Configuration #1 with
function A supporting FS and HS and

function B supporting FS only.
Configuration #2 with
function B supporting FS only.
Configuration #3 with
function A supporting FS and HS and
function C supporting HS only (dunno if that makes sense).

When connected to FS hub, we will return the following:

Configuration #1: A, B
Configuration #2: B
Configuration #3: A

When connect to HS hub, we will return the following:

Configuration #1: A

Quoted 4 lines · Felipe Balbi

Quoted 3 lines

I'm trying to think if this is really what we want. This might mean $current_config will be different depending if we connect it to SS, HS or FS hubs.

Quoted 1 line · Michal Nazarewicz

What do you mean by $current_config? The bConfigurationValue will be

the currently chosen configuration.

Quoted 4 lines

the same for "the same" configuration regardless of speed but different configurations may be available for different speeds and "the same" configuration in various speeds might have different functions.

Quoted 4 lines

So just for the sake of having something concrete, let's say we have:

Configuration #1 with
function A supporting FS and HS and

my question is what happens with the HS support on function A (descriptors, endpoints, etc) when you connect it to a HS hub or to a FS hub.

Quoted 13 lines

function B supporting FS only.
Configuration #2 with
function B supporting FS only.
Configuration #3 with
function A supporting FS and HS and
function C supporting HS only (dunno if that makes sense).

When connected to FS hub, we will return the following:

Configuration #1: A, B
Configuration #2: B
Configuration #3: A

Quoted 3 lines

When connect to HS hub, we will return the following:

Configuration #1: A

See, I'm not sure this is allowed by USB spec. But I could be wrong. (I mean having a function disappear)

Hi,

Quoted 10 lines · Alan Stern

Quoted 2 lines · Michal Nazarewicz

My understanding was that one could consciously create a composite gadget such that not all of the functions support all of the speeds.

That's an important point. If somebody wants to build a composite gadget that includes both midi and storage support, the storage driver would be able to go all the way up to SuperSpeed (or rather, it will once Felipe's patch is finished and merged) but the midi driver would only support full speed. So what speed should the UDC connect at?

if they are both part of the of the same configuration, full speed. Otherwise Audio won't work. If they are part of different configurations, then maximum_speed will be defined by the chosen config, no ?

Quoted 3 lines

In cases like this there's really no way to know. Ultimately the best approach would be to update the midi driver, but that's a long-term sort of thing.

yeah, I agree here.

Quoted 6 lines

Usually when the kernel can't decide something, it asks the user for help (by allowing the user to specify the speed via a module parameter, for instance). In this case even that wouldn't work, because the gadget's userspace isn't aware of which functions the host's user will want to utilize.

Quoted 2 lines

At the moment I don't know the best answer. This is something we should all think about.

I stand with the fact that if they are part of the same configuration, there's no way around it, storage will have to come down to full speed. Unless I'm missing something.

Quoted 4 lines

if they are both part of the of the same configuration, full speed. Otherwise Audio won't work. If they are part of different configurations, then maximum_speed will be defined by the chosen config, no ?

No. The speed has to be chosen first, when the gadget connects to the host. The config isn't chosen until later, after the host retrieves the various descriptors and decides among them.

Quoted 3 lines

I stand with the fact that if they are part of the same configuration, there's no way around it, storage will have to come down to full speed. Unless I'm missing something.

What if they are in different configurations?

Hi,

Quoted 8 lines · Alan Stern

Quoted 4 lines

if they are both part of the of the same configuration, full speed. Otherwise Audio won't work. If they are part of different configurations, then maximum_speed will be defined by the chosen config, no ?

No. The speed has to be chosen first, when the gadget connects to the host. The config isn't chosen until later, after the host retrieves the various descriptors and decides among them.

true

Quoted 5 lines

Quoted 3 lines

I stand with the fact that if they are part of the same configuration, there's no way around it, storage will have to come down to full speed. Unless I'm missing something.

What if they are in different configurations?

then you use the fastest speed between both configurations?

For the time being, perhaps the best answer is to use the _lowest_ max_speed value among all the function drivers. It's the simplest answer and most likely to be correct most of the time.

Quoted 3 lines · Alan Stern

For the time being, perhaps the best answer is to use the _lowest_ max_speed value among all the function drivers. It's the simplest answer and most likely to be correct most of the time.

In theory, one could create a composite function with only SS/HS descriptors.

So it would work for SS/HS but not for FS. I'm not
saying that it would make sense (or that it's not a bug) but that adds to the discussion.

Quoted 8 lines · Michal Nazarewicz

Quoted 3 lines · Alan Stern

For the time being, perhaps the best answer is to use the _lowest_ max_speed value among all the function drivers. It's the simplest answer and most likely to be correct most of the time.

In theory, one could create a composite function with only SS/HS descriptors.

The only reason for doing that would be if the function was incapable of carrying out its duties when running at full speed (insufficient bandwidth, for example). Something like this is discussed briefly in section 9.2.6.6 of the USB-2 spec.

Quoted 3 lines

So it would work for SS/HS but not for FS. I'm not
saying that it would make sense (or that it's not a bug) but that adds to the discussion.

If somebody writes a composite gadget containing two functions, one of which supports only full speed and the other only high speed, they'll get what they deserve. :-)

Still, maybe I was wrong. Maybe it would be better to use the fastest speed supported by at least one of the function drivers. The user can always force a SuperSpeed-capable device to run at high speed by using a USB-2 cable to plug it in. I'm not sure whether the user could force such a device to run at full speed, however.

Still, maybe I was wrong. Maybe it would be better to use the fastest speed supported by at least one of the function drivers. The user can always force a SuperSpeed-capable device to run at high speed by using a USB-2 cable to plug it in. I'm not sure whether the user could force such a device to run at full speed, however.

Quoted 5 lines · Alan Stern

Still, maybe I was wrong. Maybe it would be better to use the fastest speed supported by at least one of the function drivers. The user can always force a SuperSpeed-capable device to run at high speed by using a USB-2 cable to plug it in. I'm not sure whether the user could force such a device to run at full speed, however.

I'm actually wondering whether we really need to worry about it. composite_driver has the “max_speed” field and I would just leave it as composite driver's author responsibility to put correct value there.

One thing that could be worth doing is iterate over all the configurations and figure out if at least one supports the speed declared by “max_speed” and if not, lower it.

Like I've said before, as additional functionality, composite.c could check if it's USB_SPEED_UNKWONW in which case it would look for the lowest speed that all the functions support.

Such configuration would allow composite driver authors set the speed to USB_SPEED_SUPER when they mean “choose maximum speed at least one function supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed all the functions support”.

Does that make sense?

Quoted 6 lines

I'm actually wondering whether we really need to worry about it. composite_driver has the “max_speed” field and I would just leave it as composite driver's author responsibility to put correct value there.

One thing that could be worth doing is iterate over all the configurations and figure out if at least one supports the speed declared by “max_speed” and if not, lower it.

That's the same as what I said: Use the highest speed supported by any function driver.

Quoted 2 lines

Like I've said before, as additional functionality, composite.c could
check if it's USB_SPEED_UNKWONW in which case it would look for the lowest speed that all the functions support.

It's reasonable. Just be sure to document it properly; otherwise nobody will understand or remember it!

I've found where my reasoning was faulty. The usb_gadget_driver's max_speed is set before all the functions get added so composite.c has no way to figure those things in advance. That's why we need to relay on usb_composite_driver's max_speed be set to a proper value.

Quoted 4 lines · Michal Nazarewicz

I've found where my reasoning was faulty. The usb_gadget_driver's max_speed is set before all the functions get added so composite.c has no way to figure those things in advance. That's why we need to relay on usb_composite_driver's max_speed be set to a proper value.

This is what Felipe was complaining about earlier. We shouldn't set max_speed or allow the UDC to connect to the host until all the functions have been added.

Quoted 10 lines · Alan Stern

Quoted 4 lines · Michal Nazarewicz

I've found where my reasoning was faulty. The usb_gadget_driver's max_speed is set before all the functions get added so composite.c has no way to figure those things in advance. That's why we need to relay on usb_composite_driver's max_speed be set to a proper value.

This is what Felipe was complaining about earlier. We shouldn't set max_speed or allow the UDC to connect to the host until all the functions have been added.

We're not doing the latter. The functions are added in bind callback so (hopefully) UDC will first run the bind callback and then try connecting to host.

And the former would be fixed if we allowed gadget driver to set max_speed in bind callback rather then prior to calling usb_gadget_probe_driver().

there's one catch. As of today, we always start UDCs with data pullups connected, which means that we could connect to a host even before a gadget driver is loaded. My point in moving to udc_start/udc_stop is that the above would be take care of. See udc-core.c: [...]

If all UDCs are converted to udc_start()/udc_stop() we get the guarantee that they will only conect to host after gadget driver is fully loaded for free.

We can also, finally, properly use the usb_function_deactivate/ usb_function_activate properly. So for each registered function, composite.c calls usb_function_deactivate() and function is _required_ to call usb_function_activate when it's ready.

Then, when on gadget driver's bind() we can take this kind of speed decision and pass that on to UDC driver.

Quoted 4 lines · Felipe Balbi

there's one catch. As of today, we always start UDCs with data pullups connected, which means that we could connect to a host even before a gadget driver is loaded. My point in moving to udc_start/udc_stop is that the above would be take care of. See udc-core.c: [...]

Honestly I'm not quite sure why udc_start/udc_stop is needed here. Even without those the UDC driver can start with all hw disabled and turn it on only after the gadget driver's bind callback finishes.

Quoted 8 lines

If all UDCs are converted to udc_start()/udc_stop() we get the guarantee that they will only conect to host after gadget driver is fully loaded for free.

We can also, finally, properly use the usb_function_deactivate/ usb_function_activate properly. So for each registered function, composite.c calls usb_function_deactivate() and function is _required_ to call usb_function_activate when it's ready.

I'm not really sure why that would be beneficial. Also, it would still require disconnect-connect cycle if some function decides to (de)activate itself while gadget is connected.

Quoted 2 lines

Then, when on gadget driver's bind() we can take this kind of speed decision and pass that on to UDC driver.

So can we leave things as they are for now and wait for UDCs to be converted
and once this is done, do all kinds of magic we want in copomiset's bind callback?

Quoted 8 lines · Michal Nazarewicz

Quoted 5 lines · Alan Stern

Still, maybe I was wrong. Maybe it would be better to use the fastest speed supported by at least one of the function drivers. The user can always force a SuperSpeed-capable device to run at high speed by using a USB-2 cable to plug it in. I'm not sure whether the user could force such a device to run at full speed, however.

Quoted 7 lines

I'm actually wondering whether we really need to worry about it. composite_driver has the “max_speed” field and I would just leave it as composite driver's author responsibility to put correct value there.

One thing that could be worth doing is iterate over all the configurations and figure out if at least one supports the speed declared by “max_speed” and if not, lower it.

That's the same as what I said: Use the highest speed supported by any function driver. There really isn't any point in allowing connections faster than that. And as Felipe pointed out, the user always has the capability to force the connection to be slower, by using the right sorts of cables and hubs.

Quoted 9 lines

Like I've said before, as additional functionality, composite.c could check if it's USB_SPEED_UNKWONW in which case it would look for the lowest speed that all the functions support.

Such configuration would allow composite driver authors set the speed to USB_SPEED_SUPER when they mean “choose maximum speed at least one function supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed all the functions support”.

Quoted 1 line

Does that make sense?

It's reasonable. Just be sure to document it properly; otherwise nobody will understand or remember it!

Quoted 8 lines · Felipe Balbi

Quoted 7 lines · Felipe Balbi

Quoted 4 lines

IMHO the logic is inverted here. It should start from the function drivers. They should say which USB speeds they support, that would go up to composite layer and composite would call e.g. usb_gadget_set_speed(gadget, maximum_speed);

This is actually not how composite works at the moment. Currently,

Quoted 4 lines

my suggestion was exactly to change that :-) Speed is something functions support. composite.c shouldn't dictate which speed functions should use, rather composite.c should use the maximum speed which all functions support.

Strictly speaking, composite.c does not dictate anything. It just copies
what the composite gadget driver declared as maximum speed.

My understanding was that one could consciously create a composite gadget such that not all of the functions support all of the speeds.

Quoted 9 lines

Quoted 8 lines

a composite gadget can declare a maximum speed of say “high” even if all the functions do not support that speed. Of course when host asks about descriptors for given speed, only functions that support that speed will be returned (and hence only configurations that have at least one function supporting that speed).

Whether the behaviour should be changed is, in my opinion, issue separate from the patchset that I'm sending.

Quoted 3 lines

I wouldn't say that, actually. Just replacing is_dualspeed with max_speed isn't changing much and if you want to make that part of the code better, why not doing The Right Thing(TM) ?

I'm just saying that the main reason for this patchset is to get rid of the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. So

the purpose of changing is_dualspeed with max_speed is to be able to check if gadget is super speed at run-time so that gadget_is_superspeed() can be implemented.

So I would like to just get this done and then figure out what to do with composite.c. How does that sound?

Quoted 2 lines

All of the speed negotiation between composite.c and f_*.c should happen before even connecting to host

Yep, obviously. The usb_gadget_probe_driver() is called at the very and once all the functions and everything is added so composite.c can do all the analysis it wants and figure out the maximum speed.

Quoted 3 lines

(before attaching data pullups, enabling IRQs, etc), that's exactly why me and Sebastian have decided (at that time off list) to add udc_start()/udc_stop() methods.

I don't really follow why those would be needed...

Quoted 5 lines

One of the reason was that it would be a quite intrusive change to all UDC drivers, second we wanted to give maintainers/authors of those UDC drivers some grace period for the change, third when all UDCs are converted, it allow us to do the speed negotiation before connecting to host.

Again, I don't follow. We can figure out the max_speed before calling usb_gadget_probe_driver() just fine. We don't even have to have UDC to figure that out (ie. gadget driver's max_speed does not change depending on UDC, right?).

Quoted 4 lines

you already maximum_speed (below) and speed alone looses some extra hint of what kind of information will be there. I think it's better to change this to current_speed and make a symbolic link called 'speed' which we can keep for the next 5 years and remove it in e.g. Linux v5.0

OK, I'll do that (as soon as I figure out/recall how to make symlinks that is ;) ).

Quoted 2 lines · Felipe Balbi

Quoted 1 line

All of the speed negotiation between composite.c and f_*.c should happen before even connecting to host

Quoted 9 lines · Michal Nazarewicz

Yep, obviously. The usb_gadget_probe_driver() is called at the very and once all the functions and everything is added so composite.c can do all the analysis it wants and figure out the maximum speed.

Quoted 3 lines

(before attaching data pullups, enabling IRQs, etc), that's exactly why me and Sebastian have decided (at that time off list) to add udc_start()/udc_stop() methods.

I don't really follow why those would be needed...

Ok, I guess I need to give the full picture here, my bad.

Let's say you have a SuperSpeed controller, but you know that this particular gadget driver can only support fullspeed, so why do you need to go through RX detection, HS chirp sequence and whatnot if you can decide the maximum_speed before kickstarting the UDC's state machine ?

Quoted 6 lines

Quoted 4 lines

you already maximum_speed (below) and speed alone looses some extra hint of what kind of information will be there. I think it's better to change this to current_speed and make a symbolic link called 'speed' which we can keep for the next 5 years and remove it in e.g. Linux v5.0

OK, I'll do that (as soon as I figure out/recall how to make symlinks that is ;) ).

yeah, I would have to go through the same re-education ;-)

(please add a note on feature-removal-schedule too)