Re: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-02-03 01:13:04
Also in:
lkml
On Mon, 1 Feb 2021 10:54:40 +0530 Hariprasad Kelam wrote:
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 hunk ↗ jump to hunk
+ 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.
+ } }
quoted hunk ↗ jump to hunk
+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];
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ } else { + err = rsp->fec; + }