* [PATCH net v3] net: marvell: prestera: fix hw structure laid out
@ 2021-11-04 13:09 Volodymyr Mytnyk
2021-11-04 13:26 ` Arnd Bergmann
0 siblings, 1 reply; 5+ messages in thread
From: Volodymyr Mytnyk @ 2021-11-04 13:09 UTC (permalink / raw)
To: netdev
Cc: Taras Chornyi, Mickey Rachamim, Serhiy Pshyk, Andrew Lunn,
Arnd Bergmann, Geert Uytterhoeven, Denis Kirjanov,
Volodymyr Mytnyk, Taras Chornyi, David S. Miller, Jakub Kicinski,
Vadym Kochan, Yevhen Orlov, linux-kernel
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>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out
2021-11-04 13:09 [PATCH net v3] net: marvell: prestera: fix hw structure laid out Volodymyr Mytnyk
@ 2021-11-04 13:26 ` Arnd Bergmann
2021-11-05 16:33 ` Volodymyr Mytnyk [C]
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2021-11-04 13:26 UTC (permalink / raw)
To: Volodymyr Mytnyk
Cc: Networking, Taras Chornyi, Mickey Rachamim, Serhiy Pshyk,
Andrew Lunn, Arnd Bergmann, Geert Uytterhoeven, Denis Kirjanov,
Volodymyr Mytnyk, Taras Chornyi, David S. Miller, Jakub Kicinski,
Vadym Kochan, Yevhen Orlov, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out
2021-11-04 13:26 ` Arnd Bergmann
@ 2021-11-05 16:33 ` Volodymyr Mytnyk [C]
2021-11-05 16:56 ` Arnd Bergmann
0 siblings, 1 reply; 5+ messages in thread
From: Volodymyr Mytnyk [C] @ 2021-11-05 16:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Networking, Taras Chornyi, Mickey Rachamim, Serhiy Pshyk,
Andrew Lunn, Geert Uytterhoeven, Denis Kirjanov,
Taras Chornyi [C],
David S. Miller, Jakub Kicinski, Vadym Kochan [C],
Yevhen Orlov, Linux Kernel Mailing List
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out
2021-11-05 16:33 ` Volodymyr Mytnyk [C]
@ 2021-11-05 16:56 ` Arnd Bergmann
2021-11-06 13:03 ` Volodymyr Mytnyk [C]
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2021-11-05 16:56 UTC (permalink / raw)
To: Volodymyr Mytnyk [C]
Cc: Arnd Bergmann, Networking, Taras Chornyi, Mickey Rachamim,
Serhiy Pshyk, Andrew Lunn, Geert Uytterhoeven, Denis Kirjanov,
Taras Chornyi [C],
David S. Miller, Jakub Kicinski, Vadym Kochan [C],
Yevhen Orlov, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out
2021-11-05 16:56 ` Arnd Bergmann
@ 2021-11-06 13:03 ` Volodymyr Mytnyk [C]
0 siblings, 0 replies; 5+ messages in thread
From: Volodymyr Mytnyk [C] @ 2021-11-06 13:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Networking, Taras Chornyi, Mickey Rachamim, Serhiy Pshyk,
Andrew Lunn, Geert Uytterhoeven, Denis Kirjanov,
Taras Chornyi [C],
David S. Miller, Jakub Kicinski, Vadym Kochan [C],
Yevhen Orlov, Linux Kernel Mailing List
>
> > >
> > > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-06 13:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-04 13:09 [PATCH net v3] net: marvell: prestera: fix hw structure laid out Volodymyr Mytnyk
2021-11-04 13:26 ` Arnd Bergmann
2021-11-05 16:33 ` Volodymyr Mytnyk [C]
2021-11-05 16:56 ` Arnd Bergmann
2021-11-06 13:03 ` Volodymyr Mytnyk [C]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).