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

[Andrew Lunn <andrew@lunn.ch>]

> > 
> >> + *
> >> + * @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.
 
Ah, sorry. I missed you did not start the documentation correctly, so
it is not even considered kerneldoc.

You need

/**

to start the block.

https://docs.kernel.org/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments

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

O.K, i've no real knowledge of AVB...

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

Having an open user space side helps get such an extra attribute
added.

Would the kernel bridge have any use of this? It is normal to add a
feature to the kernel, and then offload it to hardware. 

	Andrew