Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding

[Volodymyr Mytnyk [C]]

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> 
> Looks good to me,
> 
> Reviewed-by: Arnd Bergmann <arnd@xxxxxxxx>
> 
> > 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