[U-Boot] [PATCH v2 1/2] usb: composite: fix possible alignment issues

[Marek Vasut]

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 at 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 at 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.