Re: [PATCH net-next v13 4/4] net: dsa: add basic initial driver for MxL862xx switches

[Vladimir Oltean]

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.