[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Heinrich Schuchardt]

With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur when
passing a member of a packed structure to le16_to_cpus() on a big endian
system (e.g. P2041RDB_defconfig).

Replace le16_to_cpus() by get_unaligned_le16(). Check defined(__BIG_ENDIAN)
to avoid the introduction of unnecessary instructions on little endian
systems as seen on aarch64.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 common/usb.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index d9bcb5a57e..de6f7a3bf4 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1078,10 +1078,16 @@ int usb_select_config(struct usb_device *dev)
 		return err;

 	/* correct le values */
-	le16_to_cpus(&dev->descriptor.bcdUSB);
-	le16_to_cpus(&dev->descriptor.idVendor);
-	le16_to_cpus(&dev->descriptor.idProduct);
-	le16_to_cpus(&dev->descriptor.bcdDevice);
+#if defined(__BIG_ENDIAN)
+	dev->descriptor.bcdUSB =
+		get_unaligned_le16(&dev->descriptor.bcdUSB);
+	dev->descriptor.idVendor =
+		get_unaligned_le16(&dev->descriptor.idVendor);
+	dev->descriptor.idProduct =
+		get_unaligned_le16(&dev->descriptor.idProduct);
+	dev->descriptor.bcdDevice =
+		get_unaligned_le16(&dev->descriptor.bcdDevice);
+#endif

 	/*
 	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
--
2.24.0

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Marek Vasut]

On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur when
> passing a member of a packed structure to le16_to_cpus() on a big endian
> system (e.g. P2041RDB_defconfig).
> 
> Replace le16_to_cpus() by get_unaligned_le16(). Check defined(__BIG_ENDIAN)
> to avoid the introduction of unnecessary instructions on little endian
> systems as seen on aarch64.

I would expect the compiler would optimize such stuff out ?
Can we do without the ifdef ?

[...]

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Heinrich Schuchardt]

On 12/17/19 11:00 AM, Marek Vasut wrote:
> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur when
>> passing a member of a packed structure to le16_to_cpus() on a big endian
>> system (e.g. P2041RDB_defconfig).
>>
>> Replace le16_to_cpus() by get_unaligned_le16(). Check defined(__BIG_ENDIAN)
>> to avoid the introduction of unnecessary instructions on little endian
>> systems as seen on aarch64.
>
> I would expect the compiler would optimize such stuff out ?
> Can we do without the ifdef ?

When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
adds assembler instructions:

         /* correct le values */
         dev->descriptor.bcdUSB =
   48:   79020660        strh    w0, [x19, #258]
   4c:   39442660        ldrb    w0, [x19, #265]
   50:   2a002020        orr     w0, w1, w0, lsl #8
   54:   39442a61        ldrb    w1, [x19, #266]
                 get_unaligned_le16(&dev->descriptor.bcdUSB);
         dev->descriptor.idVendor =
   58:   79021260        strh    w0, [x19, #264]
   5c:   39442e60        ldrb    w0, [x19, #267]
   60:   2a002020        orr     w0, w1, w0, lsl #8
   64:   39443261        ldrb    w1, [x19, #268]
                 get_unaligned_le16(&dev->descriptor.idVendor);
         dev->descriptor.idProduct =
   68:   79021660        strh    w0, [x19, #266]
   6c:   39443660        ldrb    w0, [x19, #269]
   70:   2a002020        orr     w0, w1, w0, lsl #8
                 get_unaligned_le16(&dev->descriptor.idProduct);
         dev->descriptor.bcdDevice =
   74:   79021a60        strh    w0, [x19, #268]
         udelay(1000 * msec);
   78:   d2807d00        mov     x0, #0x3e8                      // #1000
   7c:   94000000        bl      0 <udelay>
          * one microframe duration here (1mS for USB 1.x , 125uS for
USB 2.0).
          */
         mdelay(1);

Best regards

Heinrich

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Marek Vasut]

On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
> On 12/17/19 11:00 AM, Marek Vasut wrote:
>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
>>> when
>>> passing a member of a packed structure to le16_to_cpus() on a big endian
>>> system (e.g. P2041RDB_defconfig).
>>>
>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
>>> defined(__BIG_ENDIAN)
>>> to avoid the introduction of unnecessary instructions on little endian
>>> systems as seen on aarch64.
>>
>> I would expect the compiler would optimize such stuff out ?
>> Can we do without the ifdef ?
> 
> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
> adds assembler instructions:

Why ?

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Heinrich Schuchardt]

On 12/17/19 12:19 PM, Marek Vasut wrote:
> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
>> On 12/17/19 11:00 AM, Marek Vasut wrote:
>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
>>>> when
>>>> passing a member of a packed structure to le16_to_cpus() on a big endian
>>>> system (e.g. P2041RDB_defconfig).
>>>>
>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
>>>> defined(__BIG_ENDIAN)
>>>> to avoid the introduction of unnecessary instructions on little endian
>>>> systems as seen on aarch64.
>>>
>>> I would expect the compiler would optimize such stuff out ?
>>> Can we do without the ifdef ?
>>
>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
>> adds assembler instructions:
>
> Why ?
>

I am not a GCC developer. I simply observed that GCC currently cannot
optimize this away on its own. That is why I added the #ifdef.

Identifying that bit operations like << 8 in __get_unaligned_le16() have
zero effect is not an easy task when developing a compiler. You would
have to follow the flow of every bit.

Best regards

Heinrich

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Marek Vasut]

On 12/17/19 12:59 PM, Heinrich Schuchardt wrote:
> On 12/17/19 12:19 PM, Marek Vasut wrote:
>> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
>>> On 12/17/19 11:00 AM, Marek Vasut wrote:
>>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
>>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
>>>>> when
>>>>> passing a member of a packed structure to le16_to_cpus() on a big
>>>>> endian
>>>>> system (e.g. P2041RDB_defconfig).
>>>>>
>>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
>>>>> defined(__BIG_ENDIAN)
>>>>> to avoid the introduction of unnecessary instructions on little endian
>>>>> systems as seen on aarch64.
>>>>
>>>> I would expect the compiler would optimize such stuff out ?
>>>> Can we do without the ifdef ?
>>>
>>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
>>> adds assembler instructions:
>>
>> Why ?
>>
> 
> I am not a GCC developer. I simply observed that GCC currently cannot
> optimize this away on its own. That is why I added the #ifdef.

Are we now adding workarounds instead of solving issues properly?

> Identifying that bit operations like << 8 in __get_unaligned_le16() have
> zero effect is not an easy task when developing a compiler. You would
> have to follow the flow of every bit.

Maybe the fix is then to somehow optimize the get_unaligned_le16() to
help the compiler ?

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Heinrich Schuchardt]

On 12/17/19 1:19 PM, Marek Vasut wrote:
> On 12/17/19 12:59 PM, Heinrich Schuchardt wrote:
>> On 12/17/19 12:19 PM, Marek Vasut wrote:
>>> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
>>>> On 12/17/19 11:00 AM, Marek Vasut wrote:
>>>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
>>>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
>>>>>> when
>>>>>> passing a member of a packed structure to le16_to_cpus() on a big
>>>>>> endian
>>>>>> system (e.g. P2041RDB_defconfig).
>>>>>>
>>>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
>>>>>> defined(__BIG_ENDIAN)
>>>>>> to avoid the introduction of unnecessary instructions on little endian
>>>>>> systems as seen on aarch64.
>>>>>
>>>>> I would expect the compiler would optimize such stuff out ?
>>>>> Can we do without the ifdef ?
>>>>
>>>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
>>>> adds assembler instructions:
>>>
>>> Why ?
>>>
>>
>> I am not a GCC developer. I simply observed that GCC currently cannot
>> optimize this away on its own. That is why I added the #ifdef.
>
> Are we now adding workarounds instead of solving issues properly?
>
>> Identifying that bit operations like << 8 in __get_unaligned_le16() have
>> zero effect is not an easy task when developing a compiler. You would
>> have to follow the flow of every bit.
>
> Maybe the fix is then to somehow optimize the get_unaligned_le16() to
> help the compiler ?

Inside get_unaligned_le16() it is not known that we will be reassigning
to the same memory location. So I cannot imagine what to improve here.

You could invent new functions for in place byte swapping. But that
would only move the #ifdef to a different place.

Best regards

Heinrich

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Marek Vasut]

On 12/17/19 8:32 PM, Heinrich Schuchardt wrote:
> On 12/17/19 1:19 PM, Marek Vasut wrote:
>> On 12/17/19 12:59 PM, Heinrich Schuchardt wrote:
>>> On 12/17/19 12:19 PM, Marek Vasut wrote:
>>>> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
>>>>> On 12/17/19 11:00 AM, Marek Vasut wrote:
>>>>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
>>>>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
>>>>>>> when
>>>>>>> passing a member of a packed structure to le16_to_cpus() on a big
>>>>>>> endian
>>>>>>> system (e.g. P2041RDB_defconfig).
>>>>>>>
>>>>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
>>>>>>> defined(__BIG_ENDIAN)
>>>>>>> to avoid the introduction of unnecessary instructions on little
>>>>>>> endian
>>>>>>> systems as seen on aarch64.
>>>>>>
>>>>>> I would expect the compiler would optimize such stuff out ?
>>>>>> Can we do without the ifdef ?
>>>>>
>>>>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
>>>>> adds assembler instructions:
>>>>
>>>> Why ?
>>>>
>>>
>>> I am not a GCC developer. I simply observed that GCC currently cannot
>>> optimize this away on its own. That is why I added the #ifdef.
>>
>> Are we now adding workarounds instead of solving issues properly?
>>
>>> Identifying that bit operations like << 8 in __get_unaligned_le16() have
>>> zero effect is not an easy task when developing a compiler. You would
>>> have to follow the flow of every bit.
>>
>> Maybe the fix is then to somehow optimize the get_unaligned_le16() to
>> help the compiler ?
> 
> Inside get_unaligned_le16() it is not known that we will be reassigning
> to the same memory location. So I cannot imagine what to improve here.
> 
> You could invent new functions for in place byte swapping. But that
> would only move the #ifdef to a different place.

Isn't there already such a function in Linux ?

Also, why would you need the ifdef if the compiler would now know that
the operation is noop ?

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Tom Rini]

On Tue, Dec 17, 2019 at 08:52:01PM +0100, Marek Vasut wrote:
> On 12/17/19 8:32 PM, Heinrich Schuchardt wrote:
> > On 12/17/19 1:19 PM, Marek Vasut wrote:
> >> On 12/17/19 12:59 PM, Heinrich Schuchardt wrote:
> >>> On 12/17/19 12:19 PM, Marek Vasut wrote:
> >>>> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
> >>>>> On 12/17/19 11:00 AM, Marek Vasut wrote:
> >>>>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
> >>>>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
> >>>>>>> when
> >>>>>>> passing a member of a packed structure to le16_to_cpus() on a big
> >>>>>>> endian
> >>>>>>> system (e.g. P2041RDB_defconfig).
> >>>>>>>
> >>>>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
> >>>>>>> defined(__BIG_ENDIAN)
> >>>>>>> to avoid the introduction of unnecessary instructions on little
> >>>>>>> endian
> >>>>>>> systems as seen on aarch64.
> >>>>>>
> >>>>>> I would expect the compiler would optimize such stuff out ?
> >>>>>> Can we do without the ifdef ?
> >>>>>
> >>>>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
> >>>>> adds assembler instructions:
> >>>>
> >>>> Why ?
> >>>>
> >>>
> >>> I am not a GCC developer. I simply observed that GCC currently cannot
> >>> optimize this away on its own. That is why I added the #ifdef.
> >>
> >> Are we now adding workarounds instead of solving issues properly?
> >>
> >>> Identifying that bit operations like << 8 in __get_unaligned_le16() have
> >>> zero effect is not an easy task when developing a compiler. You would
> >>> have to follow the flow of every bit.
> >>
> >> Maybe the fix is then to somehow optimize the get_unaligned_le16() to
> >> help the compiler ?
> > 
> > Inside get_unaligned_le16() it is not known that we will be reassigning
> > to the same memory location. So I cannot imagine what to improve here.
> > 
> > You could invent new functions for in place byte swapping. But that
> > would only move the #ifdef to a different place.
> 
> Isn't there already such a function in Linux ?
> 
> Also, why would you need the ifdef if the compiler would now know that
> the operation is noop ?

This particular patch looks to me exactly why we want to follow the
Linux Kernel and disable this particular warning for GCC like we already
do for LLVM.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20191230/02ff3b3b/attachment.sig>

[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Masahiro Yamada]

On Tue, Dec 31, 2019 at 12:41 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 17, 2019 at 08:52:01PM +0100, Marek Vasut wrote:
> > On 12/17/19 8:32 PM, Heinrich Schuchardt wrote:
> > > On 12/17/19 1:19 PM, Marek Vasut wrote:
> > >> On 12/17/19 12:59 PM, Heinrich Schuchardt wrote:
> > >>> On 12/17/19 12:19 PM, Marek Vasut wrote:
> > >>>> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
> > >>>>> On 12/17/19 11:00 AM, Marek Vasut wrote:
> > >>>>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
> > >>>>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
> > >>>>>>> when
> > >>>>>>> passing a member of a packed structure to le16_to_cpus() on a big
> > >>>>>>> endian
> > >>>>>>> system (e.g. P2041RDB_defconfig).
> > >>>>>>>
> > >>>>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
> > >>>>>>> defined(__BIG_ENDIAN)
> > >>>>>>> to avoid the introduction of unnecessary instructions on little
> > >>>>>>> endian
> > >>>>>>> systems as seen on aarch64.
> > >>>>>>
> > >>>>>> I would expect the compiler would optimize such stuff out ?
> > >>>>>> Can we do without the ifdef ?
> > >>>>>
> > >>>>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
> > >>>>> adds assembler instructions:
> > >>>>
> > >>>> Why ?
> > >>>>
> > >>>
> > >>> I am not a GCC developer. I simply observed that GCC currently cannot
> > >>> optimize this away on its own. That is why I added the #ifdef.
> > >>
> > >> Are we now adding workarounds instead of solving issues properly?
> > >>
> > >>> Identifying that bit operations like << 8 in __get_unaligned_le16() have
> > >>> zero effect is not an easy task when developing a compiler. You would
> > >>> have to follow the flow of every bit.
> > >>
> > >> Maybe the fix is then to somehow optimize the get_unaligned_le16() to
> > >> help the compiler ?
> > >
> > > Inside get_unaligned_le16() it is not known that we will be reassigning
> > > to the same memory location. So I cannot imagine what to improve here.
> > >
> > > You could invent new functions for in place byte swapping. But that
> > > would only move the #ifdef to a different place.
> >
> > Isn't there already such a function in Linux ?
> >
> > Also, why would you need the ifdef if the compiler would now know that
> > the operation is noop ?
>
> This particular patch looks to me exactly why we want to follow the
> Linux Kernel and disable this particular warning for GCC like we already
> do for LLVM.

Agree. I think we can follow
Linux commit 6f303d60534c46aa1a239f29c321f95c83dda748



-- 
Best Regards
Masahiro Yamada