Re: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 1 Feb 2021 10:54:40 +0530 Hariprasad Kelam wrote:
> From: Christina Jacob <cjacob@marvell.com>
> 
> Add ethtool support to configure fec modes baser/rs and
> support to fecth FEC stats from CGX as well PHY.
> 
> Configure fec mode
> 	- ethtool --set-fec eth0 encoding rs/baser/off/auto
> Query fec mode
> 	- ethtool --show-fec eth0

> +	if (pfvf->linfo.fec) {
> +		sprintf(data, "Fec Corrected Errors: ");
> +		data += ETH_GSTRING_LEN;
> +		sprintf(data, "Fec Uncorrected Errors: ");
> +		data += ETH_GSTRING_LEN;

Once again, you can't dynamically hide stats. ethtool makes 3 separate
system calls - to get the number of stats, get the names, and get the
values. If someone changes the FEC config in between those user space
dumping stats will get confused.

> +	}
>  }

> +static int otx2_get_fecparam(struct net_device *netdev,
> +			     struct ethtool_fecparam *fecparam)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(netdev);
> +	struct cgx_fw_data *rsp;
> +	const int fec[] = {
> +		ETHTOOL_FEC_OFF,
> +		ETHTOOL_FEC_BASER,
> +		ETHTOOL_FEC_RS,
> +		ETHTOOL_FEC_BASER | ETHTOOL_FEC_RS};
> +#define FEC_MAX_INDEX 3
> +	if (pfvf->linfo.fec < FEC_MAX_INDEX)

This should be <

> +		fecparam->active_fec = fec[pfvf->linfo.fec];


> +	rsp = otx2_get_fwdata(pfvf);
> +	if (IS_ERR(rsp))
> +		return PTR_ERR(rsp);
> +
> +	if (rsp->fwdata.supported_fec <= FEC_MAX_INDEX) {
> +		if (!rsp->fwdata.supported_fec)
> +			fecparam->fec = ETHTOOL_FEC_NONE;
> +		else
> +			fecparam->fec = fec[rsp->fwdata.supported_fec];
> +	}
> +	return 0;
> +}
> +
> +static int otx2_set_fecparam(struct net_device *netdev,
> +			     struct ethtool_fecparam *fecparam)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(netdev);
> +	struct mbox *mbox = &pfvf->mbox;
> +	struct fec_mode *req, *rsp;
> +	int err = 0, fec = 0;
> +
> +	switch (fecparam->fec) {
> +	/* Firmware does not support AUTO mode consider it as FEC_NONE */
> +	case ETHTOOL_FEC_OFF:
> +	case ETHTOOL_FEC_AUTO:
> +	case ETHTOOL_FEC_NONE:

I _think_ NONE is for drivers to report that they don't support FEC
settings. It's an output only parameter. On input OFF should be used.

> +		fec = OTX2_FEC_NONE;
> +		break;
> +	case ETHTOOL_FEC_RS:
> +		fec = OTX2_FEC_RS;
> +		break;
> +	case ETHTOOL_FEC_BASER:
> +		fec = OTX2_FEC_BASER;
> +		break;
> +	default:
> +		netdev_warn(pfvf->netdev, "Unsupported FEC mode: %d",
> +			    fecparam->fec);
> +		return -EINVAL;
> +	}
> +
> +	if (fec == pfvf->linfo.fec)
> +		return 0;
> +
> +	mutex_lock(&mbox->lock);
> +	req = otx2_mbox_alloc_msg_cgx_set_fec_param(&pfvf->mbox);
> +	if (!req) {
> +		err = -ENOMEM;
> +		goto end;
> +	}
> +	req->fec = fec;
> +	err = otx2_sync_mbox_msg(&pfvf->mbox);
> +	if (err)
> +		goto end;
> +
> +	rsp = (struct fec_mode *)otx2_mbox_get_rsp(&pfvf->mbox.mbox,
> +						   0, &req->hdr);
> +	if (rsp->fec >= 0) {
> +		pfvf->linfo.fec = rsp->fec;
> +		/* clear stale counters */
> +		pfvf->hw.cgx_fec_corr_blks = 0;
> +		pfvf->hw.cgx_fec_uncorr_blks = 0;

Stats are supposed to be cumulative. Don't reset the stats just because
someone changed the FEC mode. You can miss errors this way.

> +	} else {
> +		err = rsp->fec;
> +	}