Messages in this thread | | | From | "Volodymyr Mytnyk [C]" <> | Subject | Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding | Date | Tue, 2 Nov 2021 11:36:19 +0000 |
| |
Hi All,
For some unknown reason, the bb5dbf2cc64d5cfa ("net: marvell: prestera: add firmware v4.0 support") changes have been merged into net-next w/o review comments addressed and waiting for the final patch set to be uploaded. Any idea why ?
Right now, I'm working on fixing all the issues/comments and rebasing them based on latest net-next master. Also, my changes include those posted in this thread to fix m68k build and comments related to structure pack/align.
Should I rebase my changes based on yours now ? Is it possible to make a relation chain ?
The bb5dbf2cc64d5cfa mail thread discussion (waiting for new v5 patchset to be uploaded) can be found at:
[PATCH net-next v4] net: marvell: prestera: add firmware v4.0 support https://www.spinics.net/lists/kernel/msg4127689.html
Regards, Volodymyr
> On Tue, Nov 2, 2021 at 9:24 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On m68k: > > > > In function ‘prestera_hw_build_tests’, > > inlined from ‘prestera_hw_switch_init’ at drivers/net/ethernet/marvell/prestera/prestera_hw.c:788:2: > > ././include/linux/compiler_types.h:335:38: error: call to ‘__compiletime_assert_345’ declared with attribute error: BUILD_BUG_ON failed: sizeof(struct prestera_msg_switch_attr_req) != 16 > > ... > > > > The driver assumes structure members are naturally aligned, but does not > > add explicit padding, thus breaking architectures where integral values > > are not always naturally aligned (e.g. on m68k, __alignof(int) is 2, not > > 4). > > > > Fixes: bb5dbf2cc64d5cfa ("net: marvell: prestera: add firmware v4.0 support") > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Looks good to me, > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > > Compile-tested only. > > > > BTW, I sincerely doubt the use of __packed on structs like: > > > > union prestera_msg_switch_param { > > u8 mac[ETH_ALEN]; > > __le32 ageing_timeout_ms; > > } __packed; > > > > This struct is only used as a member in another struct, where it is > > be naturally aligned anyway. > > Agreed, this __packed attribute is clearly bogus and should be removed. > > Same for > > +struct prestera_msg_event_port_param { > + union { > + struct { > + u8 oper; > + __le32 mode; > + __le32 speed; > + u8 duplex; > + u8 fc; > + u8 fec; > + } __packed mac; > + struct { > + u8 mdix; > + __le64 lmode_bmap; > + u8 fc; > + } __packed phy; > + } __packed; > +} __packed __aligned(4); > > This makes no sense at all. I would suggest marking only > the individual fields that are misaligned as __packed, but > not the structure itself. > > and then there is this > > + union { > + struct { > + u8 admin:1; > + u8 fc; > + u8 ap_enable; > + union { > + struct { > + __le32 mode; > + u8 inband:1; > + __le32 speed; > + u8 duplex; > + u8 fec; > + u8 fec_supp; > + } __packed reg_mode; > + struct { > + __le32 mode; > + __le32 speed; > + u8 fec; > + u8 fec_supp; > + } __packed ap_modes[PRESTERA_AP_PORT_MAX]; > + } __packed; > + } __packed mac; > + struct { > + u8 admin:1; > + u8 adv_enable; > + __le64 modes; > + __le32 mode; > + u8 mdix; > + } __packed phy; > + } __packed link; > > which puts misaligned bit fields in the middle of a packed structure! > > Arnd
| |