From: Volodymyr Mytnyk <[email protected]>
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 <[email protected]>
Fixes: bb5dbf2cc64d ("net: marvell: prestera: add firmware v4.0 support")
Signed-off-by: Volodymyr Mytnyk <[email protected]>
---
Changes in V2:
- fix structure laid out discussed in:
+ [PATCH net-next v4] net: marvell: prestera: add firmware v4.0 support
https://www.spinics.net/lists/kernel/msg4127689.html
+ [PATCH] [-next] net: marvell: prestera: Add explicit padding
https://www.spinics.net/lists/kernel/msg4130293.html
Changes in V3:
- update commit message
- fix more laid out comments
- split into two patches suggested in:
https://www.spinics.net/lists/netdev/msg778322.html
.../net/ethernet/marvell/prestera/prestera_hw.c | 124 +++++++++++----------
1 file changed, 64 insertions(+), 60 deletions(-)
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;
--
2.7.4
On Thu, Nov 4, 2021 at 2:09 PM Volodymyr Mytnyk
<[email protected]> wrote:
>
> From: Volodymyr Mytnyk <[email protected]>
>
> 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 <[email protected]>
> Fixes: bb5dbf2cc64d ("net: marvell: prestera: add firmware v4.0 support")
> Signed-off-by: Volodymyr Mytnyk <[email protected]>
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
> <[email protected]> wrote:
> >
> > From: Volodymyr Mytnyk <[email protected]>
> >
> > 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 <[email protected]>
> > Fixes: bb5dbf2cc64d ("net: marvell: prestera: add firmware v4.0 support")
> > Signed-off-by: Volodymyr Mytnyk <[email protected]>
>
> 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] <[email protected]> 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