[U-Boot] [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 stefan.bruens@rwth-aachen.de --- common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb.c b/common/usb.c index 700bfc3..c276bf2 100644 --- a/common/usb.c +++ b/common/usb.c @@ -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 */
On Sunday, December 13, 2015 at 05:47:18 AM, Stefan Brüns wrote:
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 stefan.bruens@rwth-aachen.de
Can the size be determined in a dynamic manner instead of this ad-hoc random number ?
Best regards, Marek Vasut
On Sunday 13 December 2015 05:49:24 Marek Vasut wrote:
On Sunday, December 13, 2015 at 05:47:18 AM, Stefan Brüns wrote:
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 stefan.bruens@rwth-aachen.de
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
On Sunday, December 13, 2015 at 07:35:19 PM, Stefan Bruens wrote:
On Sunday 13 December 2015 05:49:24 Marek Vasut wrote:
On Sunday, December 13, 2015 at 05:47:18 AM, Stefan Brüns wrote:
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 stefan.bruens@rwth-aachen.de
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.
Do you feel like working on it ?
Best regards, Marek Vasut
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 stefan.bruens@rwth-aachen.de --- common/usb.c | 41 +++++++++++++++++++++++++++-------------- include/usb.h | 5 +++-- 2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 1d0a151..061bc85 100644 --- a/common/usb.c +++ b/common/usb.c @@ -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 diff --git a/include/usb.h b/include/usb.h index 6e12876..9efc2ca 100644 --- a/include/usb.h +++ b/include/usb.h @@ -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,
On Friday, December 18, 2015 at 02:07:21 AM, Stefan Brüns wrote:
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 stefan.bruens@rwth-aachen.de
Fine with me,
Reviewed-by: Marek Vasut marex@denx.de
[...]
Best regards, Marek Vasut
Hi,
On 17 December 2015 at 18:07, Stefan Brüns stefan.bruens@rwth-aachen.de wrote:
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 stefan.bruens@rwth-aachen.de
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
On Friday 18 December 2015 15:41:10 Simon Glass wrote:
Hi,
On 17 December 2015 at 18:07, Stefan Brüns stefan.bruens@rwth-aachen.de
wrote:
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 stefan.bruens@rwth-aachen.de
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
Hi Stafan,
On 19 December 2015 at 08:23, Stefan Bruens stefan.bruens@rwth-aachen.de wrote:
On Friday 18 December 2015 15:41:10 Simon Glass wrote:
Hi,
On 17 December 2015 at 18:07, Stefan Brüns stefan.bruens@rwth-aachen.de
wrote:
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 stefan.bruens@rwth-aachen.de
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
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 sjg@chromium.org
Regards, Simon
On Saturday 19 December 2015 13:29:38 Simon Glass wrote:
Hi Stafan,
On 19 December 2015 at 08:23, Stefan Bruens
stefan.bruens@rwth-aachen.de wrote:
On Friday 18 December 2015 15:41:10 Simon Glass wrote:
Hi,
On 17 December 2015 at 18:07, Stefan Brüns stefan.bruens@rwth-aachen.de
wrote:
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 stefan.bruens@rwth-aachen.de
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
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
participants (4)
-
Marek Vasut -
Simon Glass -
Stefan Bruens -
Stefan Brüns