| Message ID | 20191121211523.7219-1-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 616ebd8b9cb455c5949bd94c47283835eba1954a |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,v2,1/2] usb: composite: fix possible alignment issues | expand |
On Thu, 21 Nov 2019 22:15:22 +0100 Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> 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> > --- > > Changes in v2: > - fix compiler warning "dereferencing ‘void *'" > > drivers/usb/gadget/composite.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/composite.c > b/drivers/usb/gadget/composite.c index 618a7d5016..c98a444245 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,21 @@ 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; > + __le16_packed *end = (buf + 252); > > 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 < end; tmp++) { > + if (tmp->val == language) > goto repeat; > } > - *tmp++ = language; > + tmp->val = language; > repeat: > sp++; > } > @@ -705,7 +714,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 +731,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; Acked-by: Lukasz Majewski <lukma@denx.de> Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 11/21/19 10:15 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> Applied both, thanks.
On 11/22/19 1:25 AM, Marek Vasut wrote: > On 11/21/19 10:15 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> > > Applied both, thanks. > With these two patches applied to origin/master GCC 9.2.1 does not produce warnings anymore but for tbs2910_defconfig: u-boot.imx exceeds file size limit: limit: 0x5fc00 bytes actual: 0x60c00 bytes excess: 0x1000 bytes make: *** [Makefile:1159: u-boot.imx] Error 1 make: *** Deleting file 'u-boot.imx' So irrespective of my patches for the USB keyboard we need to address the size limit on TBS2910. Best regards Heinrich
On Fri, Nov 22, 2019 at 7:50 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11/22/19 1:25 AM, Marek Vasut wrote: > > On 11/21/19 10:15 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> > > > > Applied both, thanks. > > > > With these two patches applied to origin/master GCC 9.2.1 does not > produce warnings anymore but for tbs2910_defconfig: > > u-boot.imx exceeds file size limit: > limit: 0x5fc00 bytes > actual: 0x60c00 bytes > excess: 0x1000 bytes > make: *** [Makefile:1159: u-boot.imx] Error 1 > make: *** Deleting file 'u-boot.imx' > > So irrespective of my patches for the USB keyboard we need to address > the size limit on TBS2910. Is that due to my cleanup patches? Can you compare the size by compiling without them? That should work if you leave away the -Werror switch. Regards, Simon > > Best regards > > Heinrich
On 11/22/19 8:47 AM, Simon Goldschmidt wrote: > On Fri, Nov 22, 2019 at 7:50 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 11/22/19 1:25 AM, Marek Vasut wrote: >>> On 11/21/19 10:15 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> >>> >>> Applied both, thanks. >>> >> >> With these two patches applied to origin/master GCC 9.2.1 does not >> produce warnings anymore but for tbs2910_defconfig: >> >> u-boot.imx exceeds file size limit: >> limit: 0x5fc00 bytes >> actual: 0x60c00 bytes >> excess: 0x1000 bytes >> make: *** [Makefile:1159: u-boot.imx] Error 1 >> make: *** Deleting file 'u-boot.imx' >> >> So irrespective of my patches for the USB keyboard we need to address >> the size limit on TBS2910. > > Is that due to my cleanup patches? Can you compare the size by compiling > without them? That should work if you leave away the -Werror switch. > > Regards, > Simon GCC 9.2.1 without your patches but with -Wno-address-of-packed-member: u-boot.imx exceeds file size limit: limit: 0x5fc00 bytes actual: 0x60c00 bytes excess: 0x1000 bytes Best regards Heinrich > >> >> Best regards >> >> Heinrich >
On 11/22/19 12:58 PM, Heinrich Schuchardt wrote: > On 11/22/19 8:47 AM, Simon Goldschmidt wrote: >> On Fri, Nov 22, 2019 at 7:50 AM Heinrich Schuchardt >> <xypron.glpk@gmx.de> wrote: >>> >>> On 11/22/19 1:25 AM, Marek Vasut wrote: >>>> On 11/21/19 10:15 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> >>>> >>>> Applied both, thanks. >>>> >>> >>> With these two patches applied to origin/master GCC 9.2.1 does not >>> produce warnings anymore but for tbs2910_defconfig: >>> >>> u-boot.imx exceeds file size limit: >>> limit: 0x5fc00 bytes >>> actual: 0x60c00 bytes >>> excess: 0x1000 bytes >>> make: *** [Makefile:1159: u-boot.imx] Error 1 >>> make: *** Deleting file 'u-boot.imx' >>> >>> So irrespective of my patches for the USB keyboard we need to address >>> the size limit on TBS2910. >> >> Is that due to my cleanup patches? Can you compare the size by compiling >> without them? That should work if you leave away the -Werror switch. >> >> Regards, >> Simon > > GCC 9.2.1 without your patches but with -Wno-address-of-packed-member: > > u-boot.imx exceeds file size limit: > limit: 0x5fc00 bytes > actual: 0x60c00 bytes > excess: 0x1000 bytes I see, so you have additional options added to the build which trigger the size issue. It would be nice to mention that up front. Do you use any other extra options ?
On 11/22/19 1:14 PM, Marek Vasut wrote: > On 11/22/19 12:58 PM, Heinrich Schuchardt wrote: >> On 11/22/19 8:47 AM, Simon Goldschmidt wrote: >>> On Fri, Nov 22, 2019 at 7:50 AM Heinrich Schuchardt >>> <xypron.glpk@gmx.de> wrote: >>>> >>>> On 11/22/19 1:25 AM, Marek Vasut wrote: >>>>> On 11/21/19 10:15 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> >>>>> >>>>> Applied both, thanks. >>>>> >>>> >>>> With these two patches applied to origin/master GCC 9.2.1 does not >>>> produce warnings anymore but for tbs2910_defconfig: >>>> >>>> u-boot.imx exceeds file size limit: >>>> limit: 0x5fc00 bytes >>>> actual: 0x60c00 bytes >>>> excess: 0x1000 bytes >>>> make: *** [Makefile:1159: u-boot.imx] Error 1 >>>> make: *** Deleting file 'u-boot.imx' >>>> >>>> So irrespective of my patches for the USB keyboard we need to address >>>> the size limit on TBS2910. >>> >>> Is that due to my cleanup patches? Can you compare the size by compiling >>> without them? That should work if you leave away the -Werror switch. >>> >>> Regards, >>> Simon >> >> GCC 9.2.1 without your patches but with -Wno-address-of-packed-member: >> >> u-boot.imx exceeds file size limit: >> limit: 0x5fc00 bytes >> actual: 0x60c00 bytes >> excess: 0x1000 bytes > > I see, so you have additional options added to the build which trigger > the size issue. It would be nice to mention that up front. Do you use > any other extra options ? > Dear Marek, Simon asked me to determine if origin/master exceeds the u-boot.imx size limit when compiled without his patches. The only way to do so is to suppress the build warnings. -Wno-address-of-packed-member is the only option I added to origin/master. This option suppresses the build error that we get without Simon's patches. The only other difference between Gitlab CI and my setup is that I use GCC 9.2.1 (arm-linux-gnueabihf-gcc on Debian Bullseye). These are the sizes of u-boot.bin: 390080 bytes with Simon's patches 390080 bytes with Simon's patches + -Wno-address-of-packed-member 390064 bytes origin/master + -Wno-address-of-packed-member 386248 bytes with Simon's patches + CONFIG_REGEX=n 390024 bytes with Simon patches + "usb: kbd: simplify coding for arrow keys" 390440 bytes with Simon patches + all my USB keyboard patches So I will add a CONFIG option to my "usb: kbd: implement special keys" patch to avoid the code increase. With CONFIG_REGEX=n the image fits into the current board limit in all cases. Soeren could you, please, evaluate if this configuration works with your board. It should only be relevant if multiple network interfaces are supported by U-Boot. Best regards Heinrich
On Fri, Nov 22, 2019 at 06:32:13PM +0100, Heinrich Schuchardt wrote: > On 11/22/19 1:14 PM, Marek Vasut wrote: > > On 11/22/19 12:58 PM, Heinrich Schuchardt wrote: > > > On 11/22/19 8:47 AM, Simon Goldschmidt wrote: > > > > On Fri, Nov 22, 2019 at 7:50 AM Heinrich Schuchardt > > > > <xypron.glpk@gmx.de> wrote: > > > > > > > > > > On 11/22/19 1:25 AM, Marek Vasut wrote: > > > > > > On 11/21/19 10:15 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> > > > > > > > > > > > > Applied both, thanks. > > > > > > > > > > > > > > > > With these two patches applied to origin/master GCC 9.2.1 does not > > > > > produce warnings anymore but for tbs2910_defconfig: > > > > > > > > > > u-boot.imx exceeds file size limit: > > > > > limit: 0x5fc00 bytes > > > > > actual: 0x60c00 bytes > > > > > excess: 0x1000 bytes > > > > > make: *** [Makefile:1159: u-boot.imx] Error 1 > > > > > make: *** Deleting file 'u-boot.imx' > > > > > > > > > > So irrespective of my patches for the USB keyboard we need to address > > > > > the size limit on TBS2910. > > > > > > > > Is that due to my cleanup patches? Can you compare the size by compiling > > > > without them? That should work if you leave away the -Werror switch. > > > > > > > > Regards, > > > > Simon > > > > > > GCC 9.2.1 without your patches but with -Wno-address-of-packed-member: > > > > > > u-boot.imx exceeds file size limit: > > > limit: 0x5fc00 bytes > > > actual: 0x60c00 bytes > > > excess: 0x1000 bytes > > > > I see, so you have additional options added to the build which trigger > > the size issue. It would be nice to mention that up front. Do you use > > any other extra options ? > > > > Dear Marek, > > Simon asked me to determine if origin/master exceeds the u-boot.imx size > limit when compiled without his patches. The only way to do so is to > suppress the build warnings. > > -Wno-address-of-packed-member is the only option I added to > origin/master. This option suppresses the build error that we get > without Simon's patches. For the record, with gcc 7.2, these types of fixes result in: u-boot: add: 0/0, grow: 2/0 bytes: 36/0 (36) function old new delta collect_langs 76 100 +24 composite_setup 2440 2452 +12 spl-u-boot-spl: add: 0/0, grow: 2/0 bytes: 36/0 (36) function old new delta collect_langs 76 100 +24 composite_setup 2440 2452 +12
On 11/22/19 6:32 PM, Heinrich Schuchardt wrote: > On 11/22/19 1:14 PM, Marek Vasut wrote: >> On 11/22/19 12:58 PM, Heinrich Schuchardt wrote: >>> On 11/22/19 8:47 AM, Simon Goldschmidt wrote: >>>> On Fri, Nov 22, 2019 at 7:50 AM Heinrich Schuchardt >>>> <xypron.glpk@gmx.de> wrote: >>>>> >>>>> On 11/22/19 1:25 AM, Marek Vasut wrote: >>>>>> On 11/21/19 10:15 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> >>>>>> >>>>>> Applied both, thanks. >>>>>> >>>>> >>>>> With these two patches applied to origin/master GCC 9.2.1 does not >>>>> produce warnings anymore but for tbs2910_defconfig: >>>>> >>>>> u-boot.imx exceeds file size limit: >>>>> limit: 0x5fc00 bytes >>>>> actual: 0x60c00 bytes >>>>> excess: 0x1000 bytes >>>>> make: *** [Makefile:1159: u-boot.imx] Error 1 >>>>> make: *** Deleting file 'u-boot.imx' >>>>> >>>>> So irrespective of my patches for the USB keyboard we need to address >>>>> the size limit on TBS2910. >>>> >>>> Is that due to my cleanup patches? Can you compare the size by >>>> compiling >>>> without them? That should work if you leave away the -Werror switch. >>>> >>>> Regards, >>>> Simon >>> >>> GCC 9.2.1 without your patches but with -Wno-address-of-packed-member: >>> >>> u-boot.imx exceeds file size limit: >>> limit: 0x5fc00 bytes >>> actual: 0x60c00 bytes >>> excess: 0x1000 bytes >> >> I see, so you have additional options added to the build which trigger >> the size issue. It would be nice to mention that up front. Do you use >> any other extra options ? >> > > Dear Marek, Hi, > Simon asked me to determine if origin/master exceeds the u-boot.imx size > limit when compiled without his patches. The only way to do so is to > suppress the build warnings. > > -Wno-address-of-packed-member is the only option I added to > origin/master. This option suppresses the build error that we get > without Simon's patches. But you somehow got this -Werror into the build too, right ? > The only other difference between Gitlab CI and my setup is that I use > GCC 9.2.1 (arm-linux-gnueabihf-gcc on Debian Bullseye). That's what I use too. > These are the sizes of u-boot.bin: > > 390080 bytes with Simon's patches > 390080 bytes with Simon's patches + -Wno-address-of-packed-member > 390064 bytes origin/master + -Wno-address-of-packed-member > 386248 bytes with Simon's patches + CONFIG_REGEX=n > 390024 bytes with Simon patches + > "usb: kbd: simplify coding for arrow keys" > 390440 bytes with Simon patches + all my USB keyboard patches > > So I will add a CONFIG option to my "usb: kbd: implement special keys" > patch to avoid the code increase. There might be another option. How about improving the encoding of these escape sequences? There seem to be a lot of duplication, e.g. the leading '\e' is in all of them. So is the '[' . Maybe deduplicating those could help ? I did a quick try by putting all the sequences into a table, then removed the \e and sent it explicitly with usb_kbd_put_queue(), and got ~100 bytes saved. And then each of those strings has trailing '\0', which I don't think is needed either, so that might be another few bytes saved. $ arm-linux-gnueabi-readelf -s u-boot | sort -nk 3 | grep usb_kbd can help tracking down these bloat hotspots. > With CONFIG_REGEX=n the image fits into the current board limit in all > cases. > > Soeren could you, please, evaluate if this configuration works with your > board. It should only be relevant if multiple network interfaces are > supported by U-Boot. CONFIG_REGEX is used by setexpr to handle regexes on U-Boot command line. Hence, removing CONFIG_REGEX would degrade the board functionality and that is what I don't want to see.
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 618a7d5016..c98a444245 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,21 @@ 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; + __le16_packed *end = (buf + 252); 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 < end; tmp++) { + if (tmp->val == language) goto repeat; } - *tmp++ = language; + tmp->val = language; repeat: sp++; } @@ -705,7 +714,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 +731,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> --- Changes in v2: - fix compiler warning "dereferencing ‘void *'" drivers/usb/gadget/composite.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)