* [PATCH] [-next] net: marvell: prestera: Add explicit padding
@ 2021-11-02 8:24 Geert Uytterhoeven
2021-11-02 8:47 ` Arnd Bergmann
2021-11-03 1:20 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2021-11-02 8:24 UTC (permalink / raw)
To: Taras Chornyi, David S . Miller, Jakub Kicinski,
Volodymyr Mytnyk, Yevhen Orlov, Vadym Kochan
Cc: Arnd Bergmann, netdev, linux-kernel, Geert Uytterhoeven
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(+)
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 {
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding
2021-11-02 8:24 [PATCH] [-next] net: marvell: prestera: Add explicit padding Geert Uytterhoeven
@ 2021-11-02 8:47 ` Arnd Bergmann
2021-11-02 11:36 ` Volodymyr Mytnyk [C]
2021-11-03 1:20 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2021-11-02 8:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Taras Chornyi, David S . Miller, Jakub Kicinski,
Volodymyr Mytnyk, Yevhen Orlov, Vadym Kochan, Arnd Bergmann,
Networking, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding
2021-11-02 8:47 ` Arnd Bergmann
@ 2021-11-02 11:36 ` Volodymyr Mytnyk [C]
2021-11-03 1:10 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Volodymyr Mytnyk [C] @ 2021-11-02 11:36 UTC (permalink / raw)
To: Arnd Bergmann, Geert Uytterhoeven, Andrew Lunn
Cc: Taras Chornyi [C],
David S . Miller, Jakub Kicinski, Yevhen Orlov, Vadym Kochan [C],
Networking, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding
2021-11-02 11:36 ` Volodymyr Mytnyk [C]
@ 2021-11-03 1:10 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2021-11-03 1:10 UTC (permalink / raw)
To: Volodymyr Mytnyk [C]
Cc: Arnd Bergmann, Geert Uytterhoeven, Andrew Lunn, Taras Chornyi [C],
David S . Miller, Yevhen Orlov, Vadym Kochan [C],
Networking, Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding
2021-11-02 8:24 [PATCH] [-next] net: marvell: prestera: Add explicit padding Geert Uytterhoeven
2021-11-02 8:47 ` Arnd Bergmann
@ 2021-11-03 1:20 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-03 1:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: tchornyi, davem, kuba, vmytnyk, yevhen.orlov, vkochan, arnd,
netdev, linux-kernel
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-03 1:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-02 8:24 [PATCH] [-next] net: marvell: prestera: Add explicit padding Geert Uytterhoeven
2021-11-02 8:47 ` Arnd Bergmann
2021-11-02 11:36 ` Volodymyr Mytnyk [C]
2021-11-03 1:10 ` Jakub Kicinski
2021-11-03 1:20 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.