Re: [PATCH net-next] net: macb: allow MTU changes while the interface is running

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Wed Mar 18, 2026 at 12:25 PM CET, Nicolai Buchwitz wrote:
> On 18.3.2026 10:53, Théo Lebrun wrote:
>>> Hm, not really. Take a look at fbnic_set_ringparam()
>>> You need some struct that's config + pointers to all the resources.
>>> And make all allocation helpers operate on that without touching the 
>>> HW.
>>> Then you can just allocate a new struct, give it whatever config you
>>> need, call all the alloc helpers with it. Now you have a fully
>>> populated struct and haven't touched the HW yet at all. Stop HW,
>>> swap the resources, start HW.
>>> 
>>> I did something similar for the nfp driver but that code has been
>>> slightly adulterated since I left Netronome so fbnic is clearer :)
>> 
>> Do you feel we should (1) clone the full `struct macb` as done by fbnic
>> or, (2) just partially, with the few interesting fields. Something like
>> `struct stmmac_dma_conf`.
>> 
>> stmmac is not the greatest example. They have this struct that carries
>> their buffers but they still "close -> update -> open" on operations
>> versus the optimal "alloc -> reconfigure_hw -> free".
>> 
>> With #2 we could use an unnamed structure field.
>> https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html
>> See commit c4781dc3d1cf ("Kbuild: enable -fms-extensions").
>> 
>> struct macb_buffers {
>> 	struct macb_queue queues[N];
>> 	...
>> };
>> 
>> struct macb {
>> 	struct macb_buffers;
>> 	...
>> };
>> 
>> Goal is to keep `bp->queues` & co as before, to minimise the diff.
>
> The unnamed struct idea is nice for keeping the diff small, but I'm not
> sure it works cleanly for the per-queue case. The macb_queue_ring fields
> would be embedded anonymously in macb_queue, but the alloc helpers need
> to write into a separate clone. The swap then becomes memcpy/cast tricks
> between the anonymous portion of each queue and the clone's qring[q].
>
> Embedding the full queues[] array in the swappable struct would be
> simpler for the swap, but then NAPI, IRQs, spinlocks etc. travel
> along, which we don't want.
>
> For struct macb itself (rx_buffer_size, ring sizes, tieoff) the unnamed
> struct works fine and saves a lot of churn. It's really the per-queue
> ring fields where it gets awkward.

Agreed; we want to avoid swapping the whole of `struct macb_queue`
which carries too much weight and includes some pointers we registered
to other subsystems.

We can make it an unnamed field on a per-queue basis, with a union to
allow swapping. The usage of `bp->queues` stays the same with this.

struct macb_queue_resources {
	struct macb_dma_desc	*tx_ring;
	struct macb_tx_buff	*tx_buff;
	/* ... */
};

struct macb_queue {
	/* easy usage `q->tx_ring` and
	 * easy swap `q->resources = ...`
	 */
	union {
		struct macb_queue_resources;
		struct macb_queue_resources resources;
	};
	/* ... */
};

struct macb_resources {
	size_t rx_buffer_size;
};

struct macb {
	struct macb_queue queues[MACB_MAX_QUEUES];
	/* easy usage `bp->rx_buffer_size` and
	 * easy swap `bp->resources = ...`
	 */
	union {
		struct macb_resources;
		struct macb_resources resources;
	};
	/* ... */
}

// however now the swap is not a single expression like:
bp->resources = ...;

// but instead:
bp->resources = ...;
for (i = 0; i < n; i++)
	bp->queues[i].resources = ...;

If I get an ack on the concept I can attempt an RFC. It would probably
find its use in XDP/XSK as well.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com