Message-ID: <aYYaJ4Yp_MAQ0eqw@makrotopia.org>
Date: Fri, 6 Feb 2026 16:43:19 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Frank Wunderlich <frankwu@....de>, Chad Monroe <chad@...roe.io>,
	Cezary Wilmanski <cezary.wilmanski@...ran.com>,
	Liang Xu <lxu@...linear.com>, John Crispin <john@...ozen.org>
Subject: Re: [PATCH net-next v13 4/4] net: dsa: add basic initial driver for
 MxL862xx switches

On Fri, Feb 06, 2026 at 03:34:18PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 06, 2026 at 03:14:26AM +0000, Daniel Golle wrote:
> > Hi Jakub,
> > 
> > thank you for looking into this driver another time.
> > 
> > On Thu, Feb 05, 2026 at 06:21:17PM -0800, Jakub Kicinski wrote:
> > > On Wed, 4 Feb 2026 13:33:19 +0000 Daniel Golle wrote:
> > > > +/* The switch firmware expects all structs to be byte-aligned */
> > > > +#pragma pack(push, 1)
> > > 
> > > "Byte-aligned" means..? Generally aligned means that it starts
> > > at an address which is multiple of X. All addresses are multiple of 1
> > 
> > In case fo the firmware running on this switch it means that data types
> > used in structures used as input and output parameters for firmware
> > functions should be aligned to 8 bits, without any additional padding in
> > between.
> > 
> > struct foo {
> > 	u8	var1;
> > 	__le16	var2;
> > 	__le32	var3;
> > } __packed;
> > 
> > It's size is 7 bytes and it looks like this:
> > 
> > .||||||||.||||||||.||||||||.||||||||.||||||||.||||||||.||||||||.
> > |  var1  |       var2      |                var3               |
> > .        .   LSB  .   MSB  .  LSB                         MSB  .
> > 
> > 
> > This is what the firmware on the other end expects, from all data sent
> > to it and what the Linux host has to expect from all data received from
> > it.
> > 
> > > We used you push back against blanket __packed because it's forcing
> > > all *host* accesses to also assume that the structures are unaligned.
> > 
> > Understand that in general, and of course know that using packed structs
> > without a hardware requirement of doing so needlessly wastes CPU cycles
> > on each access to struct members.
> > 
> > However, in this case this is a header file which exclusively defines
> > structs which are only used to communicate with the firmware running on
> > the switch. Using them for anything else, such as storing or processing
> > data the driver deals with internally is, very inconvenient because all
> > types are also defined as little-endian, so not only unaligned access,
> > but also endian conversion burdens every access (in the sense that it
> > burdens the programmer on little-endian machines, but the CPU as well on
> > big-endian machines).
> > 
> > tl;dr: This whole file is only API definition. And this is how the
> > firmware API is defined, and that's the only way to deal with that
> > switch.
> > 
> > (I would have preferred if they just exposed the internal 16-bit
> > registers of the switch via MDIO, and have asked MxL for that several
> > times, without success)
> > 
> > > The best practice is to pack only specific structs which need it
> > > and add compile_assert()s to make sure that the compiler doesn't add
> > > any padding.
> > 
> > Imho checking whether each of these structs is naturally packed (ie.
> > 8-bit aligned without padding between 8-bit aligned members) is prone to
> > human errors which only become visible when testing on the real
> > hardware, and hence complicates maintainance.
> > 
> > Other drivers which operate on similar APIs (many GPU drivers, for
> > example) also use #pragma pack(push, 1) in header files defining
> > external API. Also there all external API definitions are kept in a
> > separate file, away from any of the datastructures used by the driver
> > internally at runtime.
> > 
> > Anyway, if you really really want me to set individual __packed for each
> > struct which isn't naturally packed in this whole file, please tell me
> > clearly that this is what you would like, and I will of course do it
> > despite disagreeing with the reasoning.
> 
> When I created the pack_fields() API it was exactly for situations like
> this. It helps you keep naturally aligned structures in native CPU
> endianness while adapting to whatever quirks the peripheral you're
> talking to has.

I've spent an hour studying the pack_fields() API and it's (well
written) documentation. The only example of it's use in the current
kernel I could find is the Intel E800 (ICE) driver. And there it does
make sense as it is handling conversion between CPU and hardware formats
in the hotpath for DMA descriptors, a total of 3 different structs, each
with their individual accessor functions.

Using this approach for this switch driver would require writing a lot
of boilerplate code, accessor functions for each and every struct,
and a struct definition once unpacked for the host platform and then
again using the PACKED_FIELD(...) notation for the hardware format.
Surely, most of that could be auto-generated using the existing
vendor drivers API definition. Yet (at least to me) it feels like
over-engineering and also it would require rewriting most of the driver
which has been discussed for almost 2 months now.

Also note that the driver doesn't need the naturally aligned version of
all these structs in native CPU endian -- they are not used for further
processing anything, you can see that because they aren't ever used as a
function parameters, but only ever as exchange formats when
communicating with the firmware.

Maybe I'm missing something obvious here and there is a more simple way
to use this API, some generic macros using compiler introspection to
magically handle everything without needing to write packed and unpacked
struct definitions and individual pack/unpack boiler-plate functions for
each struct. If so, please provide me with an example or explain how you
imagine the pack_fields() API to be used in the context of this driver
and it's total of at more than 30 different structs which will be used
for all the different firmware function I will need to use in order to
implement phylink_pcs as well as the various offloading and VLAN-related
functionality the driver should have in the end (ie. the structs you
currently see in the mxl862xx-api.h file are just a fraction of what I
hope to add there by follow-up series)

Powered by blists - more mailing lists