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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260206133418.vxui223u4d6jdpql@skbuf>
Date: Fri, 6 Feb 2026 15:34:18 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Golle <daniel@...rotopia.org>
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: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.