Skip to contentSkip to conversation

usb: increase USB descriptor buffer size

PATCH New Stefan Brüns 3 participants 10 messages Dec 13, 2015 – Dec 19, 2015

lore
Share & tools
lore

Stefan Brüns

author
#0 lore raw

PATCH 1/7 usb: increase USB descriptor buffer size

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>
---
 common/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/usb.c +1 -1
index 700bfc3..c276bf2 100644
@@ -41,7 +41,7 @@
 #include <asm/4xx_pci.h>
 #endif
 
-#define USB_BUFSIZ	512
+#define USB_BUFSIZ	1024
 
 static int asynch_allowed;
 char usb_started; /* flag for the started/stopped USB status */
Signature · 1 line
2.1.4
#1 lore raw

PATCH 1/7 usb: increase USB descriptor buffer size

Quoted 4 lines · Stefan Br?ns

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>

Can the size be determined in a dynamic manner instead of this ad-hoc random number ?

Best regards,
Marek Vasut

author
#2 lore raw

PATCH 1/7 usb: increase USB descriptor buffer size

Quoted 8 lines · On Sunday 13 December 2015 05:49:24 Marek Vasut wrote:
Quoted 4 lines · Stefan Br?ns

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>

Can the size be determined in a dynamic manner instead of this ad-hoc random number ?

Currently the buffer is allocated on the stack with ALLOC_CACHE_ALIGN_BUFFER in usb_select_config(... The buffer is passed to usb_get_configuration_no(...), but the buffer size is implicit.

usb_get_configuration_no(...) already determines and checks the descriptor size, but as it can not readjust the buffer it just bails out if it is to small.

The absolute maximum size for the descriptor is 64kB.

So yes, this is possible, but needs a little bit more work.

Kind regards,

Stefan

Signature · 3 lines
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424
#3 lore raw

PATCH 1/7 usb: increase USB descriptor buffer size

Quoted 14 lines · Stefan Bruens
Quoted 8 lines · On Sunday 13 December 2015 05:49:24 Marek Vasut wrote:
Quoted 4 lines · Stefan Br?ns

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>

Can the size be determined in a dynamic manner instead of this ad-hoc random number ?

Currently the buffer is allocated on the stack with ALLOC_CACHE_ALIGN_BUFFER in usb_select_config(... The buffer is passed to usb_get_configuration_no(...), but the buffer size is implicit.

Quoted 7 lines

usb_get_configuration_no(...) already determines and checks the descriptor size, but as it can not readjust the buffer it just bails out if it is to small.

The absolute maximum size for the descriptor is 64kB.

So yes, this is possible, but needs a little bit more work.

Do you feel like working on it ?

Best regards,
Marek Vasut

author
#4 lore raw

PATCH 1/1 usb: Alloc buffer for USB descriptor dynamically

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 bytes may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>
---
 common/usb.c  | 41 +++++++++++++++++++++++++++--------------
 include/usb.h |  5 +++--
 2 files changed, 30 insertions(+), 16 deletions(-)
common/usb.c +27 -14
index 1d0a151..061bc85 100644
@@ -566,13 +566,12 @@ static int usb_get_descriptor(struct usb_device *dev, unsigned char type,
 }
 
 /**********************************************************************
- * gets configuration cfgno and store it in the buffer
+ * gets len of configuration cfgno
  */
-int usb_get_configuration_no(struct usb_device *dev,
-			     unsigned char *buffer, int cfgno)
+int usb_get_configuration_len(struct usb_device *dev, int cfgno)
 {
 	int result;
-	unsigned int length;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, 9);
 	struct usb_config_descriptor *config;
 
 	config = (struct usb_config_descriptor *)&buffer[0];

@@ -586,17 +585,23 @@ int usb_get_configuration_no(struct usb_device *dev,
 				"(expected %i, got %i)\n", 9, result);
 		return -EIO;
 	}
-	length = le16_to_cpu(config->wTotalLength);
+	return le16_to_cpu(config->wTotalLength);
+}
 
-	if (length > USB_BUFSIZ) {
-		printf("%s: failed to get descriptor - too long: %d\n",
-			__func__, length);
-		return -EIO;
-	}
+/**********************************************************************
+ * gets configuration cfgno and store it in the buffer
+ */
+int usb_get_configuration_no(struct usb_device *dev, int cfgno,
+			     unsigned char *buffer, int length)
+{
+	int result;
+	struct usb_config_descriptor *config;
 
+	config = (struct usb_config_descriptor *)&buffer[0];
 	result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, length);
-	debug("get_conf_no %d Result %d, wLength %d\n", cfgno, result, length);
-	config->wTotalLength = length; /* validated, with CPU byte order */
+	debug("get_conf_no %d Result %d, wLength %d\n", cfgno, result,
+	      le16_to_cpu(config->wTotalLength));
+	config->wTotalLength = result; /* validated, with CPU byte order */
 
 	return result;
 }

@@ -1070,7 +1075,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
 
 int usb_select_config(struct usb_device *dev)
 {
-	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
+	unsigned char *tmpbuf;
 	int err;
 
 	err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE);

@@ -1084,7 +1089,14 @@ int usb_select_config(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 
 	/* only support for one config for now */
-	err = usb_get_configuration_no(dev, tmpbuf, 0);
+	err = usb_get_configuration_len(dev, 0);
+	if (err >= 0) {
+		tmpbuf = (unsigned char *)malloc_cache_aligned(err);
+		if (!tmpbuf)
+			err = -ENOMEM;
+		else
+			err = usb_get_configuration_no(dev, 0, tmpbuf, err);
+	}
 	if (err < 0) {
 		printf("usb_new_device: Cannot read configuration, " \
 		       "skipping device %04x:%04x\n",

@@ -1092,6 +1104,7 @@ int usb_select_config(struct usb_device *dev)
 		return err;
 	}
 	usb_parse_config(dev, tmpbuf, 0);
+	free(tmpbuf);
 	usb_set_maxpacket(dev);
 	/*
 	 * we set the default configuration here
include/usb.h +3 -2
index 6e12876..9efc2ca 100644
@@ -266,8 +266,9 @@ int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
 			void *buffer, int transfer_len, int interval);
 int usb_disable_asynch(int disable);
 int usb_maxpacket(struct usb_device *dev, unsigned long pipe);
-int usb_get_configuration_no(struct usb_device *dev, unsigned char *buffer,
-				int cfgno);
+int usb_get_configuration_no(struct usb_device *dev, int cfgno,
+			unsigned char *buffer, int length);
+int usb_get_configuration_len(struct usb_device *dev, int cfgno);
 int usb_get_report(struct usb_device *dev, int ifnum, unsigned char type,
 			unsigned char id, void *buf, int size);
 int usb_get_class_descriptor(struct usb_device *dev, int ifnum,
Signature · 1 line
2.1.4
#6 lore raw

PATCH 1/1 usb: Alloc buffer for USB descriptor dynamically

In reply to Stefan Brüns

Hi,

Quoted 8 lines · Stefan Br?ns

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 bytes may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>
---
 common/usb.c  | 41 +++++++++++++++++++++++++++--------------
 include/usb.h |  5 +++--
 2 files changed, 30 insertions(+), 16 deletions(-)

Won't this call malloc() on every request? Can we avoid that?

Regards,
Simon

author
#7 lore raw

PATCH 1/1 usb: Alloc buffer for USB descriptor dynamically

Quoted 3 lines · On Friday 18 December 2015 15:41:10 Simon Glass wrote:

Hi,

On 17 December 2015 at 18:07, Stefan Br?ns <[email protected]>

Quoted 11 lines · wrote:
Quoted 9 lines

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 bytes may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>

---

 common/usb.c  | 41 +++++++++++++++++++++++++++--------------
 include/usb.h |  5 +++--
 2 files changed, 30 insertions(+), 16 deletions(-)

Won't this call malloc() on every request? Can we avoid that?

This only affects the configuration descriptor, so one allocation per device.

One could use a stack allocated buffer for small, e.g. 512 bytes, descriptors, and use dynamic allocation only for larger one, but I don't think this is worth the effort.

Kind regards,

Stefan

Signature · 3 lines
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424
#8 lore raw

PATCH 1/1 usb: Alloc buffer for USB descriptor dynamically

Hi Stafan,

Quoted 19 lines · Stefan Bruens
Quoted 3 lines · On Friday 18 December 2015 15:41:10 Simon Glass wrote:

Hi,

On 17 December 2015 at 18:07, Stefan Br?ns <[email protected]>

Quoted 11 lines · wrote:
Quoted 9 lines

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 bytes may not be enough.

Signed-off-by Stefan Br?ns <[email protected]>

---

 common/usb.c  | 41 +++++++++++++++++++++++++++--------------
 include/usb.h |  5 +++--
 2 files changed, 30 insertions(+), 16 deletions(-)

Won't this call malloc() on every request? Can we avoid that?

This only affects the configuration descriptor, so one allocation per device.

Quoted 7 lines

One could use a stack allocated buffer for small, e.g. 512 bytes, descriptors, and use dynamic allocation only for larger one, but I don't think this is worth the effort.

Kind regards,

Stefan

Another approach would be to have a single buffer for the USB stack and increase its size as needed. But if this is only once per device it does not seem worth it either.

Don't you need to free the buffer if usb_get_configuration_no() returns an error?

Otherwise:

Reviewed-by Simon Glass <[email protected]>

Regards,
Simon

author
#9 lore raw

PATCH 1/1 usb: Alloc buffer for USB descriptor dynamically

Quoted 33 lines · On Saturday 19 December 2015 13:29:38 Simon Glass wrote:

Hi Stafan,

On 19 December 2015 at 08:23, Stefan Bruens

Quoted 28 lines · <[email protected]> wrote:
Quoted 3 lines · On Friday 18 December 2015 15:41:10 Simon Glass wrote:

Hi,

On 17 December 2015 at 18:07, Stefan Br?ns <[email protected]>

Quoted 4 lines · wrote:
Quoted 4 lines

The configuration descriptor includes all interface, endpoint and auxiliary descriptors (e.g. report, union) so 512 bytes may not be enough.

Quoted 8 lines
Quoted 6 lines
Signed-off-by Stefan Br?ns <[email protected]>

---

 common/usb.c  | 41 +++++++++++++++++++++++++++--------------
 include/usb.h |  5 +++--
 2 files changed, 30 insertions(+), 16 deletions(-)

Won't this call malloc() on every request? Can we avoid that?

This only affects the configuration descriptor, so one allocation per device.

One could use a stack allocated buffer for small, e.g. 512 bytes, descriptors, and use dynamic allocation only for larger one, but I don't think this is worth the effort.

Kind regards,

Quoted 8 lines
Quoted 1 line

Stefan

Another approach would be to have a single buffer for the USB stack and increase its size as needed. But if this is only once per device it does not seem worth it either.

Don't you need to free the buffer if usb_get_configuration_no() returns an error?

Yes, there's a free() missing in the error path.

Kind regards,

Stefan

Signature · 3 lines
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424