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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Tue, 2026-03-03 at 15:13 +0100, Andrew Lunn wrote:
> 
> > +#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.
> 

Do you mean global scope static_assert() for all fields, and then remove
the SET/GET macros? My idea here was to get the validation for the fields
used, provided the macros are used at least. Can I do something similar
with static_assert or would I have to do all the fields globally up front?

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

I did test performance back when I first implemented this. I had some issues
getting gcc to inline the functions, and that hurt performance quite a bit.
But I did not look at object size though. I moved them to the header so I could
add the inline. I can move them to the .c file in the next version.

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

I will move it before the Lantiq (NET_DSA_TAG_GSWIP) in the next version.

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

I will move this before the MTK in the next version.

Thanks,
Emil