Re: [RFC net-next 3/5] net: dsa: mv88e6xxx: MQPRIO support

[Luke Howard <lukeh@padl.com>]

> On 27 Sep 2025, at 16:12, Andrew Lunn <andrew@lunn.ch> wrote:
> 
>> +/* MQPRIO helpers */
>> +
>> +/* Set the AVB global policy limit registers. Caller must acquired register
>> + * lock.
> 
> No where else in this driver is this assumption about the register
> lock documented. If you forget it, the low level read/write functions
> will tell you. So i don't think it adds value.

Fixed.

> 
>> + *
>> + * @param chip Marvell switch chip instance
>> + * @param hilimit Maximum frame size allowed for AVB Class A frames
>> + *
>> + * @return 0 on success, or a negative error value otherwise
>> + */
> 
> kerneldoc wants a : after return.

Should also be for @param then? Seems fairly inconsistent on a brief survey of other drivers.

>> +static int mv88e6xxx_avb_set_hilimit(struct mv88e6xxx_chip *chip, u16 hilimit)
>> +{
>> + u16 data;
>> + int err;
>> +
>> + if (hilimit > MV88E6XXX_AVB_CFG_HI_LIMIT_MASK)
>> + return -EINVAL;
> 
> Does it make sense to check it against the MTU? Does it matter if it
> is bigger than the MTU?

I don’t think so; this is hicredit in tc-cbs(8).

>> +/* Set the AVB global policy OUI filter registers. Caller must acquire register
>> + * lock.
>> + *
>> + * @param chip Marvell switch chip instance
>> + * @param addr The AVB OUI to load
>> + *
>> + * @return 0 on success, or a negative error value otherwise
>> + */
>> +static int mv88e6xxx_avb_set_oui(struct mv88e6xxx_chip *chip,
>> + const unsigned char *addr)
> 
> Maybe be a bit more specific with the documentation of addr. You pass
> a 6 byte address, not a 3 byte OUI. So "The AVB OUI to load" is not
> quite correct. It is more like, "Use the OUI from this MAC address."
> I've not looked at the big picture, but i was woundering if it makes
> more sense to pass an actual OUI? But that is not something we tend to
> do in the kernel.

Passing in an addr lets us reuse eth_maap_mcast_addr_base. Agree the documentation could be clearer. Perhaps this extra restriction (requiring AVTP DAs in secure mode) is not necessary anyway.

>> +static inline u16 mv88e6352_avb_pri_map_to_reg(const struct mv88e6xxx_avb_priority_map map[])
>> +{
>> + return MV88E6352_AVB_CFG_AVB_HI_FPRI_SET(map[MV88E6XXX_AVB_TC_HI].fpri) |
>> + MV88E6352_AVB_CFG_AVB_HI_QPRI_SET(map[MV88E6XXX_AVB_TC_HI].qpri) |
>> + MV88E6352_AVB_CFG_AVB_LO_FPRI_SET(map[MV88E6XXX_AVB_TC_LO].fpri) |
>> + MV88E6352_AVB_CFG_AVB_LO_QPRI_SET(map[MV88E6XXX_AVB_TC_LO].qpri);
>> +}
>> +
>> +static int mv88e6352_qav_map_fpri_qpri(u8 fpri, u8 qpri, void *reg)
>> +{
>> + mv88e6352_g1_ieee_pri_set(fpri, qpri, (u16 *)reg);
>> + return 0;
> 
> Blank line before the return please.

Fixed.

>> + * - because the Netlink API has no way to distinguish between FDB/MDB
>> + *  entries managed by SRP from those that are not, the
>> + *  "marvell,mv88e6xxx-avb-mode" device tree property controls whether
>> + *  a FDB or MDB entry is required in order for AVB frames to egress.
> 
> We probably need to think about this. What about other devices which
> require this? Would it be better to extend the netlink API to pass
> some sort of owner? If i remember correctly, routes passed by netlink
> can indicate which daemon is responsible for it, quagga, zebra, bgp
> etc.

An additional flag to Netlink when adding a FDB or MDB entry would certainly be cleaner. I’m not sure what other devices do, I suspect some support similar functionality but do not have driver support, because most SRP implementations have managed the switch directly. (Mine [1] uses standard kernel interfaces.)

> Since these are part of a standard, i assume other drivers will need
> it as well? These should probably be somewhere common.

Yes, can put elsewhere. Or, if we have a suitable Netlink interface, they can be removed, as we won’t need heuristics to determine whether a MDB entry is for AVB. (This also likely takes care of mv88e6xxx_avb_set_oui.)

[1] github.com/PADL/OpenSRP