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

Hi Jakub,

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, February 3, 2021 6:42 AM
To: Hariprasad Kelam <redacted>
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
davem@davemloft.net; willemdebruijn.kernel@gmail.com;
andrew@lunn.ch; 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]
Subject: [EXT] Re: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode
support

On Mon, 1 Feb 2021 10:54:40 +0530 Hariprasad Kelam wrote:

quoted

From: Christina Jacob <redacted>

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

quoted

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

Agreed. Will fix this in next version.

quoted

+	}
 }

quoted

+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

quoted

+	if (pfvf->linfo.fec < FEC_MAX_INDEX)

This should be <

Agreed . Current code miss the "ETHTOOL_FEC_BASER | ETHTOOL_FEC_RS" condition,  will fix this in next version.

quoted

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

quoted

+	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

*/

quoted

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

Thanks for pointing this.  Cross checked code also  _NONE is output is only parameter.
Will fix in next version.

quoted

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

Thanks for pointing this. Will fix in next version.

Thanks,
Hariprasad k 

quoted

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