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

[Luke Howard]

Hi Cedric,

> On 29 May 2026, at 10:32 pm, Cedric Jehasse <cedric.jehasse@xxxxxxxxx> wrote:
> 
> On Fri, May 29, 2026 at 09:48:33AM +1000, Luke Howard wrote:
>>> +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;

These could be initialised at the top of the function, might save some lines…

>>> + 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.
>> 
> That's not the same logic.
> If one of the two function pointer is NULL, the other can still be executed
> (Maybe not that likely as that's probably a mistake in the driver)

True, but agree also that it shouldn’t happen (would be a driver bug).

>>> + 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.
> 
> Is that a personal preference, or a guideline I’ve missed?

You’re right, I had just assumed it was because it’s the case in the rest of the driver (97% adherence according to Claude :))

BTW, I have a (as yet untested) MQPRIO patch rebased on your v4 CBS patch. In DCB mode, TCs represent AVB classes (as my previous patch did). In CHANNEL mode, they represent queues, so you can flexibly map priorities to queues, with the additional advantage that these mappings can be per-port on the 6390.

It’s potentially a questionable overloading of the MQPRIO modes but, it’s a starting point for discussion, and it avoids needing out-of-band configuration (e.g. command line, devlink, etc, although I am now using devlink to set the AVB mode rather than the device tree).

Cheers,
Luke