From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Fri, 22 Nov 2019 19:28:12 +0100 Subject: [U-Boot] [PATCH v2 1/2] usb: composite: fix possible alignment issues In-Reply-To: References: <20191121211523.7219-1-simon.k.r.goldschmidt@gmail.com> <36eb9135-50cd-304d-a906-a759cd862717@gmx.de> <39634233-5121-0c2a-08cc-100c6118e7aa@gmx.de> <820fb5fc-58b0-3a39-2e14-2f40416a5427@denx.de> Message-ID: <51c60383-ca66-c46e-a6af-942da6d8a7d5@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>> 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 >>>>>> >>>>>> 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.