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(+)
On Tue, Nov 2, 2021 at 9:24 AM Geert Uytterhoeven [off-list ref] 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 [off-list ref] wrote:
quoted
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>
quoted
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
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski [off-list ref]:
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
...
[...]