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

[Wei Fang]

> -----Original Message-----
> From: Simon Horman <simon.horman@xxxxxxxxxxxx>
> Sent: 2023年2月10日 0:14
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: Shenwei Wang <shenwei.wang@xxxxxxx>; Clark Wang
> <xiaoning.wang@xxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] net: fec: add CBS offload support
> 
> On Thu, Feb 09, 2023 at 05:24:22PM +0800, wei.fang@xxxxxxx wrote:
> > From: Wei Fang <wei.fang@xxxxxxx>
> >
> > +
> > +	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.