* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member @ 2019-12-17 9:27 Heinrich Schuchardt 2019-12-17 10:00 ` Marek Vasut 0 siblings, 1 reply; 10+ messages in thread From: Heinrich Schuchardt @ 2019-12-17 9:27 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 9:27 [PATCH 1/1] usb: avoid -Werror=address-of-packed-member Heinrich Schuchardt @ 2019-12-17 10:00 ` Marek Vasut 2019-12-17 11:14 ` Heinrich Schuchardt 0 siblings, 1 reply; 10+ messages in thread From: Marek Vasut @ 2019-12-17 10:00 UTC (permalink / raw) To: u-boot 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 ? [...] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 10:00 ` Marek Vasut @ 2019-12-17 11:14 ` Heinrich Schuchardt 2019-12-17 11:19 ` Marek Vasut 0 siblings, 1 reply; 10+ messages in thread From: Heinrich Schuchardt @ 2019-12-17 11:14 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 11:14 ` Heinrich Schuchardt @ 2019-12-17 11:19 ` Marek Vasut 2019-12-17 11:59 ` Heinrich Schuchardt 0 siblings, 1 reply; 10+ messages in thread From: Marek Vasut @ 2019-12-17 11:19 UTC (permalink / raw) To: u-boot 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 ? ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 11:19 ` Marek Vasut @ 2019-12-17 11:59 ` Heinrich Schuchardt 2019-12-17 12:19 ` Marek Vasut 0 siblings, 1 reply; 10+ messages in thread From: Heinrich Schuchardt @ 2019-12-17 11:59 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 11:59 ` Heinrich Schuchardt @ 2019-12-17 12:19 ` Marek Vasut 2019-12-17 19:32 ` Heinrich Schuchardt 0 siblings, 1 reply; 10+ messages in thread From: Marek Vasut @ 2019-12-17 12:19 UTC (permalink / raw) To: u-boot 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 ? ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 12:19 ` Marek Vasut @ 2019-12-17 19:32 ` Heinrich Schuchardt 2019-12-17 19:52 ` Marek Vasut 0 siblings, 1 reply; 10+ messages in thread From: Heinrich Schuchardt @ 2019-12-17 19:32 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 19:32 ` Heinrich Schuchardt @ 2019-12-17 19:52 ` Marek Vasut 2019-12-30 15:41 ` Tom Rini 0 siblings, 1 reply; 10+ messages in thread From: Marek Vasut @ 2019-12-17 19:52 UTC (permalink / raw) To: u-boot 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 ? ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-17 19:52 ` Marek Vasut @ 2019-12-30 15:41 ` Tom Rini 2019-12-31 4:00 ` Masahiro Yamada 0 siblings, 1 reply; 10+ messages in thread From: Tom Rini @ 2019-12-30 15:41 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] usb: avoid -Werror=address-of-packed-member 2019-12-30 15:41 ` Tom Rini @ 2019-12-31 4:00 ` Masahiro Yamada 0 siblings, 0 replies; 10+ messages in thread From: Masahiro Yamada @ 2019-12-31 4:00 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-12-31 4:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-17 9:27 [PATCH 1/1] usb: avoid -Werror=address-of-packed-member Heinrich Schuchardt 2019-12-17 10:00 ` Marek Vasut 2019-12-17 11:14 ` Heinrich Schuchardt 2019-12-17 11:19 ` Marek Vasut 2019-12-17 11:59 ` Heinrich Schuchardt 2019-12-17 12:19 ` Marek Vasut 2019-12-17 19:32 ` Heinrich Schuchardt 2019-12-17 19:52 ` Marek Vasut 2019-12-30 15:41 ` Tom Rini 2019-12-31 4:00 ` Masahiro Yamada
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox