Re: [PATCH net-next 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Théo Lebrun]

On Mon Mar 9, 2026 at 10:15 PM CET, Jakub Kicinski wrote:
> On Mon, 09 Mar 2026 18:04:06 +0100 Théo Lebrun wrote:
>> On Sat Mar 7, 2026 at 4:09 AM CET, Jakub Kicinski wrote:
>> > On Thu, 05 Mar 2026 18:20:14 +0100 Théo Lebrun wrote:  
>> >> +	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
>> >> +		return -EOPNOTSUPP;  
>> >
>> > Why not set max to 1 in this case?  
>> 
>> With !QUEUE_DISABLE, we only know how to run with all queues enabled.
>> It doesn't imply that max_num_queues == 1.
>> 
>> MACB_CAPS_QUEUE_DISABLE means that the field QUEUE_DISABLE (BIT0) in the
>> per-queue register RBQP disables queue Rx. If we don't have that
>> capability we can have multiple queues (if HW supports it) but we must
>> always run with all enabled.
>
> Oh, I see! Perhaps just a comment over the check to inform the reader
> that the lack of capabilities means all rx queues must be enabled.

ACK, will insert a comment.

>> >> +	if (running) {
>> >> +		ret = macb_open(bp->dev);
>> >> +		if (ret) {
>> >> +			bp->num_queues = old_count;
>> >> +			netif_set_real_num_queues(bp->dev, old_count, old_count);
>> >> +			macb_open(bp->dev);  
>> >
>> > both macb_open() calls may fail under memory pressure
>> > For new functionality we ask drivers to allocate all necessary
>> > resources upfront then just swap them in and reconfigure HW  
>> 
>> The main reason we want to set queue count is memory savings. If we take
>> the Mobileye EyeQ5 SoC, it has a small 32MiB RAM alias usable for DMA.
>> If we waste it on networking we have less available for the remaining
>> peripherals. Is there some way we could avoid upfront allocations?
>
> We've been asking everyone to follow the "pre-allocate resources
> paradigm" for a few years now. It has huge benefits for system
> reliability. If you don't want to complicate code at this stage
> you can support configuring queue count only when the device is down.
> But the ask will keep coming back, any time you try to do the close+open

I'll iterate with a .set_channels() implementation that returns -EBUSY
if netif_running(), making sure to leave a comment explaining the
tradeoff.

Thanks Jakub,

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