Re: Re: [PATCH] usb: allow max 8192 bytes for desc

[Daniel P . Berrangé]

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 :|