Re: [PATCH net-next v4 2/2] net: dsa: mv88e6xxx: add support for credit based shaper

[Luke Howard]

Hi Cedric,

Looking good!

> +static int mv88e6xxx_setup_tc_cbs(struct dsa_switch *ds, int port,
> +  struct tc_cbs_qopt_offload *cbs)
> +{
> + const struct mv88e6xxx_avb_ops *avb_ops;
> + struct mv88e6xxx_chip *chip = ds->priv;
> + const struct mv88e6xxx_qav_info *qav;
> + const struct mv88e6xxx_ops *ops;
> + int hilimit_reg;
> + int rate_reg;
> + u8 queue_bit;
> + u16 hi_limit;
> + u16 rate = 0;
> + int err;
> +
> + ops = chip->info->ops;
> + avb_ops = ops->avb_ops;
> + qav = chip->info->qav;
> +
> + if (!qav || !avb_ops || !avb_ops->port_qav_write ||
> +    !ops->port_set_scheduling_mode)
> + return -EOPNOTSUPP;

Only the first check should be necessary because the wrapper functions return -EOPNOTSUPP in the case of a NULL function pointer.

> + if (!(chip->ports[port].cbs_active_queues & ~queue_bit)) {
> + err = mv88e6xxx_port_set_scheduling_mode(chip, port, 0);
> + if (err)
> + goto unlock;
> + }

Missing a newline here after closing brace.

> + chip->ports[port].cbs_active_queues &= ~queue_bit;
> + goto unlock;
> + }

The control flow might be clearer were goto unlock only taken in case of error. i.e. wrap the CBS enable case in an else block.

> +
> + hi_limit = cbs->hicredit & qav->hi_limit_mask;
> + err = mv88e6xxx_port_qav_write(chip, port, hilimit_reg, hi_limit);
> + if (err)
> + goto unlock;
> +
> + err = mv88e6xxx_port_qav_write(chip, port, rate_reg, rate);
> + if (err)
> + goto unlock;
> +
> + err = mv88e6xxx_port_set_scheduling_mode(chip, port,
> + chip->info->num_tx_queues - 1);

For the 6020 (which you brought to my attention as only supporting CBS on half its four queues), should strict priority be enabled only on the CBS queues or on all queues? (I don’t know the answer, just asking.)

Cheers,
Luke