lkml.org 
[lkml]   [2021]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] [-next] net: marvell: prestera: Add explicit padding
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

\
 
 \ /
  Last update: 2021-11-02 09:49    [W:0.056 / U:1.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site