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

[Vladimir Oltean]

On Fri, Jun 09, 2023 at 04:18:12PM +0200, alexis.lothore@xxxxxxxxxxx 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?

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.

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

> +
> +	/* 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.

> +}
> +
> +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;
> +}