[U-Boot] [PATCH 1/7] usb: increase USB descriptor buffer size

[U-Boot] [PATCH 1/7] usb: increase USB descriptor buffer size

[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 <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 */
-- 
2.1.4

[U-Boot] [PATCH 1/7] usb: increase USB descriptor buffer size

[Marek Vasut]

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

[U-Boot] [PATCH 1/7] usb: increase USB descriptor buffer size

[Stefan Bruens]

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

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

[U-Boot] [PATCH 1/7] usb: increase USB descriptor buffer size

[Marek Vasut]

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

[U-Boot] [PATCH] usb: Alloc buffer for USB descriptor dynamically

[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 <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,
-- 
2.1.4

[U-Boot] [PATCH] usb: Alloc buffer for USB descriptor dynamically

[Marek Vasut]

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

[U-Boot] [PATCH] usb: Alloc buffer for USB descriptor dynamically

[Simon Glass]

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

[U-Boot] [PATCH] usb: Alloc buffer for USB descriptor dynamically

[Stefan Bruens]

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

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

[U-Boot] [PATCH] usb: Alloc buffer for USB descriptor dynamically

[Simon Glass]

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

[U-Boot] [PATCH] usb: Alloc buffer for USB descriptor dynamically

[Stefan Bruens]

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
-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424