netdev - Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <MWHPR18MB142149F54F5AB88590B4AB2BDEB79@MWHPR18MB1421.namprd18.prod.outlook.com>
Date:   Sun, 31 Jan 2021 07:01:55 +0000
From:   Hariprasad Kelam <hkelam@...vell.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        Linu Cherian <lcherian@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        Jerin Jacob Kollanukkaran <jerinj@...vell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        Felix Manlunas <fmanlunas@...vell.com>,
        Christina Jacob <cjacob@...vell.com>,
        "Sunil Kovvuri Goutham" <Sunil.Goutham@...ium.com>
Subject: Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY
 FEC statistics

Hi Willem,

> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> Sent: Saturday, January 30, 2021 7:57 PM
> To: Hariprasad Kelam <hkelam@...vell.com>
> Cc: Network Development <netdev@...r.kernel.org>; LKML <linux-
> kernel@...r.kernel.org>; David Miller <davem@...emloft.net>; Jakub
> Kicinski <kuba@...nel.org>; Sunil Kovvuri Goutham
> <sgoutham@...vell.com>; Linu Cherian <lcherian@...vell.com>;
> Geethasowjanya Akula <gakula@...vell.com>; Jerin Jacob Kollanukkaran
> <jerinj@...vell.com>; Subbaraya Sundeep Bhatta <sbhatta@...vell.com>;
> Felix Manlunas <fmanlunas@...vell.com>; Christina Jacob
> <cjacob@...vell.com>; Sunil Kovvuri Goutham
> <Sunil.Goutham@...ium.com>
> Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD
> to get PHY FEC statistics
> On Sat, Jan 30, 2021 at 4:53 AM Hariprasad Kelam <hkelam@...vell.com>
> wrote:
> >
> > Hi Willem,
> >
> > > -----Original Message-----
> > > From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> > > Sent: Thursday, January 28, 2021 1:50 AM
> > > To: Hariprasad Kelam <hkelam@...vell.com>
> > > Cc: Network Development <netdev@...r.kernel.org>; LKML <linux-
> > > kernel@...r.kernel.org>; David Miller <davem@...emloft.net>; Jakub
> > > Kicinski <kuba@...nel.org>; Sunil Kovvuri Goutham
> > > <sgoutham@...vell.com>; Linu Cherian <lcherian@...vell.com>;
> > > Geethasowjanya Akula <gakula@...vell.com>; Jerin Jacob Kollanukkaran
> > > <jerinj@...vell.com>; Subbaraya Sundeep Bhatta
> > > <sbhatta@...vell.com>; Felix Manlunas <fmanlunas@...vell.com>;
> > > Christina Jacob <cjacob@...vell.com>; Sunil Kovvuri Goutham
> > > <Sunil.Goutham@...ium.com>
> > > Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new
> > > CGX_CMD to get PHY FEC statistics
> > >
> > > On Wed, Jan 27, 2021 at 4:04 AM Hariprasad Kelam
> > > <hkelam@...vell.com>
> > > wrote:
> > > >
> > > > From: Felix Manlunas <fmanlunas@...vell.com>
> > > >
> > > > This patch adds support to fetch fec stats from PHY. The stats are
> > > > put in the shared data struct fwdata.  A PHY driver indicates that
> > > > it has FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
> > > >
> > > > Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> > > > CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list
> > > > is in sync with firmware's enum list.
> > > >
> > > > Signed-off-by: Felix Manlunas <fmanlunas@...vell.com>
> > > > Signed-off-by: Christina Jacob <cjacob@...vell.com>
> > > > Signed-off-by: Sunil Kovvuri Goutham <Sunil.Goutham@...ium.com>
> > > > Signed-off-by: Hariprasad Kelam <hkelam@...vell.com>
> > >
> > >
> > > > +struct phy_s {
> > > > +       struct {
> > > > +               u64 can_change_mod_type : 1;
> > > > +               u64 mod_type            : 1;
> > > > +               u64 has_fec_stats       : 1;
> > >
> > > this style is not customary
> >
> > These structures are shared with firmware and stored in a shared memory.
> Any change in size of structures will break compatibility. To avoid frequent
> compatible issues with new vs old firmware we have put spaces where ever
> we see that there could be more fields added in future.
> > So changing this to u8 can have an impact in future.
> 
> My comment was intended much simpler: don't add whitespace between the
> bit-field variable name and its size expression.
> 
>   u64 mod_type:1;
> 
> not
> 
>   u64 mod_type     : 1;
> 
> At least, I have not seen that style anywhere else in the kernel.
Got it . Will fix this in next version.

Thanks,
Hariprasad k