| Message ID | 20191112212643.915-1-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,1/2] usb: composite: fix possible alignment issues | expand |
On 11/12/19 10:26 PM, Simon Goldschmidt wrote: > Since upgrading to gcc9, warnings are issued: > "taking address of packed member of ‘...’ may result in an unaligned > pointer value" > > Fix this by converting two functions to use unaligned access since packed > structures may be on an unaligned address, depending on USB hardware. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > > drivers/usb/gadget/composite.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index 618a7d5016..cfc9512caa 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -12,8 +12,16 @@ > > #define USB_BUFSIZ 4096 > > +/* Helper type for accessing packed u16 pointers */ > +typedef struct { __le16 val; } __packed __le16_packed; > + > static struct usb_composite_driver *composite; > > +static inline void le16_add_cpu_packed(__le16_packed *var, u16 val) > +{ > + var->val = cpu_to_le16(le16_to_cpu(var->val) + val); > +} > + > /** > * usb_add_function() - add a function to a configuration > * @config: the configuration > @@ -480,20 +488,20 @@ done: > * the host side. > */ > > -static void collect_langs(struct usb_gadget_strings **sp, __le16 *buf) > +static void collect_langs(struct usb_gadget_strings **sp, void *buf) With this patch and GCC 9.2.1 I get: In file included from drivers/usb/gadget/g_dnl.c:24: drivers/usb/gadget/composite.c: In function ‘collect_langs’: drivers/usb/gadget/composite.c:500:41: error: dereferencing ‘void *’ pointer [-Werror] 500 | for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { | ^ drivers/usb/gadget/composite.c:500:35: error: comparison of distinct pointer types lacks a cast [-Werror] 500 | for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { void does not have defined size and so cannot be subscripted. Best regards Heinrich > { > const struct usb_gadget_strings *s; > u16 language; > - __le16 *tmp; > + __le16_packed *tmp; > > while (*sp) { > s = *sp; > language = cpu_to_le16(s->language); > - for (tmp = buf; *tmp && tmp < &buf[126]; tmp++) { > - if (*tmp == language) > + for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { > + if (tmp->val == language) > goto repeat; > } > - *tmp++ = language; > + tmp->val = language; > repeat: > sp++; > } > @@ -705,7 +713,8 @@ static int bos_desc(struct usb_composite_dev *cdev) > */ > usb_ext = cdev->req->buf + le16_to_cpu(bos->wTotalLength); > bos->bNumDeviceCaps++; > - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_EXT_CAP_SIZE); > + le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength, > + USB_DT_USB_EXT_CAP_SIZE); > usb_ext->bLength = USB_DT_USB_EXT_CAP_SIZE; > usb_ext->bDescriptorType = USB_DT_DEVICE_CAPABILITY; > usb_ext->bDevCapabilityType = USB_CAP_TYPE_EXT; > @@ -721,7 +730,8 @@ static int bos_desc(struct usb_composite_dev *cdev) > > ss_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); > bos->bNumDeviceCaps++; > - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SS_CAP_SIZE); > + le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength, > + USB_DT_USB_SS_CAP_SIZE); > ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE; > ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY; > ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE; >
Am 21.11.2019 um 20:36 schrieb Heinrich Schuchardt: > On 11/12/19 10:26 PM, Simon Goldschmidt wrote: >> Since upgrading to gcc9, warnings are issued: >> "taking address of packed member of ‘...’ may result in an unaligned >> pointer value" >> >> Fix this by converting two functions to use unaligned access since packed >> structures may be on an unaligned address, depending on USB hardware. >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> --- >> >> drivers/usb/gadget/composite.c | 24 +++++++++++++++++------- >> 1 file changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >> index 618a7d5016..cfc9512caa 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -12,8 +12,16 @@ >> >> #define USB_BUFSIZ 4096 >> >> +/* Helper type for accessing packed u16 pointers */ >> +typedef struct { __le16 val; } __packed __le16_packed; >> + >> static struct usb_composite_driver *composite; >> >> +static inline void le16_add_cpu_packed(__le16_packed *var, u16 val) >> +{ >> + var->val = cpu_to_le16(le16_to_cpu(var->val) + val); >> +} >> + >> /** >> * usb_add_function() - add a function to a configuration >> * @config: the configuration >> @@ -480,20 +488,20 @@ done: >> * the host side. >> */ >> >> -static void collect_langs(struct usb_gadget_strings **sp, __le16 *buf) >> +static void collect_langs(struct usb_gadget_strings **sp, void *buf) > > With this patch and GCC 9.2.1 I get: > > In file included from drivers/usb/gadget/g_dnl.c:24: > drivers/usb/gadget/composite.c: In function ‘collect_langs’: > drivers/usb/gadget/composite.c:500:41: error: dereferencing ‘void *’ > pointer [-Werror] > 500 | for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { > | ^ > drivers/usb/gadget/composite.c:500:35: error: comparison of distinct > pointer types lacks a cast [-Werror] > 500 | for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { > > void does not have defined size and so cannot be subscripted. Right, thanks for testing. I hate it that we don't have -Werror enabled, as this just slipped through with all the output on the screen. I'll send v2 fixing that. Regards, Simon > > Best regards > > Heinrich > >> { >> const struct usb_gadget_strings *s; >> u16 language; >> - __le16 *tmp; >> + __le16_packed *tmp; >> >> while (*sp) { >> s = *sp; >> language = cpu_to_le16(s->language); >> - for (tmp = buf; *tmp && tmp < &buf[126]; tmp++) { >> - if (*tmp == language) >> + for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { >> + if (tmp->val == language) >> goto repeat; >> } >> - *tmp++ = language; >> + tmp->val = language; >> repeat: >> sp++; >> } >> @@ -705,7 +713,8 @@ static int bos_desc(struct usb_composite_dev *cdev) >> */ >> usb_ext = cdev->req->buf + le16_to_cpu(bos->wTotalLength); >> bos->bNumDeviceCaps++; >> - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_EXT_CAP_SIZE); >> + le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength, >> + USB_DT_USB_EXT_CAP_SIZE); >> usb_ext->bLength = USB_DT_USB_EXT_CAP_SIZE; >> usb_ext->bDescriptorType = USB_DT_DEVICE_CAPABILITY; >> usb_ext->bDevCapabilityType = USB_CAP_TYPE_EXT; >> @@ -721,7 +730,8 @@ static int bos_desc(struct usb_composite_dev *cdev) >> >> ss_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); >> bos->bNumDeviceCaps++; >> - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SS_CAP_SIZE); >> + le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength, >> + USB_DT_USB_SS_CAP_SIZE); >> ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE; >> ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY; >> ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE; >> >
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 618a7d5016..cfc9512caa 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -12,8 +12,16 @@ #define USB_BUFSIZ 4096 +/* Helper type for accessing packed u16 pointers */ +typedef struct { __le16 val; } __packed __le16_packed; + static struct usb_composite_driver *composite; +static inline void le16_add_cpu_packed(__le16_packed *var, u16 val) +{ + var->val = cpu_to_le16(le16_to_cpu(var->val) + val); +} + /** * usb_add_function() - add a function to a configuration * @config: the configuration @@ -480,20 +488,20 @@ done: * the host side. */ -static void collect_langs(struct usb_gadget_strings **sp, __le16 *buf) +static void collect_langs(struct usb_gadget_strings **sp, void *buf) { const struct usb_gadget_strings *s; u16 language; - __le16 *tmp; + __le16_packed *tmp; while (*sp) { s = *sp; language = cpu_to_le16(s->language); - for (tmp = buf; *tmp && tmp < &buf[126]; tmp++) { - if (*tmp == language) + for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) { + if (tmp->val == language) goto repeat; } - *tmp++ = language; + tmp->val = language; repeat: sp++; } @@ -705,7 +713,8 @@ static int bos_desc(struct usb_composite_dev *cdev) */ usb_ext = cdev->req->buf + le16_to_cpu(bos->wTotalLength); bos->bNumDeviceCaps++; - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_EXT_CAP_SIZE); + le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength, + USB_DT_USB_EXT_CAP_SIZE); usb_ext->bLength = USB_DT_USB_EXT_CAP_SIZE; usb_ext->bDescriptorType = USB_DT_DEVICE_CAPABILITY; usb_ext->bDevCapabilityType = USB_CAP_TYPE_EXT; @@ -721,7 +730,8 @@ static int bos_desc(struct usb_composite_dev *cdev) ss_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); bos->bNumDeviceCaps++; - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SS_CAP_SIZE); + le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength, + USB_DT_USB_SS_CAP_SIZE); ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE; ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY; ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE;
Since upgrading to gcc9, warnings are issued: "taking address of packed member of ‘...’ may result in an unaligned pointer value" Fix this by converting two functions to use unaligned access since packed structures may be on an unaligned address, depending on USB hardware. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- drivers/usb/gadget/composite.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)