Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Hariprasad Kelam <hkelam@marvell.com>
To:     Jakub Kicinski <kuba@kernel.org>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "willemdebruijn.kernel@gmail.com" <willemdebruijn.kernel@gmail.com>,
        "andrew@lunn.ch" <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: Re: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support
Thread-Topic: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support
Thread-Index: Adb6+wOmcyYaYYOEQTO7EJhiRx3HZg==
Date:   Thu, 4 Feb 2021 15:33:48 +0000
Message-ID: <MWHPR18MB14214E66C51B1B25725BBF0DDEB39@MWHPR18MB1421.namprd18.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ac3841b6-48d9-429e-c157-08d8c9224478
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Feb 2021 15:33:48.5451
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: w6/5BrrX6EEnFooTWR7KOhYC0sfml4Z6tWUOFUN/taNPOwouvuBv2NnO/yIKjDFoNgmp4SLfHYT22VepsIlSHw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR18MB0926
X-OriginatorOrg: marvell.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.737
 definitions=2021-02-04_08:2021-02-04,2021-02-04 signatures=0
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>=20
> 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
>=20
> > +	if (pfvf->linfo.fec) {
> > +		sprintf(data, "Fec Corrected Errors: ");
> > +		data +=3D ETH_GSTRING_LEN;
> > +		sprintf(data, "Fec Uncorrected Errors: ");
> > +		data +=3D ETH_GSTRING_LEN;
>=20
> 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 val=
ues. If
> someone changes the FEC config in between those user space dumping stats
> will get confused.
>=20
Agreed. Will fix this in next version.
> > +	}
> >  }
>=20
> > +static int otx2_get_fecparam(struct net_device *netdev,
> > +			     struct ethtool_fecparam *fecparam) {
> > +	struct otx2_nic *pfvf =3D netdev_priv(netdev);
> > +	struct cgx_fw_data *rsp;
> > +	const int fec[] =3D {
> > +		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)
>=20
> This should be <
Agreed . Current code miss the "ETHTOOL_FEC_BASER | ETHTOOL_FEC_RS" conditi=
on,  will fix this in next version.
>=20
> > +		fecparam->active_fec =3D fec[pfvf->linfo.fec];
>=20
>=20
> > +	rsp =3D otx2_get_fwdata(pfvf);
> > +	if (IS_ERR(rsp))
> > +		return PTR_ERR(rsp);
> > +
> > +	if (rsp->fwdata.supported_fec <=3D FEC_MAX_INDEX) {
> > +		if (!rsp->fwdata.supported_fec)
> > +			fecparam->fec =3D ETHTOOL_FEC_NONE;
> > +		else
> > +			fecparam->fec =3D fec[rsp->fwdata.supported_fec];
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int otx2_set_fecparam(struct net_device *netdev,
> > +			     struct ethtool_fecparam *fecparam) {
> > +	struct otx2_nic *pfvf =3D netdev_priv(netdev);
> > +	struct mbox *mbox =3D &pfvf->mbox;
> > +	struct fec_mode *req, *rsp;
> > +	int err =3D 0, fec =3D 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:
>=20
> I _think_ NONE is for drivers to report that they don't support FEC setti=
ngs.
> It's an output only parameter. On input OFF should be used.
>=20
Thanks for pointing this.  Cross checked code also  _NONE is output is only=
 parameter.
Will fix in next version.

> > +		fec =3D OTX2_FEC_NONE;
> > +		break;
> > +	case ETHTOOL_FEC_RS:
> > +		fec =3D OTX2_FEC_RS;
> > +		break;
> > +	case ETHTOOL_FEC_BASER:
> > +		fec =3D OTX2_FEC_BASER;
> > +		break;
> > +	default:
> > +		netdev_warn(pfvf->netdev, "Unsupported FEC mode: %d",
> > +			    fecparam->fec);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (fec =3D=3D pfvf->linfo.fec)
> > +		return 0;
> > +
> > +	mutex_lock(&mbox->lock);
> > +	req =3D otx2_mbox_alloc_msg_cgx_set_fec_param(&pfvf->mbox);
> > +	if (!req) {
> > +		err =3D -ENOMEM;
> > +		goto end;
> > +	}
> > +	req->fec =3D fec;
> > +	err =3D otx2_sync_mbox_msg(&pfvf->mbox);
> > +	if (err)
> > +		goto end;
> > +
> > +	rsp =3D (struct fec_mode *)otx2_mbox_get_rsp(&pfvf->mbox.mbox,
> > +						   0, &req->hdr);
> > +	if (rsp->fec >=3D 0) {
> > +		pfvf->linfo.fec =3D rsp->fec;
> > +		/* clear stale counters */
> > +		pfvf->hw.cgx_fec_corr_blks =3D 0;
> > +		pfvf->hw.cgx_fec_uncorr_blks =3D 0;
>=20
> 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=20
> > +	} else {
> > +		err =3D rsp->fec;
> > +	}