Message ID | 20211227142734.691900-5-pizhenwei@bytedance.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Introduce camera subsystem and USB video device | expand |
On 27/12/21 15:27, zhenwei pi wrote: > A device of USB video class usually uses larger desc structure, so > use larger buffer to avoid failure. > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> > --- > hw/usb/desc.c | 15 ++++++++------- > hw/usb/desc.h | 1 + > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/hw/usb/desc.c b/hw/usb/desc.c > index 8b6eaea407..7f6cc2f99b 100644 > --- a/hw/usb/desc.c > +++ b/hw/usb/desc.c > @@ -632,7 +632,8 @@ 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]; > + size_t buflen = USB_DESC_MAX_LEN; > + g_autofree uint8_t *buf = g_malloc(buflen); Do we want to have a per-device desc_size (in USBDevice, default to 256, video devices set it to 8K)? How "hot" is this path? Could we keep 8K on the stack? > diff --git a/hw/usb/desc.h b/hw/usb/desc.h > index 3ac604ecfa..35babdeff6 100644 > --- a/hw/usb/desc.h > +++ b/hw/usb/desc.h > @@ -199,6 +199,7 @@ struct USBDesc { > const USBDescMSOS *msos; > }; > > +#define USB_DESC_MAX_LEN 8192 > #define USB_DESC_FLAG_SUPER (1 << 1) > > /* little helpers */
On 1/4/22 11:22 PM, Philippe Mathieu-Daudé wrote: > On 27/12/21 15:27, zhenwei pi wrote: >> A device of USB video class usually uses larger desc structure, so >> use larger buffer to avoid failure. >> >> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> >> --- >> hw/usb/desc.c | 15 ++++++++------- >> hw/usb/desc.h | 1 + >> 2 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/hw/usb/desc.c b/hw/usb/desc.c >> index 8b6eaea407..7f6cc2f99b 100644 >> --- a/hw/usb/desc.c >> +++ b/hw/usb/desc.c >> @@ -632,7 +632,8 @@ 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]; >> + size_t buflen = USB_DESC_MAX_LEN; >> + g_autofree uint8_t *buf = g_malloc(buflen); > > Do we want to have a per-device desc_size (in USBDevice, default to > 256, video devices set it to 8K)? > > How "hot" is this path? Could we keep 8K on the stack? > It's an unlikely code path: 1, During guest startup, guest tries to probe device. 2, run 'lsusb' command in guest Keeping 8K on the stack also seems OK. >> diff --git a/hw/usb/desc.h b/hw/usb/desc.h >> index 3ac604ecfa..35babdeff6 100644 >> --- a/hw/usb/desc.h >> +++ b/hw/usb/desc.h >> @@ -199,6 +199,7 @@ struct USBDesc { >> const USBDescMSOS *msos; >> }; >> +#define USB_DESC_MAX_LEN 8192 >> #define USB_DESC_FLAG_SUPER (1 << 1) >> /* little helpers */ >
On Mon, Dec 27, 2021 at 10:27:33PM +0800, zhenwei pi wrote: > A device of USB video class usually uses larger desc structure, so > use larger buffer to avoid failure. > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> > --- > hw/usb/desc.c | 15 ++++++++------- > hw/usb/desc.h | 1 + > 2 files changed, 9 insertions(+), 7 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On 1/5/22 08:25, zhenwei pi wrote: > > On 1/4/22 11:22 PM, Philippe Mathieu-Daudé wrote: >> On 27/12/21 15:27, zhenwei pi wrote: >>> A device of USB video class usually uses larger desc structure, so >>> use larger buffer to avoid failure. >>> >>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> >>> --- >>> hw/usb/desc.c | 15 ++++++++------- >>> hw/usb/desc.h | 1 + >>> 2 files changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c >>> index 8b6eaea407..7f6cc2f99b 100644 >>> --- a/hw/usb/desc.c >>> +++ b/hw/usb/desc.c >>> @@ -632,7 +632,8 @@ 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]; >>> + size_t buflen = USB_DESC_MAX_LEN; >>> + g_autofree uint8_t *buf = g_malloc(buflen); >> >> Do we want to have a per-device desc_size (in USBDevice, default to >> 256, video devices set it to 8K)? >> >> How "hot" is this path? Could we keep 8K on the stack? >> > It's an unlikely code path: > 1, During guest startup, guest tries to probe device. > 2, run 'lsusb' command in guest If you have to respin, do you mind adding this 3 lines in the description? Anyhow: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/usb/desc.c b/hw/usb/desc.c index 8b6eaea407..7f6cc2f99b 100644 --- a/hw/usb/desc.c +++ b/hw/usb/desc.c @@ -632,7 +632,8 @@ 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]; + size_t buflen = USB_DESC_MAX_LEN; + g_autofree uint8_t *buf = g_malloc(buflen); uint8_t type = value >> 8; uint8_t index = value & 0xff; int flags, ret = -1; @@ -650,36 +651,36 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p, switch(type) { case USB_DT_DEVICE: - ret = usb_desc_device(&desc->id, dev->device, msos, buf, sizeof(buf)); + ret = usb_desc_device(&desc->id, dev->device, msos, buf, buflen); trace_usb_desc_device(dev->addr, len, ret); break; case USB_DT_CONFIG: if (index < dev->device->bNumConfigurations) { ret = usb_desc_config(dev->device->confs + index, flags, - buf, sizeof(buf)); + buf, buflen); } trace_usb_desc_config(dev->addr, index, len, ret); break; case USB_DT_STRING: - ret = usb_desc_string(dev, index, buf, sizeof(buf)); + ret = usb_desc_string(dev, index, buf, buflen); trace_usb_desc_string(dev->addr, index, len, ret); break; case USB_DT_DEVICE_QUALIFIER: if (other_dev != NULL) { - ret = usb_desc_device_qualifier(other_dev, buf, sizeof(buf)); + ret = usb_desc_device_qualifier(other_dev, buf, buflen); } trace_usb_desc_device_qualifier(dev->addr, len, ret); break; case USB_DT_OTHER_SPEED_CONFIG: if (other_dev != NULL && index < other_dev->bNumConfigurations) { ret = usb_desc_config(other_dev->confs + index, flags, - buf, sizeof(buf)); + buf, buflen); buf[0x01] = USB_DT_OTHER_SPEED_CONFIG; } trace_usb_desc_other_speed_config(dev->addr, index, len, ret); break; case USB_DT_BOS: - ret = usb_desc_bos(desc, buf, sizeof(buf)); + ret = usb_desc_bos(desc, buf, buflen); trace_usb_desc_bos(dev->addr, len, ret); break; diff --git a/hw/usb/desc.h b/hw/usb/desc.h index 3ac604ecfa..35babdeff6 100644 --- a/hw/usb/desc.h +++ b/hw/usb/desc.h @@ -199,6 +199,7 @@ struct USBDesc { const USBDescMSOS *msos; }; +#define USB_DESC_MAX_LEN 8192 #define USB_DESC_FLAG_SUPER (1 << 1) /* little helpers */
A device of USB video class usually uses larger desc structure, so use larger buffer to avoid failure. Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> --- hw/usb/desc.c | 15 ++++++++------- hw/usb/desc.h | 1 + 2 files changed, 9 insertions(+), 7 deletions(-)