1. Patchew
  2. QEMU
  3. View series

[PATCH] usb: allow max 8192 bytes for desc

zhenwei pi posted 1 patch 2 years, 3 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220111104918.896841-1-pizhenwei@bytedance.com
There is a newer version of this series
hw/usb/desc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] usb: allow max 8192 bytes for desc
Posted by zhenwei pi 2 years, 3 months ago 
A device of USB video class usually uses larger desc structure, so
use larger buffer to avoid failure. (dev-video.c is ready)

Allocating memory dynamically by g_malloc of the orignal version of
this change, Philippe suggested just using the stack. Test the two
versions of qemu binary, the size of stack gets no change.

CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 hw/usb/desc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/desc.c b/hw/usb/desc.c
index 8b6eaea407..57d2aedba1 100644
--- a/hw/usb/desc.c
+++ b/hw/usb/desc.c
@@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
     bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
     const USBDesc *desc = usb_device_get_usb_desc(dev);
     const USBDescDevice *other_dev;
-    uint8_t buf[256];
+    uint8_t buf[8192];
     uint8_t type = value >> 8;
     uint8_t index = value & 0xff;
     int flags, ret = -1;
-- 
2.25.1
Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by Philippe Mathieu-Daudé 2 years, 3 months ago 
On 1/11/22 11:49, zhenwei pi wrote:
> A device of USB video class usually uses larger desc structure, so
> use larger buffer to avoid failure. (dev-video.c is ready)
> 
> Allocating memory dynamically by g_malloc of the orignal version of
> this change, Philippe suggested just using the stack. Test the two
> versions of qemu binary, the size of stack gets no change.
> 
> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  hw/usb/desc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> index 8b6eaea407..57d2aedba1 100644
> --- a/hw/usb/desc.c
> +++ b/hw/usb/desc.c
> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>      bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>      const USBDesc *desc = usb_device_get_usb_desc(dev);
>      const USBDescDevice *other_dev;
> -    uint8_t buf[256];
> +    uint8_t buf[8192];
>      uint8_t type = value >> 8;
>      uint8_t index = value & 0xff;
>      int flags, ret = -1;

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by Peter Maydell 2 years, 3 months ago 
On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
>
> A device of USB video class usually uses larger desc structure, so
> use larger buffer to avoid failure. (dev-video.c is ready)
>
> Allocating memory dynamically by g_malloc of the orignal version of
> this change, Philippe suggested just using the stack. Test the two
> versions of qemu binary, the size of stack gets no change.
>
> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  hw/usb/desc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> index 8b6eaea407..57d2aedba1 100644
> --- a/hw/usb/desc.c
> +++ b/hw/usb/desc.c
> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>      bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>      const USBDesc *desc = usb_device_get_usb_desc(dev);
>      const USBDescDevice *other_dev;
> -    uint8_t buf[256];
> +    uint8_t buf[8192];
>      uint8_t type = value >> 8;
>      uint8_t index = value & 0xff;
>      int flags, ret = -1;

I think 8K is too large to be allocating as an array on
the stack, so if we need this buffer to be larger we should
switch to some other allocation strategy for it.

thanks
-- PMM
Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by Daniel P. Berrangé 2 years, 3 months ago 
On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
> >
> > A device of USB video class usually uses larger desc structure, so
> > use larger buffer to avoid failure. (dev-video.c is ready)
> >
> > Allocating memory dynamically by g_malloc of the orignal version of
> > this change, Philippe suggested just using the stack. Test the two
> > versions of qemu binary, the size of stack gets no change.
> >
> > CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > ---
> >  hw/usb/desc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> > index 8b6eaea407..57d2aedba1 100644
> > --- a/hw/usb/desc.c
> > +++ b/hw/usb/desc.c
> > @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
> >      bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
> >      const USBDesc *desc = usb_device_get_usb_desc(dev);
> >      const USBDescDevice *other_dev;
> > -    uint8_t buf[256];
> > +    uint8_t buf[8192];
> >      uint8_t type = value >> 8;
> >      uint8_t index = value & 0xff;
> >      int flags, ret = -1;
> 
> I think 8K is too large to be allocating as an array on
> the stack, so if we need this buffer to be larger we should
> switch to some other allocation strategy for it.

IIUC, querying USB device descriptors is not a hot path, so using
heap allocation feels sufficient.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by zhenwei pi 2 years, 3 months ago 

On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
>> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
>>>
>>> A device of USB video class usually uses larger desc structure, so
>>> use larger buffer to avoid failure. (dev-video.c is ready)
>>>
>>> Allocating memory dynamically by g_malloc of the orignal version of
>>> this change, Philippe suggested just using the stack. Test the two
>>> versions of qemu binary, the size of stack gets no change.
>>>
>>> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>> ---
>>>   hw/usb/desc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>>> index 8b6eaea407..57d2aedba1 100644
>>> --- a/hw/usb/desc.c
>>> +++ b/hw/usb/desc.c
>>> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>>>       bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>>>       const USBDesc *desc = usb_device_get_usb_desc(dev);
>>>       const USBDescDevice *other_dev;
>>> -    uint8_t buf[256];
>>> +    uint8_t buf[8192];
>>>       uint8_t type = value >> 8;
>>>       uint8_t index = value & 0xff;
>>>       int flags, ret = -1;
>>
>> I think 8K is too large to be allocating as an array on
>> the stack, so if we need this buffer to be larger we should
>> switch to some other allocation strategy for it.
> 
> IIUC, querying USB device descriptors is not a hot path, so using
> heap allocation feels sufficient.
> 
Yes, I tested this a lot, and found that it's an unlikely code path:
1, during guest startup, guest tries to probe device.
2, run 'lsusb' command in guest(or other similar commands).

The original patch and context link:
https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/
> 
> Regards,
> Daniel
> 

-- 
zhenwei pi
Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by Philippe Mathieu-Daudé 2 years, 3 months ago 
On 1/11/22 13:27, zhenwei pi wrote:
> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
>> On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
>>> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com>
>>> wrote:
>>>>
>>>> A device of USB video class usually uses larger desc structure, so
>>>> use larger buffer to avoid failure. (dev-video.c is ready)
>>>>
>>>> Allocating memory dynamically by g_malloc of the orignal version of
>>>> this change, Philippe suggested just using the stack. Test the two
>>>> versions of qemu binary, the size of stack gets no change.
>>>>
>>>> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>>> ---
>>>>   hw/usb/desc.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>>>> index 8b6eaea407..57d2aedba1 100644
>>>> --- a/hw/usb/desc.c
>>>> +++ b/hw/usb/desc.c
>>>> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev,
>>>> USBPacket *p,
>>>>       bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>>>>       const USBDesc *desc = usb_device_get_usb_desc(dev);
>>>>       const USBDescDevice *other_dev;
>>>> -    uint8_t buf[256];
>>>> +    uint8_t buf[8192];
>>>>       uint8_t type = value >> 8;
>>>>       uint8_t index = value & 0xff;
>>>>       int flags, ret = -1;
>>>
>>> I think 8K is too large to be allocating as an array on
>>> the stack, so if we need this buffer to be larger we should
>>> switch to some other allocation strategy for it.
>>
>> IIUC, querying USB device descriptors is not a hot path, so using
>> heap allocation feels sufficient.
>>
> Yes, I tested this a lot, and found that it's an unlikely code path:
> 1, during guest startup, guest tries to probe device.
> 2, run 'lsusb' command in guest(or other similar commands).

Sorry, I thought this was a hot path without looking at the code.

> The original patch and context link:
> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/
> 
>>
>> Regards,
>> Daniel
>>
>
Re: Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by Daniel P. Berrangé 2 years, 3 months ago 
On Tue, Jan 11, 2022 at 08:27:35PM +0800, zhenwei pi wrote:
> 
> 
> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
> > On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
> > > On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
> > > > 
> > > > A device of USB video class usually uses larger desc structure, so
> > > > use larger buffer to avoid failure. (dev-video.c is ready)
> > > > 
> > > > Allocating memory dynamically by g_malloc of the orignal version of
> > > > this change, Philippe suggested just using the stack. Test the two
> > > > versions of qemu binary, the size of stack gets no change.
> > > > 
> > > > CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > > > ---
> > > >   hw/usb/desc.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> > > > index 8b6eaea407..57d2aedba1 100644
> > > > --- a/hw/usb/desc.c
> > > > +++ b/hw/usb/desc.c
> > > > @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
> > > >       bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
> > > >       const USBDesc *desc = usb_device_get_usb_desc(dev);
> > > >       const USBDescDevice *other_dev;
> > > > -    uint8_t buf[256];
> > > > +    uint8_t buf[8192];
> > > >       uint8_t type = value >> 8;
> > > >       uint8_t index = value & 0xff;
> > > >       int flags, ret = -1;
> > > 
> > > I think 8K is too large to be allocating as an array on
> > > the stack, so if we need this buffer to be larger we should
> > > switch to some other allocation strategy for it.
> > 
> > IIUC, querying USB device descriptors is not a hot path, so using
> > heap allocation feels sufficient.
> > 
> Yes, I tested this a lot, and found that it's an unlikely code path:
> 1, during guest startup, guest tries to probe device.
> 2, run 'lsusb' command in guest(or other similar commands).
> 
> The original patch and context link:
> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/

Yes, the orignal patch is better I think.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: Re: Re: [PATCH] usb: allow max 8192 bytes for desc
Posted by zhenwei pi 2 years, 3 months ago 
On 1/11/22 8:38 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 08:27:35PM +0800, zhenwei pi wrote:
>>
>>
>> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
>>> On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
>>>> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
>>>>>
>>>>> A device of USB video class usually uses larger desc structure, so
>>>>> use larger buffer to avoid failure. (dev-video.c is ready)
>>>>>
>>>>> Allocating memory dynamically by g_malloc of the orignal version of
>>>>> this change, Philippe suggested just using the stack. Test the two
>>>>> versions of qemu binary, the size of stack gets no change.
>>>>>
>>>>> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>>>> ---
>>>>>    hw/usb/desc.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>>>>> index 8b6eaea407..57d2aedba1 100644
>>>>> --- a/hw/usb/desc.c
>>>>> +++ b/hw/usb/desc.c
>>>>> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>>>>>        bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>>>>>        const USBDesc *desc = usb_device_get_usb_desc(dev);
>>>>>        const USBDescDevice *other_dev;
>>>>> -    uint8_t buf[256];
>>>>> +    uint8_t buf[8192];
>>>>>        uint8_t type = value >> 8;
>>>>>        uint8_t index = value & 0xff;
>>>>>        int flags, ret = -1;
>>>>
>>>> I think 8K is too large to be allocating as an array on
>>>> the stack, so if we need this buffer to be larger we should
>>>> switch to some other allocation strategy for it.
>>>
>>> IIUC, querying USB device descriptors is not a hot path, so using
>>> heap allocation feels sufficient.
>>>
>> Yes, I tested this a lot, and found that it's an unlikely code path:
>> 1, during guest startup, guest tries to probe device.
>> 2, run 'lsusb' command in guest(or other similar commands).
>>
>> The original patch and context link:
>> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/
> 
> Yes, the orignal patch is better I think.
> 
> 
> 
> Regards,
> Daniel
> 
By the way, could you please review the v2 version of "camera subsystem"?

https://patchwork.kernel.org/project/qemu-devel/cover/20220106085304.795010-1-pizhenwei@bytedance.com/

-- 
zhenwei pi

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.