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

[Cedric Jehasse]

On Thu, May 28, 2026 at 1:52 AM Luke Howard <lukeh@xxxxxxxx> wrote:
> Would also be good to add support for the 6341, which as 4 queues but uses the same Qav multipliers as the 6390.
I'll have a look at the 6341

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

I think some of these were the result of running the kernel review
prompts through Claude. Other drivers are probably from before LLM
reviews were introduced.

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

I have a different opinion. The driver is in control, if these get out of sync
you're having bigger problems. Either something else is writing to the device,
or the hardware is changing state by itself which means it's unstable.

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

Eg. for the 6393 the documentation says the maximum supported rate is 4000Mbps.
It could be this comes from the rate register set to 0xffff,
which is 4193240Kbps, and then rounded to 4000Mbps in the docs.

The 6020 family has 4 queues, but only 2 queues support CBS. That's why there's
a seperate queue mask, even if CBS support wasn't added for 6020.

Cedric