Patchwork [RESEND] usb: increase buffer for USB control requests

login
register
mail settings
Submitter Christian Krause
Date Jan. 24, 2010, 4:34 p.m.
Message ID <20100124163452.GB8270@gondor.middle-earth.priv>
Download mbox | patch
Permalink /patch/43599/
State New
Headers show

Comments

Christian Krause - Jan. 24, 2010, 4:34 p.m.
Resend. The patch was already sent to the list on 2009-12-11. It would
be great if it could be reviewed and applied. Thank you very much
in advance.

The WLAN USB stick ZyXEL NWD271N (0586:3417) uses very large
usb control transfers of more than 2048 bytes which won't fit
into the buffer of the ctrl_struct. This results in an error message
"husb: ctrl buffer too small" and a non-working device.
Increasing the buffer size to 8192 seems to be a safe choice.

Signed-off-by: Christian Krause <chkr@plauener.de>
---
 usb-linux.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Michael S. Tsirkin - Jan. 24, 2010, 9:20 p.m.
On Sun, Jan 24, 2010 at 05:34:52PM +0100, Christian Krause wrote:
> Resend. The patch was already sent to the list on 2009-12-11. It would
> be great if it could be reviewed and applied. Thank you very much
> in advance.
> 
> The WLAN USB stick ZyXEL NWD271N (0586:3417) uses very large
> usb control transfers of more than 2048 bytes which won't fit
> into the buffer of the ctrl_struct. This results in an error message
> "husb: ctrl buffer too small" and a non-working device.
> Increasing the buffer size to 8192 seems to be a safe choice.
> 
> Signed-off-by: Christian Krause <chkr@plauener.de>

Are there any drawbacks to make\ing the buffer larger?
If no, let's just make it 64K? IIUC that's a maximum
length for control transfers as length is a 16 bit field.


> ---
>  usb-linux.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/usb-linux.c b/usb-linux.c
> index 285ac22..d205bd3 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -113,7 +113,7 @@ struct ctrl_struct {
>      uint16_t offset;
>      uint8_t  state;
>      struct   usb_ctrlrequest req;
> -    uint8_t  buffer[2048];
> +    uint8_t  buffer[8192];
>  };
>  
>  struct USBAutoFilter {
> -- 
> 1.6.2.5
> 
>
Christian Krause - Jan. 25, 2010, 10:51 p.m.
Hello Michael,

On 01/24/2010 10:20 PM, Michael S. Tsirkin wrote:
> On Sun, Jan 24, 2010 at 05:34:52PM +0100, Christian Krause wrote:
>> Resend. The patch was already sent to the list on 2009-12-11. It would
>> be great if it could be reviewed and applied. Thank you very much
>> in advance.
>>
>> The WLAN USB stick ZyXEL NWD271N (0586:3417) uses very large
>> usb control transfers of more than 2048 bytes which won't fit
>> into the buffer of the ctrl_struct. This results in an error message
>> "husb: ctrl buffer too small" and a non-working device.
>> Increasing the buffer size to 8192 seems to be a safe choice.
>>
>> Signed-off-by: Christian Krause <chkr@plauener.de>
> 
> Are there any drawbacks to make\ing the buffer larger?

I've roughly looked into the source code of usb-linux.c and I don't see
an issue if the buffer would be larger:

- if the buffer is used to get data via USBDEVFS_SUBMITURB from the
kernel then the check:

    if (buffer_len > sizeof(s->ctrl.buffer)) {
        fprintf(stderr, "husb: ctrl buffer too small (%u > %zu)\n",
                buffer_len, sizeof(s->ctrl.buffer));
        return USB_RET_STALL;
    }

already ensures that the we only supply a buffer_len which matches the
buffer

- on the other hand, when we copy data out of the buffer into the data
structures from the HC, then there is also a check that we only copy as
much data as the HC requests:

            if (len > p->len)
                len = p->len;
            memcpy(p->data, s->ctrl.buffer + s->ctrl.offset, len);


> If no, let's just make it 64K? IIUC that's a maximum
> length for control transfers as length is a 16 bit field.

I think that's OK since it looks like that the increase of memory would
only be 64k per usb host device.


Best regards,
Christian


>> ---
>>  usb-linux.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/usb-linux.c b/usb-linux.c
>> index 285ac22..d205bd3 100644
>> --- a/usb-linux.c
>> +++ b/usb-linux.c
>> @@ -113,7 +113,7 @@ struct ctrl_struct {
>>      uint16_t offset;
>>      uint8_t  state;
>>      struct   usb_ctrlrequest req;
>> -    uint8_t  buffer[2048];
>> +    uint8_t  buffer[8192];
>>  };
>>  
>>  struct USBAutoFilter {
>> -- 
>> 1.6.2.5
>>
>>
>
Aurelien Jarno - Feb. 6, 2010, 4:15 p.m.
On Sun, Jan 24, 2010 at 05:34:52PM +0100, Christian Krause wrote:
> Resend. The patch was already sent to the list on 2009-12-11. It would
> be great if it could be reviewed and applied. Thank you very much
> in advance.
> 
> The WLAN USB stick ZyXEL NWD271N (0586:3417) uses very large
> usb control transfers of more than 2048 bytes which won't fit
> into the buffer of the ctrl_struct. This results in an error message
> "husb: ctrl buffer too small" and a non-working device.
> Increasing the buffer size to 8192 seems to be a safe choice.

Thanks, applied.

Patch

diff --git a/usb-linux.c b/usb-linux.c
index 285ac22..d205bd3 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -113,7 +113,7 @@  struct ctrl_struct {
     uint16_t offset;
     uint8_t  state;
     struct   usb_ctrlrequest req;
-    uint8_t  buffer[2048];
+    uint8_t  buffer[8192];
 };
 
 struct USBAutoFilter {