lkml.org 
[lkml]   [2021]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support
Date
Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, February 3, 2021 6:42 AM
> To: Hariprasad Kelam <hkelam@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> davem@davemloft.net; willemdebruijn.kernel@gmail.com;
> andrew@lunn.ch; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Linu
> Cherian <lcherian@marvell.com>; Geethasowjanya Akula
> <gakula@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
> 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:
> > 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.
>
Agreed. Will fix this in next version.
> > + }
> > }
>
> > +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 <
Agreed . Current code miss the "ETHTOOL_FEC_BASER | ETHTOOL_FEC_RS" condition, will fix this in next version.
>
> > + 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.
>
Thanks for pointing this. Cross checked code also _NONE is output is only parameter.
Will fix in next version.

> > + 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
> > + } else {
> > + err = rsp->fec;
> > + }

\
 
 \ /
  Last update: 2021-02-04 16:38    [W:0.043 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site