Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: implement egress tbf qdisc for 6393x family

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

Hello Vladimir, thanks for the feedback,

On 6/9/23 16:57, Vladimir Oltean wrote:
> On Fri, Jun 09, 2023 at 04:18:12PM +0200, alexis.lothore@bootlin.com wrote:
>> +int mv88e6393x_tbf_add(struct mv88e6xxx_chip *chip, int port,
>> +		       struct tc_tbf_qopt_offload_replace_params *replace_params)
>> +{
>> +	int rate_kbps = DIV_ROUND_UP(replace_params->rate.rate_bytes_ps * 8, 1000);
>> +	int overhead = DIV_ROUND_UP(replace_params->rate.overhead, 4);
>> +	int rate_step, decrement_rate, err;
>> +	u16 val;
>> +
>> +	if (rate_kbps < MV88E6393X_PORT_EGRESS_RATE_MIN_KBPS ||
>> +	    rate_kbps >= MV88E6393X_PORT_EGRESS_RATE_MAX_KBPS)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (replace_params->rate.overhead > MV88E6393X_PORT_EGRESS_MAX_OVERHEAD)
>> +		return -EOPNOTSUPP;
> 
> How does tbf react to the driver returning -EOPNOTSUPP? I see tbf_offload_change()
> returns void and doesn't check the ndo_setup_tc() return code.
> 
> Should we resolve that so that the error code is propagated to the user?

Indeed, checking some other TC Qdisc, some reports ndo_setup_tc errors (htb,
taprio, ...) to caller, some others do not (red, ets...). I can give it a try
and see the impact
> 
> Also, it would be nice to extend struct tc_tbf_qopt_offload with a
> netlink extack, for the driver to state exactly the reason for the
> offload failure.

ACK, I will add the extack struct

> 
> Not sure if EOPNOTSUPP is the return code to use here for range checks,
> rather than ERANGE.

I was not sure about proper error codes on all those checks. Since all those
errors are about what hardware can handle/can not handle, I felt like EOPNOTSUPP
was the most relevant one. But indeed it may make more sense for user to get
ERANGE here, I will update accordingly
>> +
>> +	/* Switch supports only max rate configuration. There is no
>> +	 * configurable burst/max size nor latency.
>> +	 * Formula defining registers value is:
>> +	 * EgressRate = 8 * EgressDec / (16ns * desired Rate)
>> +	 * EgressRate is a set of fixed values depending of targeted range
>> +	 */
>> +	if (rate_kbps < MBPS_TO_KBPS(1)) {
>> +		decrement_rate = rate_kbps / 64;
>> +		rate_step = MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_64_KBPS;
>> +	} else if (rate_kbps < MBPS_TO_KBPS(100)) {
>> +		decrement_rate = rate_kbps / MBPS_TO_KBPS(1);
>> +		rate_step = MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_1_MBPS;
>> +	} else if (rate_kbps < GBPS_TO_KBPS(1)) {
>> +		decrement_rate = rate_kbps / MBPS_TO_KBPS(10);
>> +		rate_step = MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_10_MBPS;
>> +	} else {
>> +		decrement_rate = rate_kbps / MBPS_TO_KBPS(100);
>> +		rate_step = MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_100_MBPS;
>> +	}
>> +
>> +	dev_dbg(chip->dev, "p%d: adding egress tbf qdisc with %dkbps rate",
>> +		port, rate_kbps);
>> +	val = decrement_rate;
>> +	val |= (overhead << MV88E6XXX_PORT_EGRESS_RATE_CTL1_FRAME_OVERHEAD_SHIFT);
>> +	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_EGRESS_RATE_CTL1,
>> +				   val);
>> +	if (err)
>> +		return err;
>> +
>> +	val = rate_step;
>> +	/* Configure mode to bits per second mode, on layer 1 */
>> +	val |= MV88E6XXX_PORT_EGRESS_RATE_CTL2_COUNT_L1_BYTES;
>> +	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_EGRESS_RATE_CTL2,
>> +				   val);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +
>> +int mv88e6393x_tbf_del(struct mv88e6xxx_chip *chip, int port)
>> +{
>> +	int err;
>> +
>> +	dev_dbg(chip->dev, "p%d: removing tbf qdisc", port);
>> +	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_EGRESS_RATE_CTL2,
>> +				   0x0000);
>> +	if (err)
>> +		return err;
>> +	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_EGRESS_RATE_CTL1,
>> +				    0x0001);
> 
> I guess this should return void and proceed on errors, rather than exit early.
> Maybe shout out loud that things went wrong.

ACK

> 
>> +}
>> +
>> +static int mv88e6393x_tc_setup_qdisc_tbf(struct mv88e6xxx_chip *chip, int port,
>> +					 struct tc_tbf_qopt_offload *qopt)
>> +{
>> +	/* Device only supports per-port egress rate limiting */
>> +	if (qopt->parent != TC_H_ROOT)
>> +		return -EOPNOTSUPP;
>> +
>> +	switch (qopt->command) {
>> +	case TC_TBF_REPLACE:
>> +		return mv88e6393x_tbf_add(chip, port, &qopt->replace_params);
>> +	case TC_TBF_DESTROY:
>> +		return mv88e6393x_tbf_del(chip, port);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com