Re: [PATCH net-next 1/8] net: dsa: add tag driver for LAN9645X

[Andrew Lunn]

> +#define LAN9645X_VALIDATE_FIELD(_fld, _fld_sz)				\
> +do {									\
> +	BUILD_BUG_ON_MSG((_fld_sz) > 32, "IFH field size wider than 32.");\
> +	BUILD_BUG_ON_MSG((_fld_sz) == 0, "IFH field size of 0.");	\
> +	BUILD_BUG_ON_MSG((_fld) + (_fld_sz) > LAN9645X_IFH_BITS,	\
> +			 "IFH field overflows IFH");			\
> +} while (0)
> +
> +#define LAN9645X_IFH_GET(_ifh, _fld) \
> +({ \
> +	LAN9645X_VALIDATE_FIELD(_fld, _fld##_SZ);\
> +	lan9645x_ifh_get((_ifh), (_fld), _fld##_SZ); \
> +})
> +
> +#define LAN9645X_IFH_SET(_ifh, _fld, _val) \
> +({ \
> +	LAN9645X_VALIDATE_FIELD(_fld, _fld##_SZ);\
> +	lan9645x_ifh_set((_ifh), (_val), (_fld), _fld##_SZ); \
> +})

If you change the BUILD_BUG_ON_MSG() to static_assert(), you can do
the checks in global scope, rather than in a function call.  You can
then call lan9645x_ifh_set() directly, without the macro.

> +static inline void lan9645x_ifh_set(u8 *ifh, u32 val, size_t pos, size_t length)

These functions are big enough i would place them into the .c file.
Then, normally, i would say, please don't use inline in a C file. But
here we are in the fast path. Have you tried this with and without the
inline? How does it change the object size and performance?

> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 5ed8c704636d..8592cccde7ff 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -211,4 +211,14 @@ config NET_DSA_TAG_YT921X
>  	  Say Y or M if you want to enable support for tagging frames for
>  	  Motorcomm YT921x switches.
>  
> +config NET_DSA_TAG_LAN9645X
> +	tristate "Tag driver for Lan9645x switches"
> +	help
> +	  Say Y or M if you want to enable NPI tagging for the Lan9645x switches.
> +	  In this mode, the frames over the Ethernet CPU port are prepended with
> +	  a hardware-defined injection/extraction frame header.
> +	  On injection a 28 byte internal frame header (IFH) is used. On
> +	  extraction a 16 byte prefix is prepended before the internal frame
> +	  header. This prefix starts with a broadcast MAC, to ease passage
> +	  through the host side RX filter.

The sorting in DSA is a bit hit and miss. The Kconfig file is however
sorted. Please insert before the Lantiq tag driver.

>  endif
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index bf7247759a64..dddcd85c81ce 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
>  obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
>  obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
>  obj-$(CONFIG_NET_DSA_TAG_YT921X) += tag_yt921x.o
> +obj-$(CONFIG_NET_DSA_TAG_LAN9645X) += tag_lan9645x.o

Also sorted.

	Andrew