[PATCH net-next v3 0/5] ionic: FEC reporting fixes and .get_fec_stats handler

[PATCH net-next v3 0/5] ionic: FEC reporting fixes and .get_fec_stats handler

[Eric Joyner]

Add a .get_fec_stats ethtool handler, and fix the FEC reporting it
depends on.

Patch 1 writes the IONIC_STAT_INVALID markers in extra_stats one
counter at a time, so a reader that tests a single counter cannot see a
torn value.

Patches 2-4 fix ionic_get_fecparam(). fec->fec reported the constant
set of supported modes instead of the configured mode, and
fec->active_fec reported the requested mode instead of the operational
one. Patch 2 has a Fixes: tag, but patches 3 and 4 rework the lines it
changes, so it is sent to net-next with them rather than to net. The
bug only affects what ethtool reports.

Patch 5 adds the handler. It reports statistics only while the
operational FEC mode is RS.

The netdev AI review of v2, which Simon forwarded, asked whether the
per-bin local really guarantees a single read of each counter. It does
not, so v3 reads every counter with READ_ONCE() and drops the struct
copy. The port_info lifetime problem that the AI review raised on v1
and v2 is fixed in net-next by commit d90d1a768b3b ("ionic: fix
port_info use-after-free on firmware recovery").

Claude was used while developing this series: to investigate the v2
review questions, and to help write the code changes and commit
messages. The authors reviewed every change. The series was tested on
Pollara and Vulcano NICs.

Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
Changes in v3:
- Read each counter with READ_ONCE() and drop the struct copy of
  extra_stats (netdev AI review of v2).
- Report statistics based on status.fec_type, the operational FEC mode,
  instead of config.fec_type, matching the active FEC mode that patch 4
  reports.
- Add patches 1-4, which fix the FEC reporting the handler depends on.
  Patches 1-4 are by Nikhil, who is also a Co-developed-by on patch 5.
- Drop Vadim's Reviewed-by from v1, since the code has changed.
- Rebase onto current net-next.
- Link to v2: https://lore.kernel.org/r/20260814-ionic-get-fec-stats-v2-1-13f0e7d2e702@amd.com

Changes in v2:
- Latch each histogram bin into a local before testing it against
  IONIC_STAT_INVALID, so the sentinel check and the store cannot be
  folded into two separate reads of the firmware-updated DMA buffer.
- Explain in the commit message why the RS FEC check gates every
  reported counter rather than just the histogram: they all come from
  the RS decoder, including fec_corrected_bits_total despite its name.
- Rebased onto current net-next.
- Link to v1: https://lore.kernel.org/r/20260731214021.15279-1-eric.joyner@amd.com

---
Eric Joyner (1):
      ionic: Add .get_fec_stats ethtool handler

Nikhil P. Rao (4):
      ionic: write the extra_stats invalid markers one counter at a time
      ionic: report the configured FEC mode in ethtool_fecparam.fec
      ionic: map the FEC type through a helper in ionic_get_fecparam()
      ionic: report the operational FEC mode from ethtool --show-fec

 .../net/ethernet/pensando/ionic/ionic_ethtool.c    | 116 ++++++++++++++++++---
 drivers/net/ethernet/pensando/ionic/ionic_main.c   |   8 +-
 drivers/net/ethernet/pensando/ionic/ionic_stats.h  |   2 +
 3 files changed, 112 insertions(+), 14 deletions(-)
---
base-commit: 7b6ab4dbc156e2f96419715130b3d5738a306c93
change-id: 20260814-ionic-get-fec-stats-990736964908

Best regards,
-- 
Eric Joyner <eric.joyner@amd.com>

[PATCH net-next v3 1/5] ionic: write the extra_stats invalid markers one counter at a time

[Eric Joyner]

From: "Nikhil P. Rao" <nikhil.rao@amd.com>

ionic_port_init() marks every counter in the device-written extra_stats
block invalid before asking firmware to refresh it, so that a statistic
this driver knows about but the firmware does not is reported as absent
rather than as zero.  It does that with a memset() of the whole block.

The .get_fec_stats handler added later in this series reads those
counters one at a time and tests each against IONIC_STAT_INVALID to
decide whether to report it.  memset() gives no guarantee about the
width of its stores, so such a reader can observe a counter that is
neither its old value nor the marker.

The block is entirely __le64, each member naturally aligned, so walk it
as an array and write the markers one counter at a time instead.  A
counter read on its own is then either a complete value or the marker.

Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_main.c  | 8 ++++++--
 drivers/net/ethernet/pensando/ionic/ionic_stats.h | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index 38b47b959a11..8197df5532f5 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -12,6 +12,7 @@
 #include "ionic.h"
 #include "ionic_bus.h"
 #include "ionic_lif.h"
+#include "ionic_stats.h"
 #include "ionic_debugfs.h"
 
 MODULE_DESCRIPTION(IONIC_DRV_DESCRIPTION);
@@ -723,7 +724,9 @@ int ionic_port_init(struct ionic *ionic)
 	struct ionic_identity *ident = &ionic->ident;
 	struct ionic_dev *idev = &ionic->idev;
 	bool allocated = false;
+	__le64 *stat;
 	size_t sz;
+	size_t i;
 	int err;
 
 	if (!idev->port_info) {
@@ -741,8 +744,9 @@ int ionic_port_init(struct ionic *ionic)
 	/* If the driver knows about more "extra stats" than the firmware,
 	 * make sure these stats are marked as invalid.
 	 */
-	memset(&idev->port_info->extra_stats, 0xff,
-	       sizeof(idev->port_info->extra_stats));
+	stat = (__le64 *)&idev->port_info->extra_stats;
+	for (i = 0; i < sizeof(idev->port_info->extra_stats) / sizeof(*stat); i++)
+		WRITE_ONCE(stat[i], IONIC_STAT_INVALID);
 
 	WRITE_ONCE(idev->link_down_count_init, false);
 
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_stats.h b/drivers/net/ethernet/pensando/ionic/ionic_stats.h
index 7ed935868e84..8a0edcc86fb8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_stats.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_stats.h
@@ -4,6 +4,8 @@
 #ifndef _IONIC_STATS_H_
 #define _IONIC_STATS_H_
 
+#include <linux/ethtool.h>
+
 #define IONIC_STAT_INVALID	(cpu_to_le64(~0ULL))
 
 #define IONIC_STAT_TO_OFFSET(type, stat_name) (offsetof(type, stat_name))

-- 
2.53.0

[PATCH net-next v3 2/5] ionic: report the configured FEC mode in ethtool_fecparam.fec

[Eric Joyner]

From: "Nikhil P. Rao" <nikhil.rao@amd.com>

ionic_get_fecparam() fills fec->fec with the constant
ETHTOOL_FEC_OFF | ETHTOOL_FEC_RS | ETHTOOL_FEC_BASER.  The uAPI defines
fec as the configured FEC modes, so a port configured for RS reports
all three:

  Supported/Configured FEC encodings: Off RS BaseR

where it should report RS alone.  Take fec->fec from config.fec_type.
Supported modes belong in ETHTOOL_GLINKSETTINGS, which
ionic_get_link_ksettings() already fills.

fec->active_fec still reads config.fec_type; reporting the operational
mode is a separate change.

Fixes: e95f922f4c2f ("ionic: implement ethtool set-fec")
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 119ac720c078..317dbf96964e 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -427,17 +427,18 @@ static int ionic_get_fecparam(struct net_device *netdev,
 	switch (lif->ionic->idev.port_info->config.fec_type) {
 	case IONIC_PORT_FEC_TYPE_NONE:
 		fec->active_fec = ETHTOOL_FEC_OFF;
+		fec->fec = ETHTOOL_FEC_OFF;
 		break;
 	case IONIC_PORT_FEC_TYPE_RS:
 		fec->active_fec = ETHTOOL_FEC_RS;
+		fec->fec = ETHTOOL_FEC_RS;
 		break;
 	case IONIC_PORT_FEC_TYPE_FC:
 		fec->active_fec = ETHTOOL_FEC_BASER;
+		fec->fec = ETHTOOL_FEC_BASER;
 		break;
 	}
 
-	fec->fec = ETHTOOL_FEC_OFF | ETHTOOL_FEC_RS | ETHTOOL_FEC_BASER;
-
 	return 0;
 }
 

-- 
2.53.0

[PATCH net-next v3 3/5] ionic: map the FEC type through a helper in ionic_get_fecparam()

[Eric Joyner]

From: "Nikhil P. Rao" <nikhil.rao@amd.com>

ionic_get_fecparam() maps config.fec_type onto the ethtool FEC values
with a switch that assigns both fec->active_fec and fec->fec in each
arm.  A later patch gives the two fields distinct sources, which would
mean duplicating the switch.

Move the mapping into ionic_fec_type_to_ethtool() and call it once per
field.

No functional change.

Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
 .../net/ethernet/pensando/ionic/ionic_ethtool.c    | 34 +++++++++++++---------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 317dbf96964e..bf7c70438a8c 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -419,25 +419,31 @@ static int ionic_set_pauseparam(struct net_device *netdev,
 	return 0;
 }
 
-static int ionic_get_fecparam(struct net_device *netdev,
-			      struct ethtool_fecparam *fec)
+static u32 ionic_fec_type_to_ethtool(u8 fec_type)
 {
-	struct ionic_lif *lif = netdev_priv(netdev);
-
-	switch (lif->ionic->idev.port_info->config.fec_type) {
+	switch (fec_type) {
 	case IONIC_PORT_FEC_TYPE_NONE:
-		fec->active_fec = ETHTOOL_FEC_OFF;
-		fec->fec = ETHTOOL_FEC_OFF;
-		break;
+		return ETHTOOL_FEC_OFF;
 	case IONIC_PORT_FEC_TYPE_RS:
-		fec->active_fec = ETHTOOL_FEC_RS;
-		fec->fec = ETHTOOL_FEC_RS;
-		break;
+		return ETHTOOL_FEC_RS;
 	case IONIC_PORT_FEC_TYPE_FC:
-		fec->active_fec = ETHTOOL_FEC_BASER;
-		fec->fec = ETHTOOL_FEC_BASER;
-		break;
+		return ETHTOOL_FEC_BASER;
+	default:
+		return 0;
 	}
+}
+
+static int ionic_get_fecparam(struct net_device *netdev,
+			      struct ethtool_fecparam *fec)
+{
+	struct ionic_port_info *port_info;
+	struct ionic_lif *lif;
+
+	lif = netdev_priv(netdev);
+	port_info = lif->ionic->idev.port_info;
+
+	fec->active_fec = ionic_fec_type_to_ethtool(port_info->config.fec_type);
+	fec->fec = ionic_fec_type_to_ethtool(port_info->config.fec_type);
 
 	return 0;
 }

-- 
2.53.0

[PATCH net-next v3 4/5] ionic: report the operational FEC mode from ethtool --show-fec

[Eric Joyner]

From: "Nikhil P. Rao" <nikhil.rao@amd.com>

ionic_get_fecparam() fills fec->active_fec from config.fec_type, the mode
the host requested, which on an autoneg link is whatever was last
programmed while the link negotiates something else.  Read it from
status.fec_type, where firmware reports the operational mode.

status.fec_type was carved out of the reserved area by commit
c4e7a75a096c ("ionic: updates to ionic FW api description"), and
firmware older than that leaves the byte reserved, where zero reads as
IONIC_PORT_FEC_TYPE_NONE, so --show-fec reports FEC off on a link that
may be running it.

Poisoning the byte before PORT_INIT, the way the driver marks unwritten
extra_stats counters with IONIC_STAT_INVALID, does not work here:
firmware builds the whole port status block and writes it over the DMA
region, so the marker is gone whether or not that firmware knows the
field.  Accept the wrong answer on firmware that old.

Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index bf7c70438a8c..2e067481bb15 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -442,8 +442,10 @@ static int ionic_get_fecparam(struct net_device *netdev,
 	lif = netdev_priv(netdev);
 	port_info = lif->ionic->idev.port_info;
 
-	fec->active_fec = ionic_fec_type_to_ethtool(port_info->config.fec_type);
-	fec->fec = ionic_fec_type_to_ethtool(port_info->config.fec_type);
+	fec->active_fec =
+		ionic_fec_type_to_ethtool(READ_ONCE(port_info->status.fec_type));
+	fec->fec =
+		ionic_fec_type_to_ethtool(READ_ONCE(port_info->config.fec_type));
 
 	return 0;
 }

-- 
2.53.0

[PATCH net-next v3 5/5] ionic: Add .get_fec_stats ethtool handler

[Eric Joyner]

Reports FEC statistics totals and an 802.3ck FEC histogram. Per-lane
counts currently aren't supported. Every counter reported here comes
from the RS decoder, including fec_corrected_bits_total despite its
name, and the only histogram format firmware produces is the 16-bin one
from RS(544,514), so nothing is reported unless RS is the active FEC
mode.

These are physical port counters, so virtual functions are skipped the
same way ionic_get_link_ext_stats() skips them, rather than reporting the
port's counters as if they belonged to the VF.

The reporting of these statistics is gated by DEV_CAP_EXTRA_STATS and
checks for IONIC_STAT_INVALID, since only the newest devices support
reporting all of these stats. Older devices can only report some of the
statistics or not at all, and so the output will properly exclude those
unsupported statistics.

The firmware writes these counters by DMA while they are being read.
Use READ_ONCE() to ensure that the IONIC_STAT_INVALID test and the store
see the same value. Counters are still read one at a time, so the set
reported can span more than one firmware update.

Co-developed-by: Nikhil P. Rao <nikhil.rao@amd.com>
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
 .../net/ethernet/pensando/ionic/ionic_ethtool.c    | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 2e067481bb15..1d32fa811003 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -450,6 +450,88 @@ static int ionic_get_fecparam(struct net_device *netdev,
 	return 0;
 }
 
+#define IONIC_FEC_STAT(dst, src)			\
+	do {						\
+		__le64 __val = READ_ONCE(src);		\
+							\
+		if (__val != IONIC_STAT_INVALID)	\
+			(dst) = le64_to_cpu(__val);	\
+	} while (0)
+
+static const struct ethtool_fec_hist_range ionic_fec_hist_ranges[] = {
+	{ 0, 0},
+	{ 1, 1},
+	{ 2, 2},
+	{ 3, 3},
+	{ 4, 4},
+	{ 5, 5},
+	{ 6, 6},
+	{ 7, 7},
+	{ 8, 8},
+	{ 9, 9},
+	{ 10, 10},
+	{ 11, 11},
+	{ 12, 12},
+	{ 13, 13},
+	{ 14, 14},
+	{ 15, 15},
+	{ 0, 0},
+};
+
+static void
+ionic_fill_fec_hist(const struct ionic_port_extra_stats *port_extra_stats,
+		    struct ethtool_fec_hist *hist)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(port_extra_stats->fec_codeword_error_bin); i++) {
+		__le64 val = READ_ONCE(port_extra_stats->fec_codeword_error_bin[i]);
+
+		if (val == IONIC_STAT_INVALID)
+			return;
+
+		hist->values[i].sum = le64_to_cpu(val);
+	}
+
+	hist->ranges = ionic_fec_hist_ranges;
+}
+
+static void ionic_get_fec_stats(struct net_device *netdev,
+				struct ethtool_fec_stats *fec_stats,
+				struct ethtool_fec_hist *hist)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	struct ionic_port_info *port_info;
+
+	if (lif->ionic->pdev->is_virtfn)
+		return;
+
+	if (test_bit(IONIC_LIF_F_FW_RESET, lif->state))
+		return;
+
+	if (!(lif->ionic->ident.dev.capabilities &
+	      cpu_to_le64(IONIC_DEV_CAP_EXTRA_STATS)))
+		return;
+
+	port_info = lif->ionic->idev.port_info;
+	if (!port_info) {
+		netdev_err_once(netdev, "port_info not initialized\n");
+		return;
+	}
+
+	if (READ_ONCE(port_info->status.fec_type) != IONIC_PORT_FEC_TYPE_RS)
+		return;
+
+	IONIC_FEC_STAT(fec_stats->corrected_blocks.total,
+		       port_info->extra_stats.rsfec_correctable_blocks);
+	IONIC_FEC_STAT(fec_stats->uncorrectable_blocks.total,
+		       port_info->extra_stats.rsfec_uncorrectable_blocks);
+	IONIC_FEC_STAT(fec_stats->corrected_bits.total,
+		       port_info->extra_stats.fec_corrected_bits_total);
+
+	ionic_fill_fec_hist(&port_info->extra_stats, hist);
+}
+
 static int ionic_set_fecparam(struct net_device *netdev,
 			      struct ethtool_fecparam *fec)
 {
@@ -1184,6 +1266,7 @@ static const struct ethtool_ops ionic_ethtool_ops = {
 	.get_module_eeprom_by_page	= ionic_get_module_eeprom_by_page,
 	.get_pauseparam		= ionic_get_pauseparam,
 	.set_pauseparam		= ionic_set_pauseparam,
+	.get_fec_stats		= ionic_get_fec_stats,
 	.get_fecparam		= ionic_get_fecparam,
 	.set_fecparam		= ionic_set_fecparam,
 	.get_ts_info		= ionic_get_ts_info,

-- 
2.53.0

Re: [PATCH net-next v3 0/5] ionic: FEC reporting fixes and .get_fec_stats handler

[Eric Joyner]

On 11/4/2026 1:19 PM, Eric Joyner wrote:
> Add a .get_fec_stats ethtool handler, and fix the FEC reporting it
> depends on.
> 
> Patch 1 writes the IONIC_STAT_INVALID markers in extra_stats one
> counter at a time, so a reader that tests a single counter cannot see a
> torn value.
> 
> Patches 2-4 fix ionic_get_fecparam(). fec->fec reported the constant
> set of supported modes instead of the configured mode, and
> fec->active_fec reported the requested mode instead of the operational
> one. Patch 2 has a Fixes: tag, but patches 3 and 4 rework the lines it
> changes, so it is sent to net-next with them rather than to net. The
> bug only affects what ethtool reports.
> 
> Patch 5 adds the handler. It reports statistics only while the
> operational FEC mode is RS.
> 
> The netdev AI review of v2, which Simon forwarded, asked whether the
> per-bin local really guarantees a single read of each counter. It does
> not, so v3 reads every counter with READ_ONCE() and drops the struct
> copy. The port_info lifetime problem that the AI review raised on v1
> and v2 is fixed in net-next by commit d90d1a768b3b ("ionic: fix
> port_info use-after-free on firmware recovery").
> 
> Claude was used while developing this series: to investigate the v2
> review questions, and to help write the code changes and commit
> messages. The authors reviewed every change. The series was tested on
> Pollara and Vulcano NICs.
> 
> Signed-off-by: Eric Joyner <eric.joyner@amd.com>
> ---
> Changes in v3:
> - Read each counter with READ_ONCE() and drop the struct copy of
>   extra_stats (netdev AI review of v2).
> - Report statistics based on status.fec_type, the operational FEC mode,
>   instead of config.fec_type, matching the active FEC mode that patch 4
>   reports.
> - Add patches 1-4, which fix the FEC reporting the handler depends on.
>   Patches 1-4 are by Nikhil, who is also a Co-developed-by on patch 5.
> - Drop Vadim's Reviewed-by from v1, since the code has changed.
> - Rebase onto current net-next.
> - Link to v2: https://lore.kernel.org/r/20260814-ionic-get-fec-stats-v2-1-13f0e7d2e702@amd.com
> 
> Changes in v2:
> - Latch each histogram bin into a local before testing it against
>   IONIC_STAT_INVALID, so the sentinel check and the store cannot be
>   folded into two separate reads of the firmware-updated DMA buffer.
> - Explain in the commit message why the RS FEC check gates every
>   reported counter rather than just the histogram: they all come from
>   the RS decoder, including fec_corrected_bits_total despite its name.
> - Rebased onto current net-next.
> - Link to v1: https://lore.kernel.org/r/20260731214021.15279-1-eric.joyner@amd.com
> 
> ---
> Eric Joyner (1):
>       ionic: Add .get_fec_stats ethtool handler
> 
> Nikhil P. Rao (4):
>       ionic: write the extra_stats invalid markers one counter at a time
>       ionic: report the configured FEC mode in ethtool_fecparam.fec
>       ionic: map the FEC type through a helper in ionic_get_fecparam()
>       ionic: report the operational FEC mode from ethtool --show-fec
> 
>  .../net/ethernet/pensando/ionic/ionic_ethtool.c    | 116 ++++++++++++++++++---
>  drivers/net/ethernet/pensando/ionic/ionic_main.c   |   8 +-
>  drivers/net/ethernet/pensando/ionic/ionic_stats.h  |   2 +
>  3 files changed, 112 insertions(+), 14 deletions(-)
> ---
> base-commit: 7b6ab4dbc156e2f96419715130b3d5738a306c93
> change-id: 20260814-ionic-get-fec-stats-990736964908
> 
> Best regards,

The Sashiko review from sashiko.dev says no regressions:
https://sashiko.dev/#/patchset/20261104-ionic-get-fec-stats-v3-0-b261fd53d0a7%40amd.com

But I don't see an entry for this patchset in
https://netdev-ai.bots.linux.dev/sashiko/#/ after searching for it; does it need
a manual submission there?

- Eric

Re: [PATCH net-next v3 2/5] ionic: report the configured FEC mode in ethtool_fecparam.fec

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: report the configured FEC mode in ethtool_fecparam.fec

This patch changes ionic_get_fecparam() so that fec->fec reports only the
configured FEC mode from config.fec_type. Before, it reported the constant
Off | RS | BaseR. The commit message says supported modes are already
reported through ETHTOOL_GLINKSETTINGS by ionic_get_link_ksettings().

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index 119ac720c0787..317dbf96964ef 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -427,17 +427,18 @@ static int ionic_get_fecparam(struct net_device *netdev,
>  	switch (lif->ionic->idev.port_info->config.fec_type) {
>  	case IONIC_PORT_FEC_TYPE_NONE:
>  		fec->active_fec = ETHTOOL_FEC_OFF;
> +		fec->fec = ETHTOOL_FEC_OFF;
>  		break;

[ ... ]

> -	fec->fec = ETHTOOL_FEC_OFF | ETHTOOL_FEC_RS | ETHTOOL_FEC_BASER;
> -
>  	return 0;
>  }

[Severity: Medium]
With this line removed, does any ethtool interface still report FEC off as
an available mode on ionic?

The commit message says:

  Supported modes belong in ETHTOOL_GLINKSETTINGS, which
  ionic_get_link_ksettings() already fills.

But ionic_get_link_ksettings() only adds BaseR and RS to supported. It
never sets FEC_NONE:

drivers/net/ethernet/pensando/ionic/ionic_ethtool.c:ionic_get_link_ksettings() {
    ...
	ethtool_link_ksettings_add_link_mode(ks, supported, FEC_BASER);
	ethtool_link_ksettings_add_link_mode(ks, supported, FEC_RS);
	if (idev->port_info->config.fec_type == IONIC_PORT_FEC_TYPE_FC)
		ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_BASER);
	else if (idev->port_info->config.fec_type == IONIC_PORT_FEC_TYPE_RS)
		ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_RS);
    ...
}

When config.fec_type is IONIC_PORT_FEC_TYPE_NONE, advertising gets no FEC
bit at all.

The device does support FEC off. ionic_set_fecparam() maps both
ETHTOOL_FEC_NONE and ETHTOOL_FEC_OFF to IONIC_PORT_FEC_TYPE_NONE and
programs it through IONIC_PORT_ATTR_FEC.

After this patch, GFECPARAM can return ETHTOOL_FEC_OFF as the configured
mode, but GLINKSETTINGS never lists it as supported. Userspace that checks
the supported modes before configuring would conclude that off is not
available.

Should ionic_get_link_ksettings() add FEC_NONE to supported, and also to
advertising when config.fec_type is IONIC_PORT_FEC_TYPE_NONE? Drivers such
as ice, i40e, mlx5, bnxt, nfp, hns3, otx2 and fbnic set FEC_NONE in their
supported link modes.

A related issue is pre-existing and not introduced by this patch.
ionic_get_link_ksettings() hardcodes RS and BaseR as supported for every
transceiver and speed. It ignores the firmware's per-port list in
union ionic_port_identity.fec_type[4].

Because the commit now points to GLINKSETTINGS for supported modes, that
list may not match the hardware. As far as I can tell, no later patch in
the series touches ionic_get_link_ksettings().
-- 
pw-bot: cr

Re: [PATCH net-next v3 4/5] ionic: report the operational FEC mode from ethtool --show-fec

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: report the operational FEC mode from ethtool --show-fec

This commit changes ionic_get_fecparam() to fill active_fec from the
firmware-reported status.fec_type instead of the host-requested
config.fec_type. The commit message notes that firmware older than the
status.fec_type field will report FEC as off.

> Poisoning the byte before PORT_INIT, the way the driver marks unwritten
> extra_stats counters with IONIC_STAT_INVALID, does not work here:
> firmware builds the whole port status block and writes it over the DMA
> region, so the marker is gone whether or not that firmware knows the
> field.  Accept the wrong answer on firmware that old.
>
> Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
> Signed-off-by: Eric Joyner <eric.joyner@amd.com>

[Severity: Low]
This isn't a bug, but should this have a Fixes: tag? Reporting
config.fec_type as active_fec goes back to commit e95f922f4c2f ("ionic:
implement ethtool set-fec").

The sibling patch in this series, "ionic: report the configured FEC mode
in ethtool_fecparam.fec", changes the same function and carries
Fixes: e95f922f4c2f.

If the tag was left off on purpose, for example to keep the old firmware
behavior change out of stable backports, could the commit message say so?

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index bf7c70438a8c3..2e067481bb153 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -442,8 +442,10 @@ static int ionic_get_fecparam(struct net_device *netdev,
>  	lif = netdev_priv(netdev);
>  	port_info = lif->ionic->idev.port_info;
>  
> -	fec->active_fec = ionic_fec_type_to_ethtool(port_info->config.fec_type);
> -	fec->fec = ionic_fec_type_to_ethtool(port_info->config.fec_type);
> +	fec->active_fec =
> +		ionic_fec_type_to_ethtool(READ_ONCE(port_info->status.fec_type));

[Severity: Medium]
Does this break forced-FEC links on firmware older than commit
c4e7a75a096c ("ionic: updates to ionic FW api description")?

On that firmware, status.fec_type is still a reserved byte that reads as
0. That value is IONIC_PORT_FEC_TYPE_NONE, so ionic_fec_type_to_ethtool()
returns ETHTOOL_FEC_OFF for every link.

ionic_set_fecparam() only accepts FEC changes while autoneg is disabled:

	if (lif->ionic->idev.port_info->config.an_enable) {
		netdev_err(netdev, "FEC request not allowed while autoneg is enabled\n");
		return -EINVAL;
	}

So when config.an_enable is clear, config.fec_type is the mode the port
is running. Before this patch, links forced to RS or BaseR were reported
correctly. With this change, they show "Active FEC encoding: Off" on old
firmware.

The driver has no minimum firmware version check (ionic_identify() only
logs fw_version), so that firmware is still accepted. The rest of the
series doesn't touch ionic_get_fecparam(), so this is still the behavior
after the last patch.

The commit message says "Accept the wrong answer on firmware that old",
but it doesn't mention that forced-FEC links, which used to report
correctly, now report the wrong mode too.

Could active_fec fall back to config.fec_type when !config.an_enable, and
use status.fec_type only on autoneg links? New firmware would give the
same answer, and on old firmware the wrong answer would be limited to
autoneg links.

> +	fec->fec =
> +		ionic_fec_type_to_ethtool(READ_ONCE(port_info->config.fec_type));
>  
>  	return 0;
>  }