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

[Daniel Golle <daniel@makrotopia.org>]

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.


Cheers


Daniel