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

[Luke Howard]

Hi Cedric,

In anticipation of potentially rebasing my MQPRIO patch series upon this patch, I have some comments. (My MUA appears to have ruined the indentation, apologies.)

> Some of the chips supported by this driver have credit based shaper
> support. Support is added for the 6352, 6390 and 6393 families.

Would also be good to add support for the 6341, which as 4 queues but uses the same Qav multipliers as the 6390.

> + if (cbs->enable) {
> + if (cbs->idleslope <= 0 ||
> +    cbs->idleslope > qav->max_rate ||
> +    cbs->sendslope >= 0 || cbs->hicredit <= 0 ||
> +    cbs->hicredit > qav->hi_limit_mask)
> + return -ERANGE;

Other drivers (e.g. felix_vsc9959) round up and clamp, rather than validating. I would consider whether this validation is necessary at all, and/or whether it would suffice to omit the idleslope check and check only rate_mask. The idleslope and sendslope zero validation might be better placed in sch_cbs.c.

> + if (!cbs->enable) {
> + err = avb_ops->port_qav_write(chip, port, rate_reg, 0);

The convention is to provide a wrapper function, e.g. mv88e6xxx_port_qav_write(), returning -EOPNOTSUPP if the function pointer is NULL.

> + if (!(chip->ports[port].cbs_active_queues & ~queue_bit)) {
> + err = ops->port_set_scheduling_mode(chip, port, 0);
> + if (err)
> + goto unlock;
> + }
> + chip->ports[port].cbs_active_queues &= ~queue_bit;
> + goto unlock;
> + }
> +
> + hi_limit = cbs->hicredit & qav->hi_limit_mask;
> + err = avb_ops->port_qav_write(chip, port, hilimit_reg, hi_limit);
> + if (err)
> + goto unlock;
> +
> + err = avb_ops->port_qav_write(chip, port, rate_reg, rate);
> + if (err)
> + goto unlock;
> +
> + err = ops->port_set_scheduling_mode(chip, port,
> +    chip->info->num_tx_queues - 1);
> + if (err) {
> + avb_ops->port_qav_write(chip, port, rate_reg, 0);
> + goto unlock;
> + }
> + chip->ports[port].cbs_active_queues |= queue_bit;
> +
> +unlock:
> + mv88e6xxx_reg_unlock(chip);
> +
> + return err;

This may not matter in practice, but SMI errors will leave the register state in an inconsistent configuration. I would consider having multiple cleanup handlers that revert the state to that on function entry.

With respect to cbs_active_queues, my personal opinion is to not cache state both in the driver and registers where possible (to avoid the possibility they may get out of sync), but it is true that on a device with many queues, determining the CBS state dynamically would be expensive. I would be interested in others’ opinion here.

> +struct mv88e6xxx_qav_info {
> + u32 max_rate; /* in kbps */

Per above comment, perhaps consider whether max_rate is required. Also, queue_mask can be determined dynamically from chip->info->num_tx_queues, so can be elided.

Cheers,
Luke