Message-ID: <20260207221431.y4cckfsrbmhzs6ot@skbuf>
Date: Sun, 8 Feb 2026 00:14:31 +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 04:43:19PM +0000, Daniel Golle wrote:
> 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.

OK. If the fields were packed more densely maybe the tradeoff would have
looked differently then. But you're talking to an MCU and not to hardware.
And you don't need to keep a local direct representation of the data
passed through those packed buffers. Your arguments are valid.

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

No, the API usage example is how you imagine it. There's a structure
where you only need to pull in the fields you care about (and in
whatever order), rather than every other unrelated tidbit you don't
currently need and possibly never will (like ingress_marking_mode, etc etc).
And another array of PACKED_FIELD() where you say where each field goes.
You probably don't need a pack_fields() and an unpack_fields() call for
the same data structure in most cases, just one or the other.

Powered by blists - more mailing lists