From mboxrd@z Thu Jan  1 00:00:00 1970
From: Sunil Kovvuri <sunil.kovvuri@gmail.com>
Subject: Re: [PATCH 08/20] octeontx2-af: Alloc and config NPC MCAM entry at a time
Date: Fri, 9 Nov 2018 22:43:01 +0530
Message-ID: <CA+sq2CcEi4yfuwP2dN-pqOKrwdiEPBbVUGEwst1ayh6eKCpukA@mail.gmail.com>
References: <1541702161-30673-1-git-send-email-sunil.kovvuri@gmail.com>
 <1541702161-30673-9-git-send-email-sunil.kovvuri@gmail.com>
 <CAK8P3a2=CvyXkPexQHH6oTzUccJo=ayNgz0R9Lpsd3-YmNxxEg@mail.gmail.com>
 <CA+sq2CeORXF8kL4gQx9JgfEy-EYMrAr6i5rsUdO0Znw-X5H-fg@mail.gmail.com> <CAK8P3a27xsV0iWf8oCVxjRmLxhbtrR25R=LjouTdfWqjfduZpw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: Linux Netdev List <netdev@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>, linux-soc@vger.kernel.org,
        Sunil Goutham <sgoutham@marvell.com>
To: Arnd Bergmann <arnd@arndb.de>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-wr1-f67.google.com ([209.85.221.67]:32810 "EHLO
        mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727961AbeKJCyn (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 9 Nov 2018 21:54:43 -0500
In-Reply-To: <CAK8P3a27xsV0iWf8oCVxjRmLxhbtrR25R=LjouTdfWqjfduZpw@mail.gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Fri, Nov 9, 2018 at 4:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Nov 9, 2018 at 5:21 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
> >
> > On Fri, Nov 9, 2018 at 2:13 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Thu, Nov 8, 2018 at 7:37 PM <sunil.kovvuri@gmail.com> wrote:
> > > > @@ -666,4 +668,20 @@ struct npc_mcam_unmap_counter_req {
> > > >         u8  all;   /* Unmap all entries using this counter ? */
> > > >  };
> > > >
> > > > +struct npc_mcam_alloc_and_write_entry_req {
> > > > +       struct mbox_msghdr hdr;
> > > > +       struct mcam_entry entry_data;
> > > > +       u16 ref_entry;
> > > > +       u8  priority;    /* Lower or higher w.r.t ref_entry */
> > > > +       u8  intf;        /* Rx or Tx interface */
> > > > +       u8  enable_entry;/* Enable this MCAM entry ? */
> > > > +       u8  alloc_cntr;  /* Allocate counter and map ? */
> > > > +};
> > >
> > > I noticed that this structure requires padding at the end because
> > > struct mbox_msghdr has a 32-bit alignment requirement. For
> > > data structures in an interface, I'd recommend avoiding that kind
> > > of padding and adding reserved fields or widening the types
> > > accordingly.
> > >
> >
> > When there are multiple messages in the mailbox, each message starts
> > at a 16byte aligned offset. So struct mbox_msghdr is always aligned.
> > I think adding reserved fields is not needed here.
> >
> > ===
> > struct mbox_msghdr *otx2_mbox_alloc_msg_rsp(struct otx2_mbox *mbox, int devid,
> >                                             int size, int size_rsp)
> > {
> >         size = ALIGN(size, MBOX_MSG_ALIGN);
> > ===
> >
> > Is this what you were referring to ?
> >
>
> No, I mean the padding at the end of the structure. An example
> would be a structure like
>
> struct s {
>     u16 a;
>     u32 b;
>     u16 c;
> };
>
> Since b is aligned to four bytes, you get padding between a and b.
> On top of that, you also get padding after c to make the size of
> structure itself be a multiple of its alignment. For interfaces, we
> should avoid both kinds of padding. This can be done by marking
> members as __packed (usually I don't recommend that), by
> changing the size of members, or by adding explicit 'reserved'
> fields in place of the padding.
>
> > > I also noticed a similar problem in struct mbox_msghdr. Maybe
> > > use the 'pahole' tool to check for this kind of padding in the
> > > API structures.
>
>      Arnd

Got your point now and agree that padding has to be avoided.
But this is a big change and above pointed structure is not
the only one as this applies to all structures in the file.

Would it be okay if I submit a separate patch after this series
addressing all structures ?

Thanks,
Sunil.