[PATCH 1/1] usb: avoid -Werror=address-of-packed-member
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
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 ?
[...]
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
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 ?
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
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 ?
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
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 ?
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.
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
participants (4)
-
Heinrich Schuchardt -
Marek Vasut -
Masahiro Yamada -
Tom Rini