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

[Andrew Lunn <andrew@lunn.ch>]

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

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

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

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

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

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

> + *	  To avoid breaking static IP MDB entries, only multicast addresses
> + *	  with OUI prefix of 91:e0:ff (IEEE 1722 Annex D) will have the AVB
> + *	  flag set on their ATU entry.

This probably answers my question bellow.

> +/* The MAAP address range is 91:E0:F0:00:00:00 thru 91:E0:F0:00:FF:FF
> + * (IEEE 1722 Annex D)
> + */
> +static const u8 eth_maap_mcast_addr_base[ETH_ALEN] __aligned(2) = {
> +	0x91, 0xe0, 0xf0, 0x00, 0x00, 0x00
> +};
> +
> +static inline bool ether_addr_is_maap_mcast(const u8 *addr)
> +{
> +	u8 mask[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0x00, 0x00 };
> +
> +	return ether_addr_equal_masked(addr, eth_maap_mcast_addr_base, mask);
> +}

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