Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1636031398-19867-1-git-send-email-volodymyr.mytnyk@plvision.eu>
In-Reply-To: <1636031398-19867-1-git-send-email-volodymyr.mytnyk@plvision.eu>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 4 Nov 2021 14:26:42 +0100
Message-ID: <CAK8P3a2nbEFGPRWKwjLYOz3wROLOk1SN-6Wd7-sNkaEuuid==w@mail.gmail.com>
Subject: Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out
To:     Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>
Cc:     Networking <netdev@vger.kernel.org>,
        Taras Chornyi <taras.chornyi@plvision.eu>,
        Mickey Rachamim <mickeyr@marvell.com>,
        Serhiy Pshyk <serhiy.pshyk@plvision.eu>,
        Andrew Lunn <andrew@lunn.ch>, Arnd Bergmann <arnd@arndb.de>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Denis Kirjanov <dkirjanov@suse.de>,
        Volodymyr Mytnyk <vmytnyk@marvell.com>,
        Taras Chornyi <tchornyi@marvell.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Vadym Kochan <vkochan@marvell.com>,
        Yevhen Orlov <yevhen.orlov@plvision.eu>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Nov 4, 2021 at 2:09 PM Volodymyr Mytnyk
<volodymyr.mytnyk@plvision.eu> wrote:
>
> From: Volodymyr Mytnyk <vmytnyk@marvell.com>
>
> The prestera FW v4.0 support commit has been merged
> accidentally w/o review comments addressed and waiting
> for the final patch set to be uploaded. So, fix the remaining
> comments related to structure laid out and build issues.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: bb5dbf2cc64d ("net: marvell: prestera: add firmware v4.0 support")
> Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com>

I saw this warning today on net-next:

drivers/net/ethernet/marvell/prestera/prestera_hw.c:285:1: error:
alignment 1 of 'union prestera_msg_port_param' is less than 4
[-Werror=packed-not-aligned]

and this is addressed by your patch.

However, there is still this structure that lacks explicit padding:

struct prestera_msg_acl_match {
        __le32 type;
        /* there is a four-byte hole on most architectures, but not on
x86-32 or m68k */
        union {
                struct {
                        u8 key;
                        u8 mask;
                } __packed u8;
/* The __packed here makes no sense since this one is aligned but the
other ones are not */
                struct {
                        __le16 key;
                        __le16 mask;
                } u16;
                struct {
                        __le32 key;
                        __le32 mask;
                } u32;
                struct {
                        __le64 key;
                        __le64 mask;
                } u64;
                struct {
                        u8 key[ETH_ALEN];
                        u8 mask[ETH_ALEN];
                } mac;
        } keymask;
};

and a minor issue in

struct prestera_msg_event_port_param {
        union {
                struct {
                        __le32 mode;
                        __le32 speed;
                        u8 oper;
                        u8 duplex;
                        u8 fc;
                        u8 fec;
                } mac;
                struct {
                        __le64 lmode_bmap;
                        u8 mdix;
                        u8 fc;
                        u8 __pad[2];
                } __packed phy;
        } __packed;
} __packed;

There is no need to make the outer aggregates __packed, I would
mark only the innermost ones here: mode, speed and lmode_bmap.
Same for prestera_msg_port_cap_param and prestera_msg_port_param.


It would be best to add some comments next to the __packed
attributes to explain exactly which members are misaligned
and why. I see that most of the packed structures are included in
union prestera_msg_port_param, which makes that packed
as well, however nothing that uses this union puts it on a misaligned
address, so I don't see what the purpose is.

       Arnd