Re: [PATCH net-next 01/11] ethtool: Add dump_one_dev callback for per-device sub-iteration

["Björn Töpel" <bjorn@kernel.org>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 10 Mar 2026 11:47:31 +0100 Björn Töpel wrote:
>> The per-PHY specific dump functions share a lot functionality of with
>> the default dumpit infrastructure, but does not share the actual code.
>> By introducing a new sub-iterator function, the two dumpit variants
>> can be folded into one set of functions.
>> 
>> Add a new dump_one_dev callback in ethnl_request_ops. When
>> ops->dump_one_dev is set, ethnl_default_start() saves the target
>> device's ifindex for filtered dumps, and ethnl_default_dumpit()
>> delegates per-device iteration to the callback instead of calling
>> ethnl_default_dump_one() directly. No separate start/dumpit/done
>> functions are needed.
>> 
>> For the existing per-PHY commands (PSE, PLCA, PHY, MSE), the shared
>> ethnl_perphy_dump_one_dev helper provides the xa_for_each_start loop
>> over the device's PHY topology.
>> 
>> This prepares the ethtool infrastructure for other commands that need
>> similar per-device sub-iteration.
>
> Feels like this could be split into two patches for ease of review.

Hmm, OK! My take was the it was mostly moving stuff.

> Warning: net/ethtool/netlink.h:441 struct member 'dump_one_dev' not described in 'ethnl_request_ops'
> Warning: net/ethtool/netlink.h:441 struct member 'dump_one_dev' not described in 'ethnl_request_ops'

Thanks! (I've looked thru all the pw complaints, and fixed locally!)

>
>> @@ -616,17 +580,41 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
>>  				struct netlink_callback *cb)
>>  {
>>  	struct ethnl_dump_ctx *ctx = ethnl_dump_context(cb);
>> +	const struct genl_info *info = genl_info_dump(cb);
>>  	struct net *net = sock_net(skb->sk);
>>  	netdevice_tracker dev_tracker;
>>  	struct net_device *dev;
>>  	int ret = 0;
>>  
>> +	if (ctx->ops->dump_one_dev && ctx->ifindex) {
>> +		dev = netdev_get_by_index(net, ctx->ifindex, &dev_tracker,
>> +					  GFP_KERNEL);
>> +		if (!dev)
>> +			return -ENODEV;
>> +
>> +		ctx->req_info->dev = dev;
>> +		ret = ctx->ops->dump_one_dev(skb, ctx, &ctx->pos_sub, info);
>> +
>> +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
>> +			ret = skb->len;
>> +
>> +		netdev_put(dev, &dev_tracker);
>> +		return ret;
>> +	}
>> +
>>  	rcu_read_lock();
>>  	for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
>>  		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
>>  		rcu_read_unlock();
>>  
>> -		ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
>> +		if (ctx->ops->dump_one_dev) {
>> +			ctx->req_info->dev = dev;
>> +			ret = ctx->ops->dump_one_dev(skb, ctx, &ctx->pos_sub,
>> +						     info);
>> +			ctx->req_info->dev = NULL;
>> +		} else {
>> +			ret = ethnl_default_dump_one(skb, dev, ctx, info);
>> +		}
>>  
>>  		rcu_read_lock();
>>  		netdev_put(dev, &dev_tracker);
>
> Not sure if it works here but another way to implementing single dump
> and a loop dump concisely is to init both ifindex and pos_ifindex when
> request is parsed and then add
>
> 	if (ctx->ifindex && ctx->ifindex != ctx->pos_ifindex)
> 		break;
>
> at the start of the loop. That way the body of the loop doesn't have to
> be repeated in a separate if 

Indeed! I think that would work, and that'd be cleaner!

Cheers!