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

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

[Nicolai Buchwitz]

macb_change_mtu() currently returns -EBUSY if the interface is running,
requiring users to bring the interface down before changing the MTU. This
is unnecessarily restrictive.

Instead, close and reopen the interface around the MTU change so that RX
DMA buffers are reallocated for the new MTU. This is the same approach
used by many other network drivers (e.g. igb, tg3, stmmac).

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 5e27e0e87a55..8dd01031250d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
 
 static int macb_change_mtu(struct net_device *dev, int new_mtu)
 {
-	if (netif_running(dev))
-		return -EBUSY;
+	bool was_running = netif_running(dev);
+
+	if (was_running)
+		macb_close(dev);
 
 	WRITE_ONCE(dev->mtu, new_mtu);
 
+	if (was_running)
+		return macb_open(dev);
+
 	return 0;
 }
 
-- 
2.51.0

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

[Théo Lebrun]

Hello Nicolai,

On Mon Mar 16, 2026 at 10:27 AM CET, Nicolai Buchwitz wrote:
> macb_change_mtu() currently returns -EBUSY if the interface is running,
> requiring users to bring the interface down before changing the MTU. This
> is unnecessarily restrictive.

One valid reasoning for making the operation return -EBUSY is that
macb_close() will lead to packet loss for a few seconds. Users might
not expect an MTU change to trigger that. -EBUSY means user (be it
human or software) becomes aware and has time for a second thought.

But clearly, it becomes annoying as the user in the long run, when you
own your platform and know the implications.

> Instead, close and reopen the interface around the MTU change so that RX
> DMA buffers are reallocated for the new MTU. This is the same approach
> used by many other network drivers (e.g. igb, tg3, stmmac).
>
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5e27e0e87a55..8dd01031250d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
>  
>  static int macb_change_mtu(struct net_device *dev, int new_mtu)
>  {
> -	if (netif_running(dev))
> -		return -EBUSY;
> +	bool was_running = netif_running(dev);
> +
> +	if (was_running)
> +		macb_close(dev);
>  
>  	WRITE_ONCE(dev->mtu, new_mtu);
>  
> +	if (was_running)
> +		return macb_open(dev);
> +
>  	return 0;
>  }

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
Tested-by: Théo Lebrun <theo.lebrun@bootlin.com> # on eyeq5-epm

Thanks,

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

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

[Nicolai Buchwitz]

Hi Théo

Thanks for the review and testing!

On 17.3.2026 18:00, Théo Lebrun wrote:
> Hello Nicolai,
> 
> On Mon Mar 16, 2026 at 10:27 AM CET, Nicolai Buchwitz wrote:
>> macb_change_mtu() currently returns -EBUSY if the interface is 
>> running,
>> requiring users to bring the interface down before changing the MTU. 
>> This
>> is unnecessarily restrictive.
> 
> One valid reasoning for making the operation return -EBUSY is that
> macb_close() will lead to packet loss for a few seconds. Users might
> not expect an MTU change to trigger that. -EBUSY means user (be it
> human or software) becomes aware and has time for a second thought.
> 
> But clearly, it becomes annoying as the user in the long run, when you
> own your platform and know the implications.

Since setting the MTU already requires elevated rights and usually 
involves reconfiguring surrounding infrastructure that may also drop 
packets, I think the better UX is worth the brief interruption.

> 
>> Instead, close and reopen the interface around the MTU change so that 
>> RX
>> DMA buffers are reallocated for the new MTU. This is the same approach
>> used by many other network drivers (e.g. igb, tg3, stmmac).
>> 
>> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 5e27e0e87a55..8dd01031250d 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
>> 
>>  static int macb_change_mtu(struct net_device *dev, int new_mtu)
>>  {
>> -	if (netif_running(dev))
>> -		return -EBUSY;
>> +	bool was_running = netif_running(dev);
>> +
>> +	if (was_running)
>> +		macb_close(dev);
>> 
>>  	WRITE_ONCE(dev->mtu, new_mtu);
>> 
>> +	if (was_running)
>> +		return macb_open(dev);
>> +
>>  	return 0;
>>  }
> 
> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
> Tested-by: Théo Lebrun <theo.lebrun@bootlin.com> # on eyeq5-epm
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Cheers
Nicolai

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

[Breno Leitao]

On Mon, Mar 16, 2026 at 10:27:20AM +0100, Nicolai Buchwitz wrote:
> macb_change_mtu() currently returns -EBUSY if the interface is running,
> requiring users to bring the interface down before changing the MTU. This
> is unnecessarily restrictive.
> 
> Instead, close and reopen the interface around the MTU change so that RX
> DMA buffers are reallocated for the new MTU. This is the same approach
> used by many other network drivers (e.g. igb, tg3, stmmac).
> 
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5e27e0e87a55..8dd01031250d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
>  
>  static int macb_change_mtu(struct net_device *dev, int new_mtu)
>  {
> -	if (netif_running(dev))
> -		return -EBUSY;
> +	bool was_running = netif_running(dev);
> +
> +	if (was_running)
> +		macb_close(dev);
>  
>  	WRITE_ONCE(dev->mtu, new_mtu);
>  
> +	if (was_running)
> +		return macb_open(dev);

Would you like to keep the new_mtu set, even if macb_open() fails?

Other than that, it looks good to me.
--breno

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

[Breno Leitao]

On Tue, Mar 17, 2026 at 12:42:04PM -0700, Breno Leitao wrote:
> On Mon, Mar 16, 2026 at 10:27:20AM +0100, Nicolai Buchwitz wrote:
> > macb_change_mtu() currently returns -EBUSY if the interface is running,
> > requiring users to bring the interface down before changing the MTU. This
> > is unnecessarily restrictive.
> > 
> > Instead, close and reopen the interface around the MTU change so that RX
> > DMA buffers are reallocated for the new MTU. This is the same approach
> > used by many other network drivers (e.g. igb, tg3, stmmac).
> > 
> > Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 5e27e0e87a55..8dd01031250d 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
> >  
> >  static int macb_change_mtu(struct net_device *dev, int new_mtu)
> >  {
> > -	if (netif_running(dev))
> > -		return -EBUSY;
> > +	bool was_running = netif_running(dev);
> > +
> > +	if (was_running)
> > +		macb_close(dev);
> >  
> >  	WRITE_ONCE(dev->mtu, new_mtu);
> >  
> > +	if (was_running)
> > +		return macb_open(dev);
> 
> Would you like to keep the new_mtu set, even if macb_open() fails?

Looking a bit further, I found that macb_set_ringparam() doesn't return
failure if macb_open() fails. Should it be fixed also?

Should we have something similar to your code above?

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 5bc35f651ebd2..3c43fc85af79d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3516,7 +3516,7 @@ static int macb_set_ringparam(struct net_device *netdev,
        bp->tx_ring_size = new_tx_size;

        if (reset)
-               macb_open(bp->dev);
+               return macb_open(bp->dev);

        return 0;
 }

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

[Nicolai Buchwitz]

On 17.3.2026 20:42, Breno Leitao wrote:
> On Mon, Mar 16, 2026 at 10:27:20AM +0100, Nicolai Buchwitz wrote:
>> macb_change_mtu() currently returns -EBUSY if the interface is 
>> running,
>> requiring users to bring the interface down before changing the MTU. 
>> This
>> is unnecessarily restrictive.
>> 
>> Instead, close and reopen the interface around the MTU change so that 
>> RX
>> DMA buffers are reallocated for the new MTU. This is the same approach
>> used by many other network drivers (e.g. igb, tg3, stmmac).
>> 
>> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 5e27e0e87a55..8dd01031250d 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
>> 
>>  static int macb_change_mtu(struct net_device *dev, int new_mtu)
>>  {
>> -	if (netif_running(dev))
>> -		return -EBUSY;
>> +	bool was_running = netif_running(dev);
>> +
>> +	if (was_running)
>> +		macb_close(dev);
>> 
>>  	WRITE_ONCE(dev->mtu, new_mtu);
>> 
>> +	if (was_running)
>> +		return macb_open(dev);
> 
> Would you like to keep the new_mtu set, even if macb_open() fails?

I think keeping the new MTU even if macb_open() fails is fine. The 
interface is down at that point anyway?
igb does the same and even discards the igb_up() return value entirely. 
At least we propagate the error.

> 
> Other than that, it looks good to me.
> --breno

Thanks
Nicolai

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

[Nicolai Buchwitz]

On 17.3.2026 20:47, Breno Leitao wrote:
> On Tue, Mar 17, 2026 at 12:42:04PM -0700, Breno Leitao wrote:
>> On Mon, Mar 16, 2026 at 10:27:20AM +0100, Nicolai Buchwitz wrote:
>> > macb_change_mtu() currently returns -EBUSY if the interface is running,
>> > requiring users to bring the interface down before changing the MTU. This
>> > is unnecessarily restrictive.
>> >
>> > Instead, close and reopen the interface around the MTU change so that RX
>> > DMA buffers are reallocated for the new MTU. This is the same approach
>> > used by many other network drivers (e.g. igb, tg3, stmmac).
>> >
>> > Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
>> > ---
>> >  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> > index 5e27e0e87a55..8dd01031250d 100644
>> > --- a/drivers/net/ethernet/cadence/macb_main.c
>> > +++ b/drivers/net/ethernet/cadence/macb_main.c
>> > @@ -3262,11 +3262,16 @@ static int macb_close(struct net_device *dev)
>> >
>> >  static int macb_change_mtu(struct net_device *dev, int new_mtu)
>> >  {
>> > -	if (netif_running(dev))
>> > -		return -EBUSY;
>> > +	bool was_running = netif_running(dev);
>> > +
>> > +	if (was_running)
>> > +		macb_close(dev);
>> >
>> >  	WRITE_ONCE(dev->mtu, new_mtu);
>> >
>> > +	if (was_running)
>> > +		return macb_open(dev);
>> 
>> Would you like to keep the new_mtu set, even if macb_open() fails?
> 
> Looking a bit further, I found that macb_set_ringparam() doesn't return
> failure if macb_open() fails. Should it be fixed also?
> 
> Should we have something similar to your code above?
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
> b/drivers/net/ethernet/cadence/macb_main.c
> index 5bc35f651ebd2..3c43fc85af79d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3516,7 +3516,7 @@ static int macb_set_ringparam(struct net_device 
> *netdev,
>         bp->tx_ring_size = new_tx_size;
> 
>         if (reset)
> -               macb_open(bp->dev);
> +               return macb_open(bp->dev);
> 
>         return 0;
>  }

Makes sense. I will send a separate patch for this.

Cheers
Nicolai

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

[Jakub Kicinski]

On Tue, 17 Mar 2026 18:00:02 +0100 Théo Lebrun wrote:
> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

I told you recently that open / close is not allowed.
So why are you putting a rb tag on a patch which does exactly that?

https://lore.kernel.org/all/20260306190948.44d23f8f@kernel.org/
-- 
pw-bot: reject

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

[Nicolai Buchwitz]

On 17.3.2026 23:23, Jakub Kicinski wrote:
> On Tue, 17 Mar 2026 18:00:02 +0100 Théo Lebrun wrote:
>> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> I told you recently that open / close is not allowed.
> So why are you putting a rb tag on a patch which does exactly that?
> 
> https://lore.kernel.org/all/20260306190948.44d23f8f@kernel.org/

Just checking if I got the idea before submitting another approach.
Something like:


static int macb_change_mtu(struct net_device *dev, int new_mtu)
{
	// declarations
	// ...

	if (!netif_running(dev)) {
		WRITE_ONCE(dev->mtu, new_mtu);
		return 0;
	}

	// stop TX + NAPI
	// ...

	// stop MAC (macb_reset_hw), no phylink/PHY/PM touch
	// ...

	macb_free_consistent(bp);

	WRITE_ONCE(dev->mtu, new_mtu);
	macb_init_rx_buffer_size(bp, bufsz);

	err = macb_alloc_consistent(bp);
	if (err) {
		// device is down, no DMA buffers - same as igb
		return err;
	}

	// init rings, init HW, re-enable NAPI + TX
	// ...

	return 0;
}


On alloc failure the device would be left stopped, same as 
igb_change_mtu() does it. Or would you prefer stmmac-style 
pre-allocation?
That would mean refactoring macb_alloc_consistent() to return a separate 
resource set.

Thanks
Nicolai

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

[Jakub Kicinski]

On Tue, 17 Mar 2026 23:58:21 +0100 Nicolai Buchwitz wrote:
> On 17.3.2026 23:23, Jakub Kicinski wrote:
> > On Tue, 17 Mar 2026 18:00:02 +0100 Théo Lebrun wrote:  
> >> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>  
> > 
> > I told you recently that open / close is not allowed.
> > So why are you putting a rb tag on a patch which does exactly that?
> > 
> > https://lore.kernel.org/all/20260306190948.44d23f8f@kernel.org/  
> 
> Just checking if I got the idea before submitting another approach.
> Something like:

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

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

[Théo Lebrun]

On Wed Mar 18, 2026 at 12:23 AM CET, Jakub Kicinski wrote:
> On Tue, 17 Mar 2026 23:58:21 +0100 Nicolai Buchwitz wrote:
>> On 17.3.2026 23:23, Jakub Kicinski wrote:
>> > On Tue, 17 Mar 2026 18:00:02 +0100 Théo Lebrun wrote:  
>> >> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>  
>> > 
>> > I told you recently that open / close is not allowed.
>> > So why are you putting a rb tag on a patch which does exactly that?
>> > 
>> > https://lore.kernel.org/all/20260306190948.44d23f8f@kernel.org/  

Ah, I might have misunderstood at the time.

   > allocate all necessary resources upfront then just swap them in
   > and reconfigure HW

In .set_channels() context, does "upfront" mean
(1) alloc all queue buffers at probe (and never dealloc) or
(2) alloc new buffer at operation start, swap in, then dealloc?

I had understood #1. I didn't see how that applied to this series (an
MTU change) which of course must realloc. But now it seems it was #2 as
you pointed to fbnic as reference and that's what fbnic_set_channels()
does.

>> Just checking if I got the idea before submitting another approach.
>> Something like:
>
> 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.

Thanks,

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

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

[Nicolai Buchwitz]

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.

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

Cheers
Nicolai

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

[Théo Lebrun]

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

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

[Jakub Kicinski]

On Wed, 18 Mar 2026 10:53:37 +0100 Théo Lebrun wrote:
> Goal is to keep `bp->queues` & co as before, to minimise the diff.

Minimizing the diff isn't really a goal usually.
As long as you separate out large mechanical changes so that it's easy
to scan thru them and confirm they are correct it's not a problem.
Of course if there's a mix of functional changes and renames that's 
a nightmare.