Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Sat, Jan 30, 2021 at 4:53 AM Hariprasad Kelam <hkelam@marvell.com> wrote:
>
> Hi Willem,
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Sent: Thursday, January 28, 2021 1:50 AM
> > To: Hariprasad Kelam <hkelam@marvell.com>
> > Cc: Network Development <netdev@vger.kernel.org>; LKML <linux-
> > kernel@vger.kernel.org>; David Miller <davem@davemloft.net>; Jakub
> > Kicinski <kuba@kernel.org>; Sunil Kovvuri Goutham
> > <sgoutham@marvell.com>; Linu Cherian <lcherian@marvell.com>;
> > Geethasowjanya Akula <gakula@marvell.com>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>;
> > Felix Manlunas <fmanlunas@marvell.com>; Christina Jacob
> > <cjacob@marvell.com>; Sunil Kovvuri Goutham
> > <Sunil.Goutham@cavium.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@marvell.com>
> > wrote:
> > >
> > > From: Felix Manlunas <fmanlunas@marvell.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@marvell.com>
> > > Signed-off-by: Christina Jacob <cjacob@marvell.com>
> > > Signed-off-by: Sunil Kovvuri Goutham <Sunil.Goutham@cavium.com>
> > > Signed-off-by: Hariprasad Kelam <hkelam@marvell.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.