On Fri, Jun 09, 2023 at 04:18:12PM +0200, [email protected] 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;
> +}
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, [email protected] 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