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

[Cedric Jehasse]

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;
> > +
> > + 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)

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

> 
> > + 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.
> 
I don't think it makes that much difference.
You've mentioned the felix driver before, that has similar flow :) (with a
return in the disable case isof goto)

> > +
> > + 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.)
> 
I hadn't thought about that. The 6020 has the same strict priority settings
registers as the other devices, it doesn't need special handling.