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

[Théo Lebrun]

On Sat Mar 14, 2026 at 3:54 PM CET, Jakub Kicinski wrote:
> On Fri, 13 Mar 2026 16:14:23 +0100 Théo Lebrun wrote:
>> > 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.
>
> Sorry for the delay. This code can only be reached from the IOCTL path.
> The Netlink path will check that params haven't changed in
> ethnl_set_channels() (look at the @mod variable) and return 0 directly.
> So you're basically adding a discrepancy between ioctl and Netlink.
> Not a huge deal but I don't envy any user having to debug this..

Actually, the IOCTL path also does the check (see below). So the
`count == old_count` check shall be dropped from the driver
.set_channels() callback because it is redundant. Agreed?

Extract below for the ioctl codepath:

static int ethtool_set_channels(struct net_device *dev,
				void __user *useraddr)
{
	struct ethtool_channels channels, curr = { .cmd = ETHTOOL_GCHANNELS };

	// ...

	if (copy_from_user(&channels, useraddr, sizeof(channels)))
		return -EFAULT;

	dev->ethtool_ops->get_channels(dev, &curr);

	if (channels.rx_count == curr.rx_count &&
	    channels.tx_count == curr.tx_count &&
	    channels.combined_count == curr.combined_count &&
	    channels.other_count == curr.other_count)
		return 0;

	// ...
}

https://elixir.bootlin.com/linux/v6.19.8/source/net/ethtool/ioctl.c#L2263-L2267

Thanks,

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