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

[Théo Lebrun]

Hello Jakub,

On Fri Mar 13, 2026 at 2:26 AM CET, Jakub Kicinski wrote:
> On Wed, 11 Mar 2026 17:41:53 +0100 Théo Lebrun wrote:
>> +	struct macb *bp = netdev_priv(netdev);
>> +	unsigned int old_count = bp->num_queues;
>> +	unsigned int count = ch->combined_count;
>> +	int ret = 0;
>
> unnecessary init

ack

>> +	/*
>> +	 * MACB_CAPS_QUEUE_DISABLE means that the field QUEUE_DISABLE/BIT0 in
>> +	 * the per-queue RBQP register disables queue Rx. If we don't have that
>> +	 * capability we can have multiple queues but we must always run with
>> +	 * all enabled.
>> +	 */
>> +	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * An ideal .set_channels() implementation uses upfront allocated
>> +	 * resources and swaps them in, bringing reliability under memory
>> +	 * pressure. However, here we implement it for memory savings in
>> +	 * setups with less than max number of queues active.
>> +	 *
>> +	 * Signal it by refusing .set_channels() once interface is opened.
>> +	 */
>> +	if (netif_running(bp->dev))
>> +		return -EBUSY;
>> +
>> +	if (count == old_count)
>> +		return 0;
>
> should we reorder this with the running() check?

I don't agree. For example when an operation is not supported, we start
by checking that and returning EOPNOTSUPP. Then we validate the input
data. Then we act.

Here it is the same. When netif_running(), we never reply to any
request even if it happens to be a no-op.

I'll go ahead and send V3. Seeing how this was only a question I'll make
the guess you don't care much about it and are fine either way.
Same for me.

Thanks!

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