| Message ID | 20200304221807.25212-4-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw,ui: Reduce QEMU .rodata/.bss footprint | expand |
> struct usb_device_id { > - int vendor_id; > - int product_id; > - int interface_class; > - int interface_subclass; > - int interface_protocol; > + int16_t vendor_id; > + int16_t product_id; > + int8_t interface_class; > + int8_t interface_subclass; > + int8_t interface_protocol; I guess we should better use the uint variants here ... cheers, Gerd
On 3/5/20 9:02 AM, Gerd Hoffmann wrote: >> struct usb_device_id { >> - int vendor_id; >> - int product_id; >> - int interface_class; >> - int interface_subclass; >> - int interface_protocol; >> + int16_t vendor_id; >> + int16_t product_id; >> + int8_t interface_class; >> + int8_t interface_subclass; >> + int8_t interface_protocol; > > I guess we should better use the uint variants here ... I tried it first but I got: CC hw/usb/quirks.o hw/usb/quirks.c:25:34: error: result of comparison of constant -1 with expression of type 'const uint16_t' (aka 'const unsigned short') is always true [-Werror,-Wtautological-constant-out-of-range-compare] for (i = 0; ids[i].vendor_id != -1; i++) { ~~~~~~~~~~~~~~~~ ^ ~~ hw/usb/quirks.c:28:37: error: result of comparison of constant -1 with expression of type 'const uint8_t' (aka 'const unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare] (ids[i].interface_class == -1 || ~~~~~~~~~~~~~~~~~~~~~~ ^ ~~ And went this less intrusive way. I'll respin with s/-1/UINT8_MAX/.
On Thu, Mar 5, 2020 at 11:41 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 3/5/20 9:02 AM, Gerd Hoffmann wrote: > >> struct usb_device_id { > >> - int vendor_id; > >> - int product_id; > >> - int interface_class; > >> - int interface_subclass; > >> - int interface_protocol; > >> + int16_t vendor_id; > >> + int16_t product_id; > >> + int8_t interface_class; > >> + int8_t interface_subclass; > >> + int8_t interface_protocol; > > > > I guess we should better use the uint variants here ... > > I tried it first but I got: > > CC hw/usb/quirks.o > hw/usb/quirks.c:25:34: error: result of comparison of constant -1 with > expression of type 'const uint16_t' (aka 'const unsigned short') is > always true [-Werror,-Wtautological-constant-out-of-range-compare] > for (i = 0; ids[i].vendor_id != -1; i++) { > ~~~~~~~~~~~~~~~~ ^ ~~ > hw/usb/quirks.c:28:37: error: result of comparison of constant -1 with > expression of type 'const uint8_t' (aka 'const unsigned char') is always > false [-Werror,-Wtautological-constant-out-of-range-compare] > (ids[i].interface_class == -1 || > ~~~~~~~~~~~~~~~~~~~~~~ ^ ~~ > > And went this less intrusive way. > > I'll respin with s/-1/UINT8_MAX/. Problem, now this entry is ignored (interface_class==-1): { USB_DEVICE_AND_INTERFACE_INFO(MICROCHIP_VID, MICROCHIP_USB_BOARD_PID, 0xff, 0xff, 0x00) },
> > And went this less intrusive way. > > > > I'll respin with s/-1/UINT8_MAX/. > > Problem, now this entry is ignored (interface_class==-1): Yep, "-1" is used as "not used" or "end-of-list" indicator, and it is outside the valid range to avoid that kind of clashes. You need some other way to express that if you want go with smaller types which don't allow values outside the valid range any more. Add a "flags" field for that maybe? cheers, Gerd
On 3/5/20 12:59 PM, Gerd Hoffmann wrote: >>> And went this less intrusive way. >>> >>> I'll respin with s/-1/UINT8_MAX/. >> >> Problem, now this entry is ignored (interface_class==-1): > > Yep, "-1" is used as "not used" or "end-of-list" indicator, and it is > outside the valid range to avoid that kind of clashes. You need some > other way to express that if you want go with smaller types which don't > allow values outside the valid range any more. Add a "flags" field for > that maybe? Oh I wasn't expecting 0xffff valid for idVendor / idProduct, neither 0xff for bInterfaceClass / bInterfaceSubClass / bInterfaceProtocol. Well maybe it doesn't worth the churn for 10KB of .rodata.
diff --git a/hw/usb/quirks.h b/hw/usb/quirks.h index 89480befd7..794d89a356 100644 --- a/hw/usb/quirks.h +++ b/hw/usb/quirks.h @@ -21,11 +21,11 @@ #include "quirks-pl2303-ids.h" struct usb_device_id { - int vendor_id; - int product_id; - int interface_class; - int interface_subclass; - int interface_protocol; + int16_t vendor_id; + int16_t product_id; + int8_t interface_class; + int8_t interface_subclass; + int8_t interface_protocol; }; #define USB_DEVICE(vendor, product) \
The USB descriptor sizes are specified as 16-bit for idVendor / idProduct, and 8-bit for bInterfaceClass / bInterfaceSubClass / bInterfaceProtocol. Doing so we reduce the usbredir_raw_serial_ids[] and usbredir_ftdi_serial_ids[] arrays from 16KiB to 6KiB (size reported on x86_64 host, building with --extra-cflags=-Os). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/usb/quirks.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)