| Message ID | 1636031398-19867-1-git-send-email-volodymyr.mytnyk@plvision.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: marvell: prestera: fix hw structure laid out | expand |
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. 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 */ union { struct { u8 key; u8 mask; } __packed u8; /* The __packed here makes no sense since this one is aligned but the 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. 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. Arnd
> 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
On Fri, Nov 5, 2021 at 5:33 PM Volodymyr Mytnyk [C] <vmytnyk@marvell.com> wrote: > > > > 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. That is very strange, as the union has an __le64 member that should be 64-bit aligned on x86_64. > > > > 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. I think you need it on both lmode_bmap and mode/speed to get a completely unaligned structure. If you mark phy as __packed, that will implicitly mark lmode_bmap as packed but leave the four-byte alignment on mode and speed, so the entire structure is still four-byte aligned. Arnd
> > > > > > > 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. > > That is very strange, as the union has an __le64 member that should be > 64-bit aligned on x86_64. This is what I get on x86_64 with pahole: --- struct prestera_msg_acl_match { __le32 type; /* 0 4 */ union { struct { u8 key; /* 4 1 */ u8 mask; /* 5 1 */ } u8; /* 4 2 */ struct { __le16 key; /* 4 2 */ __le16 mask; /* 6 2 */ } u16; /* 4 4 */ --- seems like the packed is added implicitly here. > > > > > > 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. > > I think you need it on both lmode_bmap and mode/speed > to get a completely unaligned structure. If you mark phy as __packed, > that will implicitly mark lmode_bmap as packed but leave the > four-byte alignment on mode and speed, so the entire structure > is still four-byte aligned. > > Arnd Makes sence. Looks like I've updated the v4 too quickly. Do you want me to update the v5 now with the changes ? Thanks and Regards, Volodymyr
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c index 4f5f52dcdd9d..f581ab84e38d 100644 --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c @@ -180,109 +180,113 @@ struct prestera_msg_common_resp { struct prestera_msg_ret ret; }; -union prestera_msg_switch_param { - u8 mac[ETH_ALEN]; - __le32 ageing_timeout_ms; -} __packed; - struct prestera_msg_switch_attr_req { struct prestera_msg_cmd cmd; __le32 attr; - union prestera_msg_switch_param param; - u8 pad[2]; + union { + __le32 ageing_timeout_ms; + struct { + u8 mac[ETH_ALEN]; + u8 __pad[2]; + }; + } param; }; struct prestera_msg_switch_init_resp { struct prestera_msg_ret ret; __le32 port_count; __le32 mtu_max; - u8 switch_id; - u8 lag_max; - u8 lag_member_max; __le32 size_tbl_router_nexthop; -} __packed __aligned(4); + u8 switch_id; + u8 lag_max; + u8 lag_member_max; +}; struct prestera_msg_event_port_param { union { struct { - u8 oper; __le32 mode; __le32 speed; + u8 oper; u8 duplex; u8 fc; u8 fec; - } __packed mac; + } mac; struct { - u8 mdix; __le64 lmode_bmap; + u8 mdix; u8 fc; + u8 __pad[2]; } __packed phy; } __packed; -} __packed __aligned(4); +} __packed; struct prestera_msg_port_cap_param { __le64 link_mode; - u8 type; - u8 fec; - u8 fc; - u8 transceiver; -}; + u8 type; + u8 fec; + u8 fc; + u8 transceiver; +} __packed; struct prestera_msg_port_flood_param { u8 type; u8 enable; + u8 __pad[2]; }; union prestera_msg_port_param { + __le32 mtu; + __le32 speed; + __le32 link_mode; u8 admin_state; u8 oper_state; - __le32 mtu; u8 mac[ETH_ALEN]; u8 accept_frm_type; - __le32 speed; u8 learning; u8 flood; - __le32 link_mode; u8 type; u8 duplex; u8 fec; u8 fc; - union { struct { - u8 admin:1; + u8 admin; u8 fc; u8 ap_enable; + u8 __reserved; union { struct { __le32 mode; - u8 inband:1; __le32 speed; - u8 duplex; - u8 fec; - u8 fec_supp; - } __packed reg_mode; + u8 inband; + u8 duplex; + u8 fec; + u8 fec_supp; + } reg_mode; struct { __le32 mode; __le32 speed; - u8 fec; - u8 fec_supp; - } __packed ap_modes[PRESTERA_AP_PORT_MAX]; - } __packed; - } __packed mac; + u8 fec; + u8 fec_supp; + u8 __pad[2]; + } ap_modes[PRESTERA_AP_PORT_MAX]; + }; + } mac; struct { - u8 admin:1; - u8 adv_enable; __le64 modes; __le32 mode; + u8 admin; + u8 adv_enable; u8 mdix; - } __packed phy; + u8 __pad; + } phy; } __packed link; struct prestera_msg_port_cap_param cap; struct prestera_msg_port_flood_param flood_ext; struct prestera_msg_event_port_param link_evt; -} __packed; +}; struct prestera_msg_port_attr_req { struct prestera_msg_cmd cmd; @@ -290,14 +294,12 @@ struct prestera_msg_port_attr_req { __le32 port; __le32 dev; union prestera_msg_port_param param; -} __packed __aligned(4); - +}; struct prestera_msg_port_attr_resp { struct prestera_msg_ret ret; union prestera_msg_port_param param; -} __packed __aligned(4); - +}; struct prestera_msg_port_stats_resp { struct prestera_msg_ret ret; @@ -322,13 +324,13 @@ struct prestera_msg_vlan_req { __le32 port; __le32 dev; __le16 vid; - u8 is_member; - u8 is_tagged; + u8 is_member; + u8 is_tagged; }; struct prestera_msg_fdb_req { struct prestera_msg_cmd cmd; - u8 dest_type; + __le32 flush_mode; union { struct { __le32 port; @@ -336,11 +338,12 @@ struct prestera_msg_fdb_req { }; __le16 lag_id; } dest; - u8 mac[ETH_ALEN]; __le16 vid; - u8 dynamic; - __le32 flush_mode; -} __packed __aligned(4); + u8 dest_type; + u8 dynamic; + u8 mac[ETH_ALEN]; + u8 __pad[2]; +}; struct prestera_msg_bridge_req { struct prestera_msg_cmd cmd; @@ -383,7 +386,7 @@ struct prestera_msg_acl_match { struct { u8 key[ETH_ALEN]; u8 mask[ETH_ALEN]; - } __packed mac; + } mac; } keymask; }; @@ -446,7 +449,8 @@ struct prestera_msg_stp_req { __le32 port; __le32 dev; __le16 vid; - u8 state; + u8 state; + u8 __pad; }; struct prestera_msg_rxtx_req { @@ -497,21 +501,21 @@ union prestera_msg_event_fdb_param { struct prestera_msg_event_fdb { struct prestera_msg_event id; - u8 dest_type; + __le32 vid; union { __le32 port_id; __le16 lag_id; } dest; - __le32 vid; union prestera_msg_event_fdb_param param; -} __packed __aligned(4); + u8 dest_type; +}; -static inline void prestera_hw_build_tests(void) +static void prestera_hw_build_tests(void) { /* check requests */ BUILD_BUG_ON(sizeof(struct prestera_msg_common_req) != 4); BUILD_BUG_ON(sizeof(struct prestera_msg_switch_attr_req) != 16); - BUILD_BUG_ON(sizeof(struct prestera_msg_port_attr_req) != 120); + BUILD_BUG_ON(sizeof(struct prestera_msg_port_attr_req) != 140); BUILD_BUG_ON(sizeof(struct prestera_msg_port_info_req) != 8); BUILD_BUG_ON(sizeof(struct prestera_msg_vlan_req) != 16); BUILD_BUG_ON(sizeof(struct prestera_msg_fdb_req) != 28); @@ -528,7 +532,7 @@ static inline void prestera_hw_build_tests(void) /* check responses */ BUILD_BUG_ON(sizeof(struct prestera_msg_common_resp) != 8); BUILD_BUG_ON(sizeof(struct prestera_msg_switch_init_resp) != 24); - BUILD_BUG_ON(sizeof(struct prestera_msg_port_attr_resp) != 112); + BUILD_BUG_ON(sizeof(struct prestera_msg_port_attr_resp) != 132); BUILD_BUG_ON(sizeof(struct prestera_msg_port_stats_resp) != 248); BUILD_BUG_ON(sizeof(struct prestera_msg_port_info_resp) != 20); BUILD_BUG_ON(sizeof(struct prestera_msg_bridge_resp) != 12); @@ -561,9 +565,9 @@ static int __prestera_cmd_ret(struct prestera_switch *sw, if (err) return err; - if (__le32_to_cpu(ret->cmd.type) != PRESTERA_CMD_TYPE_ACK) + if (ret->cmd.type != __cpu_to_le32(PRESTERA_CMD_TYPE_ACK)) return -EBADE; - if (__le32_to_cpu(ret->status) != PRESTERA_CMD_ACK_OK) + if (ret->status != __cpu_to_le32(PRESTERA_CMD_ACK_OK)) return -EINVAL; return 0;