RE: [EXT] Re: [net-next PATCH 3/7] octeontx2-pf: ethtool fec mode support

Hi Jakub,

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Saturday, January 23, 2021 10:00 AM
To: Hariprasad Kelam <redacted>
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
davem@davemloft.net; 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]; Christina Jacob
[off-list ref]
Subject: [EXT] Re: [net-next PATCH 3/7] octeontx2-pf: ethtool fec mode
support

External Email

----------------------------------------------------------------------
On Thu, 21 Jan 2021 13:23:25 +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

Signed-off-by: Christina Jacob <redacted>
Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
Signed-off-by: Hariprasad Kelam <redacted>
---
 .../ethernet/marvell/octeontx2/nic/otx2_common.c   |  23 +++
 .../ethernet/marvell/octeontx2/nic/otx2_common.h   |   6 +
 .../ethernet/marvell/octeontx2/nic/otx2_ethtool.c  | 174

++++++++++++++++++++-

quoted

 .../net/ethernet/marvell/octeontx2/nic/otx2_pf.c   |   3 +
 4 files changed, 204 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index bdfa2e2..d09119b 100644

--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

@@ -60,6 +60,22 @@ void otx2_update_lmac_stats(struct otx2_nic *pfvf)
 	mutex_unlock(&pfvf->mbox.lock);
 }

+void otx2_update_lmac_fec_stats(struct otx2_nic *pfvf) {
+	struct msg_req *req;
+
+	if (!netif_running(pfvf->netdev))
+		return;
+	mutex_lock(&pfvf->mbox.lock);
+	req = otx2_mbox_alloc_msg_cgx_fec_stats(&pfvf->mbox);
+	if (!req) {
+		mutex_unlock(&pfvf->mbox.lock);
+		return;
+	}
+	otx2_sync_mbox_msg(&pfvf->mbox);
+	mutex_unlock(&pfvf->mbox.lock);
+}
+
 int otx2_update_rq_stats(struct otx2_nic *pfvf, int qidx)  {
 	struct otx2_rcv_queue *rq = &pfvf->qset.rq[qidx]; @@ -1491,6
+1507,13 @@ void mbox_handler_cgx_stats(struct otx2_nic *pfvf,
 		pfvf->hw.cgx_tx_stats[id] = rsp->tx_stats[id];  }

+void mbox_handler_cgx_fec_stats(struct otx2_nic *pfvf,
+				struct cgx_fec_stats_rsp *rsp)
+{
+		pfvf->hw.cgx_fec_corr_blks += rsp->fec_corr_blks;
+		pfvf->hw.cgx_fec_uncorr_blks += rsp->fec_uncorr_blks;

double indented

I will fix this in V2.

quoted

+}
+
 void mbox_handler_nix_txsch_alloc(struct otx2_nic *pf,
 				  struct nix_txsch_alloc_rsp *rsp)  {

quoted

@@ -183,12 +210,42 @@ static void otx2_get_ethtool_stats(struct

net_device *netdev,

quoted

 	for (stat = 0; stat < CGX_TX_STATS_COUNT; stat++)
 		*(data++) = pfvf->hw.cgx_tx_stats[stat];
 	*(data++) = pfvf->reset_count;
+
+	if (pfvf->linfo.fec == OTX2_FEC_NONE)
+		return;

Don't hide the stats based on configuration.
Getting number of stats and requesting them are two different syscalls.

Agreed .  While getting number of stats also  , check is there to ensure interface 
Has valid fec mode. 
	if (pfvf->linfo.fec) {
                sprintf(data, "Fec Corrected Errors: ");
These checks ensures number of stats and request them are in sync.

quoted

+	fec_corr_blks = pfvf->hw.cgx_fec_corr_blks;
+	fec_uncorr_blks = pfvf->hw.cgx_fec_uncorr_blks;
+
+	rsp = otx2_get_fwdata(pfvf);
+	if (!IS_ERR(rsp) && rsp->fwdata.phy.misc.has_fec_stats &&
+	    !otx2_get_phy_fec_stats(pfvf)) {
+		/* Fetch fwdata again because it's been recently populated

with

quoted

+		 * latest PHY FEC stats.
+		 */
+		rsp = otx2_get_fwdata(pfvf);
+		if (!IS_ERR(rsp)) {
+			struct fec_stats_s *p = &rsp->fwdata.phy.fec_stats;
+
+			if (pfvf->linfo.fec == OTX2_FEC_BASER) {
+				fec_corr_blks   = p->brfec_corr_blks;
+				fec_uncorr_blks = p->brfec_uncorr_blks;
+			} else {
+				fec_corr_blks   = p->rsfec_corr_cws;
+				fec_uncorr_blks = p->rsfec_uncorr_cws;
+			}
+		}
+	}
+
+	*(data++) = fec_corr_blks;
+	*(data++) = fec_uncorr_blks;
 }

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;
+	int fec[] = {

const

Will fix in V2.
 

quoted

+		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)
+		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) {
+	case ETHTOOL_FEC_OFF:
+		fec = OTX2_FEC_NONE;
+		break;
+	case ETHTOOL_FEC_RS:
+		fec = OTX2_FEC_RS;
+		break;
+	case ETHTOOL_FEC_BASER:
+		fec = OTX2_FEC_BASER;
+		break;
+	default:
+		fec = OTX2_FEC_NONE;

IIRC fecparam->fec is a bitmask, you can't assume other than one bit set is
NONE.

Agreed . Will fix in V2.

quoted

+		break;
+	}
+
+	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 = -EAGAIN;

Why -EAGAIN? When does msg allocation fail?

Agreed . Message allocation fails incase No memory. Will fix this in V2.

quoted

+		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;
+		pfvf->hw.cgx_fec_corr_blks = 0;
+		pfvf->hw.cgx_fec_uncorr_blks = 0;

Are you clearing stats every time FEC changes?

Netdev driver gets fec stats from firmware. When user changes fec mode , counters local
To netdev driver becomes stale. So clearing fec counters.

quoted

+

spurious new line

Will fix in v2.

quoted

+	} else {
+		err = rsp->fec;
+	}
+
+end:	mutex_unlock(&mbox->lock);

label on a separate line

Will fix in v2.

Thanks,
Hariprasad k

quoted

+	return err;
+}