| Message ID | 20211102082433.3820514-1-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 236f57fe1b8853fb3505502c0f94ae64d153ae92 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [-next] net: marvell: prestera: Add explicit padding | expand |
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
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
On Tue, 2 Nov 2021 11:36:19 +0000 Volodymyr Mytnyk [C] wrote:
> Should I rebase my changes based on yours now ? Is it possible to make a relation chain ?
Please rebase your changes on top of net/master.
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 2 Nov 2021 09:24:33 +0100 you 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 > ... > > [...] Here is the summary with links: - [-next] net: marvell: prestera: Add explicit padding https://git.kernel.org/netdev/net/c/236f57fe1b88 You are awesome, thank you!
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c index 41ba17cb29657392..4f5f52dcdd9d2814 100644 --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c @@ -189,6 +189,7 @@ struct prestera_msg_switch_attr_req { struct prestera_msg_cmd cmd; __le32 attr; union prestera_msg_switch_param param; + u8 pad[2]; }; struct prestera_msg_switch_init_resp { @@ -313,6 +314,7 @@ struct prestera_msg_port_info_resp { __le32 hw_id; __le32 dev_id; __le16 fp_id; + u8 pad[2]; }; struct prestera_msg_vlan_req { @@ -345,11 +347,13 @@ struct prestera_msg_bridge_req { __le32 port; __le32 dev; __le16 bridge; + u8 pad[2]; }; struct prestera_msg_bridge_resp { struct prestera_msg_ret ret; __le16 bridge; + u8 pad[2]; }; struct prestera_msg_acl_action { @@ -408,16 +412,19 @@ struct prestera_msg_acl_ruleset_bind_req { __le32 port; __le32 dev; __le16 ruleset_id; + u8 pad[2]; }; struct prestera_msg_acl_ruleset_req { struct prestera_msg_cmd cmd; __le16 id; + u8 pad[2]; }; struct prestera_msg_acl_ruleset_resp { struct prestera_msg_ret ret; __le16 id; + u8 pad[2]; }; struct prestera_msg_span_req { @@ -425,11 +432,13 @@ struct prestera_msg_span_req { __le32 port; __le32 dev; u8 id; + u8 pad[3]; }; struct prestera_msg_span_resp { struct prestera_msg_ret ret; u8 id; + u8 pad[3]; }; struct prestera_msg_stp_req { @@ -443,6 +452,7 @@ struct prestera_msg_stp_req { struct prestera_msg_rxtx_req { struct prestera_msg_cmd cmd; u8 use_sdma; + u8 pad[3]; }; struct prestera_msg_rxtx_resp { @@ -455,12 +465,14 @@ struct prestera_msg_lag_req { __le32 port; __le32 dev; __le16 lag_id; + u8 pad[2]; }; struct prestera_msg_cpu_code_counter_req { struct prestera_msg_cmd cmd; u8 counter_type; u8 code; + u8 pad[2]; }; struct mvsw_msg_cpu_code_counter_ret {
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> --- 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. Some of the other __packed attributes look fishy to me, too. --- drivers/net/ethernet/marvell/prestera/prestera_hw.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)