Re: [PATCH net-next v3 2/5] ionic: report the configured FEC mode in ethtool_fecparam.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 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