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

[Jakub Kicinski <kuba@kernel.org>]

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;
>  }