[SLOF] [PATCH] libusb: Fix compiler warnings with GCC 9 in the USB code
Alexey Kardashevskiy
aik at ozlabs.ru
Tue Sep 24 19:03:07 AEST 2019
On 20/09/2019 19:06, Thomas Huth wrote:
> GCC 9 emits a lot of compiler warnings in the USB code like this:
>
> [...]
> usb-ohci.c: In function ‘ohci_get_last_frame’:
> usb-ohci.c:972:20: warning: taking address of packed member of ‘struct ohci_regs’
> may result in an unaligned pointer value [-Waddress-of-packed-member]
> 972 | return read_reg32(®s->fm_num);
> | ^~~~~~~~~~~~~
> [CC] usb-ehci.o
> usb-ehci.c: In function ‘ehci_hub_check_ports’:
> usb-ehci.c:63:25: warning: taking address of packed member of ‘struct ehci_cap_regs’
> may result in an unaligned pointer value [-Waddress-of-packed-member]
> 63 | num_ports = read_reg32(&ehcd->cap_regs->hcsparams) & HCS_NPORTS_MASK;
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
> usb-ehci.c:66:23: warning: taking address of packed member of ‘struct ehci_op_regs’
> may result in an unaligned pointer value [-Waddress-of-packed-member]
> 66 | portsc = read_reg32(&ehcd->op_regs->portsc[i]);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> [...]
> usb-xhci.c:227:20: warning: taking address of packed member of ‘struct xhci_op_regs’
> may result in an unaligned pointer value [-Waddress-of-packed-member]
> 227 | xhci_wait_for_cnr(&op->usbsts);
> | ^~~~~~~~~~~
> usb-xhci.c: In function ‘xhci_poll_event’:
> usb-xhci.c:309:14: warning: taking address of packed member of ‘struct xhci_int_regs’
> may result in an unaligned pointer value [-Waddress-of-packed-member]
> 309 | write_reg64(&xhcd->run_regs->irs[0].erdp, val);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [...]
>
> Most of the structures are naturally aligned, so we can simply drop the
> "packed" attribute and use a _Static_assert() instead to make sure that
> there is no padding here.
>
> The only struct that needed some more attention is struct ehci_cap_regs.
> The portroute field is not aligned to a natural boundary here. But
> looking at the EHCI spec, it seems like this field is not meant as
> a full 64-bit value anyway, but rather an array of nibbles, so switch
> it to an array instead, then we can also drop the "packed" attribute
> here.
This one is harder to fix properly (if I knew what "properly" means
here). I think that for this one the right thing seems to be disabling
-Waddress-of-packed-member warning whatsoever. May be add an assert for
hcidev->base alignment.
Adding asserts as below will silence gcc but won't protect us against
(very unlikely to happen but nevertheless) unaligned pointers.
>
> Signed-off-by: Thomas Huth <thuth at redhat.com>
> ---
> lib/libusb/usb-ehci.c | 6 +++++-
> lib/libusb/usb-ehci.h | 8 +++++---
> lib/libusb/usb-ohci.h | 3 ++-
> lib/libusb/usb-xhci.h | 25 ++++++++++++++++++-------
> 4 files changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/lib/libusb/usb-ehci.c b/lib/libusb/usb-ehci.c
> index 60af9e1..299e9da 100644
> --- a/lib/libusb/usb-ehci.c
> +++ b/lib/libusb/usb-ehci.c
> @@ -38,7 +38,11 @@ static void dump_ehci_regs(struct ehci_hcd *ehcd)
> dprintf("\n - HCIVERSION %04X", read_reg16(&cap_regs->hciversion));
> dprintf("\n - HCSPARAMS %08X", read_reg32(&cap_regs->hcsparams));
> dprintf("\n - HCCPARAMS %08X", read_reg32(&cap_regs->hccparams));
> - dprintf("\n - HCSP_PORTROUTE %016llX", read_reg64(&cap_regs->portroute));
> + dprintf("\n - HCSP_PORTROUTE %02X %02X %02X %02X %02X %02X %02X %02X",
> + read_reg8(&cap_regs->portroute[0]), read_reg8(&cap_regs->portroute[1]),
> + read_reg8(&cap_regs->portroute[2]), read_reg8(&cap_regs->portroute[3]),
> + read_reg8(&cap_regs->portroute[4]), read_reg8(&cap_regs->portroute[5]),
> + read_reg8(&cap_regs->portroute[6]), read_reg8(&cap_regs->portroute[7]));
> dprintf("\n");
>
> dprintf("\n - USBCMD %08X", read_reg32(&op_regs->usbcmd));
> diff --git a/lib/libusb/usb-ehci.h b/lib/libusb/usb-ehci.h
> index 2955a9c..a834e95 100644
> --- a/lib/libusb/usb-ehci.h
> +++ b/lib/libusb/usb-ehci.h
> @@ -28,8 +28,9 @@ struct ehci_cap_regs {
> uint16_t hciversion;
> uint32_t hcsparams;
> uint32_t hccparams;
> - uint64_t portroute;
> -} __attribute__ ((packed));
> + uint8_t portroute[8];
> +};
> +_Static_assert(sizeof(struct ehci_cap_regs) == 20, "padding in ehci_cap_regs");
>
> struct ehci_op_regs {
> uint32_t usbcmd;
> @@ -42,7 +43,8 @@ struct ehci_op_regs {
> uint32_t reserved[9];
> uint32_t configflag;
> uint32_t portsc[0];
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct ehci_op_regs) == 68, "padding in ehci_op_regs");
>
> struct ehci_framelist {
> uint32_t fl_ptr[FL_SIZE];
> diff --git a/lib/libusb/usb-ohci.h b/lib/libusb/usb-ohci.h
> index f4535fd..1f38047 100644
> --- a/lib/libusb/usb-ohci.h
> +++ b/lib/libusb/usb-ohci.h
> @@ -47,7 +47,8 @@ struct ohci_regs {
> uint32_t rh_desc_b;
> uint32_t rh_status;
> uint32_t rh_ps[5];
> -} __attribute__((packed));
> +};
> +_Static_assert(sizeof(struct ohci_regs) == 104, "padding in ohci_regs");
>
> #define EDA_FADDR(x) ((x & 0x7F))
> #define EDA_EP(x) ((x & 0x0F) << 7)
> diff --git a/lib/libusb/usb-xhci.h b/lib/libusb/usb-xhci.h
> index 02b382b..07c3957 100644
> --- a/lib/libusb/usb-xhci.h
> +++ b/lib/libusb/usb-xhci.h
> @@ -37,7 +37,8 @@ struct xhci_cap_regs {
> #define XHCI_HCCPARAMS_XECP(x) ((x & 0xFFFF0000) >> 16)
> uint32_t dboff;
> uint32_t rtsoff;
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_cap_regs) == 28, "padding in xhci_cap_regs");
>
> /* USB 3.0: Section 7 and 7.2 */
> #define XHCI_XECP_CAP_ID(x) ((x & 0xF))
> @@ -90,7 +91,8 @@ struct xhci_port_regs {
> uint32_t portpmsc;
> uint32_t portli;
> uint32_t reserved;
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_port_regs) == 16, "padding in xhci_port_regs");
>
> struct port_state {
> bool PP;
> @@ -132,7 +134,10 @@ struct xhci_op_regs {
> /* USB Port register set */
> #define XHCI_PORT_MAX 256
> struct xhci_port_regs prs[XHCI_PORT_MAX];
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_op_regs) == 0x400 +
> + XHCI_PORT_MAX * sizeof(struct xhci_port_regs),
> + "padding in xhci_op_regs");
>
> /*
> * 5.5.2 Interrupter Register Set
> @@ -148,7 +153,8 @@ struct xhci_int_regs {
> #define XHCI_ERST_ADDR_MASK (~(0x3FUL))
> uint64_t erdp;
> #define XHCI_ERDP_MASK (~(0xFUL))
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_int_regs) == 32, "padding in xhci_int_regs");
>
> /* 5.5 Host Controller Runtime Registers */
> struct xhci_run_regs {
> @@ -156,12 +162,16 @@ struct xhci_run_regs {
> uint8_t reserved[28];
> #define XHCI_IRS_MAX 1024
> struct xhci_int_regs irs[XHCI_IRS_MAX];
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_run_regs) == 32 +
> + XHCI_IRS_MAX * sizeof(struct xhci_int_regs),
> + "padding in xhci_run_regs");
>
> /* 5.6 Doorbell Registers*/
> struct xhci_db_regs {
> uint32_t db[256];
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_db_regs) == 1024, "padding in xhci_db_regs");
>
> #define COMP_SUCCESS 1
>
> @@ -259,7 +269,8 @@ struct xhci_erst_entry {
> uint64_t addr;
> uint32_t size;
> uint32_t reserved;
> -} __attribute__ ((packed));
> +};
> +_Static_assert(sizeof(struct xhci_erst_entry) == 16, "padding in xhci_erst_entry");
>
> struct xhci_erst {
> struct xhci_erst_entry *entries;
>
--
Alexey
More information about the SLOF
mailing list