netdev - RE: [PATCH net-next] net: fec: add CBS offload support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB9PR04MB8106EA847010C0A6CD5EB23588DE9@DB9PR04MB8106.eurprd04.prod.outlook.com>
Date:   Fri, 10 Feb 2023 07:24:03 +0000
From:   Wei Fang <wei.fang@....com>
To:     Simon Horman <simon.horman@...igine.com>
CC:     Shenwei Wang <shenwei.wang@....com>,
        Clark Wang <xiaoning.wang@....com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next] net: fec: add CBS offload support

> -----Original Message-----
> From: Simon Horman <simon.horman@...igine.com>
> Sent: 2023年2月10日 0:14
> To: Wei Fang <wei.fang@....com>
> Cc: Shenwei Wang <shenwei.wang@....com>; Clark Wang
> <xiaoning.wang@....com>; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; dl-linux-imx
> <linux-imx@....com>; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH net-next] net: fec: add CBS offload support
> 
> On Thu, Feb 09, 2023 at 05:24:22PM +0800, wei.fang@....com wrote:
> > From: Wei Fang <wei.fang@....com>
> >
> > +
> > +	if (idle_slope >= 128) {
> > +		/* For values equal to or greater than 128, idle_slope = 128 * m,
> > +		 * where m = 1, 2, 3, ...12. Here we use the rounding method.
> > +		 */
> 
> 	Perhaps the following would be clearer?
> 
> 	 For values greater than or equal to 128,
> 	 idle_slope is rounded to the nearest multiple of 128.
> 
> > +		quotient = idle_slope / 128;
> > +		if (idle_slope >= quotient * 128 + 64)
> > +			idle_slope = 128 * (quotient + 1);
> > +		else
> > +			idle_slope = 128 * quotient;
> 
> 	Maybe there is a helper that does this, but if
> 	not, perhaps:
> 
> 	idle_slope = DIV_ROUND_CLOSEST(idle_slope, 128U) * 128U;
> 
> 
> > +
> > +		goto end;
> 
> Maybe return here
> 
> > +	}
> 
> Or an else here is nicer?
> 
> > +
> > +	/* For values less than 128, idle_slope = 2 ^ n, where
> 
> 	Perhaps the following would be clearer?
> 
> 	 For values less than 128, idle_slope is rounded
> 	 to the nearest power of 2.
> 
> > +	 * n = 0, 1, 2, ...6. Here we use the rounding method.
> 
>          n is 7 for input idle_slope around 128 (2^7)
> 
> > +	 * So the minimum of idle_slope is 1.
> > +	 */
> > +	msb = fls(idle_slope);
> > +
> > +	if (msb == 0 || msb == 1) {
> > +		idle_slope = 1;
> > +		goto end;
> > +	}
> 
> nit: maybe this is nicer
> 
> 	if (msb <= 1)
> 		return 1;
> 
> > +
> > +	msb -= 1;
> > +	if (idle_slope >= (1 << msb) + (1 << (msb - 1)))
> > +		idle_slope = 1 << (msb + 1);
> > +	else
> > +		idle_slope = 1 << msb;
> 
> 	In the same vein as the suggestion for the >= 128 case, perhaps:
> 
> 	u32 d;
> 
> 	d = BIT(fls(idle_slope));
> 	idle_slope = DIV_ROUND_CLOSEST(idle_slope, d) * d;
> 
> > +
> > +end:
> > +	return idle_slope;
> > +}
> > +
> > +static int fec_enet_setup_tc_cbs(struct net_device *ndev, void
> > +*type_data) {
> > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	struct tc_cbs_qopt_offload *cbs = type_data;
> > +	int queue =  cbs->queue;
> 
> nit: extra space after '='
> 
> > +	u32 speed = fep->speed;
> > +	u32 val, idle_slope;
> > +	u8 bw;
> > +
> > +	if (!(fep->quirks & FEC_QUIRK_HAS_AVB))
> > +		return -EOPNOTSUPP;
> > +
> > +	/* Queue 1 for Class A, Queue 2 for Class B, so the ENET must has
> 
> nit: s/has/have/
> 
> > +	 * three queues.
> > +	 */
> > +	if (fep->num_tx_queues != FEC_ENET_MAX_TX_QS)
> > +		return -EOPNOTSUPP;
> > +
> > +	/* Queue 0 is not AVB capable */
> > +	if (queue <= 0 || queue >= fep->num_tx_queues)
> > +		return -EINVAL;
> > +
> > +	val = readl(fep->hwp + FEC_QOS_SCHEME);
> > +	val &= ~FEC_QOS_TX_SHEME_MASK;
> > +	if (!cbs->enable) {
> > +		val |= ROUND_ROBIN_SCHEME;
> > +		writel(val, fep->hwp + FEC_QOS_SCHEME);
> > +
> > +		return 0;
> > +	}
> > +
> > +	val |= CREDIT_BASED_SCHEME;
> > +	writel(val, fep->hwp + FEC_QOS_SCHEME);
> > +
> > +	/* cbs->idleslope is in kilobits per second. speed is the port rate
> > +	 * in megabits per second. So bandwidth ratio bw = (idleslope /
> > +	 * (speed * 1000)) * 100, the unit is percentage.
> > +	 */
> 
> suggestion:
> 
> 	/* cbs->idleslope is in kilobits per second.
> 	 * Speed is the port rate in megabits per second.
> 	 * So bandwidth the ratio, bw, is (idleslope / (speed * 1000)) * 100.
> 	 * The unit of bw is a percentage.
> 	 */
> 
> > +	bw = cbs->idleslope / (speed * 10UL);
> > +	/* bw% can not >= 100% */
> > +	if (bw >= 100)
> > +		return -EINVAL;
> 
> nit: maybe the above calculation and check fits better inside
>      fec_enet_get_idle_slope()
> 

Your suggestions are very helpful, I'll amend the patch in v2.
Thanks a lot.