Hi Jakub, > -----Original Message----- > From: Jakub Kicinski <k...@kernel.org> > Sent: Saturday, January 23, 2021 10:00 AM > To: Hariprasad Kelam <hke...@marvell.com> > Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org; > da...@davemloft.net; Sunil Kovvuri Goutham <sgout...@marvell.com>; > Linu Cherian <lcher...@marvell.com>; Geethasowjanya Akula > <gak...@marvell.com>; Jerin Jacob Kollanukkaran <jer...@marvell.com>; > Subbaraya Sundeep Bhatta <sbha...@marvell.com>; Christina Jacob > <cja...@marvell.com> > 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: > > From: Christina Jacob <cja...@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 > > > > Signed-off-by: Christina Jacob <cja...@marvell.com> > > Signed-off-by: Sunil Goutham <sgout...@marvell.com> > > Signed-off-by: Hariprasad Kelam <hke...@marvell.com> > > --- > > .../ethernet/marvell/octeontx2/nic/otx2_common.c | 23 +++ > > .../ethernet/marvell/octeontx2/nic/otx2_common.h | 6 + > > .../ethernet/marvell/octeontx2/nic/otx2_ethtool.c | 174 > ++++++++++++++++++++- > > .../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. > > +} > > + > > void mbox_handler_nix_txsch_alloc(struct otx2_nic *pf, > > struct nix_txsch_alloc_rsp *rsp) { > > > @@ -183,12 +210,42 @@ static void otx2_get_ethtool_stats(struct > net_device *netdev, > > 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.
> > + 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 > > + * 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; > > } > > > +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. > > + 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) > > + 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. > > > + 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. > > + 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. > > + > > spurious new line > Will fix in v2. > > + } else { > > + err = rsp->fec; > > + } > > + > > +end: mutex_unlock(&mbox->lock); > > label on a separate line > Will fix in v2. Thanks, Hariprasad k > > + return err; > > +}