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@gmail.com>
Sent: Saturday, January 30, 2021 7:57 PM
To: Hariprasad Kelam <redacted>
Cc: Network Development <redacted>; LKML <linux-
kernel@vger.kernel.org>; David Miller [off-list ref]; Jakub
Kicinski [off-list ref]; Sunil Kovvuri Goutham
[off-list ref]; Linu Cherian [off-list ref];
Geethasowjanya Akula [off-list ref]; Jerin Jacob Kollanukkaran
[off-list ref]; Subbaraya Sundeep Bhatta [off-list ref];
Felix Manlunas [off-list ref]; Christina Jacob
[off-list ref]; Sunil Kovvuri Goutham
[off-list ref]
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 [off-list ref]
wrote:

quoted

Hi Willem,

quoted

-----Original Message-----
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Sent: Thursday, January 28, 2021 1:50 AM
To: Hariprasad Kelam <redacted>
Cc: Network Development <redacted>; LKML <linux-
kernel@vger.kernel.org>; David Miller [off-list ref]; Jakub
Kicinski [off-list ref]; Sunil Kovvuri Goutham
[off-list ref]; Linu Cherian [off-list ref];
Geethasowjanya Akula [off-list ref]; Jerin Jacob Kollanukkaran
[off-list ref]; Subbaraya Sundeep Bhatta
[off-list ref]; Felix Manlunas [off-list ref];
Christina Jacob [off-list ref]; Sunil Kovvuri Goutham
[off-list ref]
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
[off-list ref]
wrote:

quoted

From: Felix Manlunas <redacted>

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 <redacted>
Signed-off-by: Christina Jacob <redacted>
Signed-off-by: Sunil Kovvuri Goutham <redacted>
Signed-off-by: Hariprasad Kelam <redacted>

quoted

+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.

quoted

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