Messages in this thread | | | From | "Volodymyr Mytnyk [C]" <> | Subject | Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out | Date | Fri, 5 Nov 2021 16:33:19 +0000 |
| |
> On Thu, Nov 4, 2021 at 2:09 PM Volodymyr Mytnyk > <volodymyr.mytnyk@plvision.eu> wrote: > > > > From: Volodymyr Mytnyk <vmytnyk@marvell.com> > > > > The prestera FW v4.0 support commit has been merged > > accidentally w/o review comments addressed and waiting > > for the final patch set to be uploaded. So, fix the remaining > > comments related to structure laid out and build issues. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Fixes: bb5dbf2cc64d ("net: marvell: prestera: add firmware v4.0 support") > > Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com> > > I saw this warning today on net-next: > > drivers/net/ethernet/marvell/prestera/prestera_hw.c:285:1: error: > alignment 1 of 'union prestera_msg_port_param' is less than 4 > [-Werror=packed-not-aligned] > > and this is addressed by your patch.
yes, I don't see any errors on the patchwork anymore.
> > However, there is still this structure that lacks explicit padding: > > struct prestera_msg_acl_match { > __le32 type; > /* there is a four-byte hole on most architectures, but not on > x86-32 or m68k */
Checked holes on x86_64 with pahole, and there is no holes. Maybe on some other there will be. Will add 4byte member here to make sure. Thx.
> union { > struct { > u8 key; > u8 mask; > } __packed u8; > /* The __packed here makes no sense since this one is aligned but the
right, will remove it.
> other ones are not */ > struct { > __le16 key; > __le16 mask; > } u16; > struct { > __le32 key; > __le32 mask; > } u32; > struct { > __le64 key; > __le64 mask; > } u64; > struct { > u8 key[ETH_ALEN]; > u8 mask[ETH_ALEN]; > } mac; > } keymask; > }; > > and a minor issue in > > struct prestera_msg_event_port_param { > union { > struct { > __le32 mode; > __le32 speed; > u8 oper; > u8 duplex; > u8 fc; > u8 fec; > } mac; > struct { > __le64 lmode_bmap; > u8 mdix; > u8 fc; > u8 __pad[2]; > } __packed phy; > } __packed; > } __packed; > > There is no need to make the outer aggregates __packed, I would > mark only the innermost ones here: mode, speed and lmode_bmap. > Same for prestera_msg_port_cap_param and prestera_msg_port_param. >
Will add __packed only to innermost ones. Looks like only phy is required to have __packed.
> > It would be best to add some comments next to the __packed > attributes to explain exactly which members are misaligned > and why. I see that most of the packed structures are included in > union prestera_msg_port_param, which makes that packed > as well, however nothing that uses this union puts it on a misaligned > address, so I don't see what the purpose is.
Will try to get rid of the __packed attributes from prestera_msg_port_param by aligning the members in nested union of that struct. Thx.
> > Arnd
| |