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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Björn,

On 25/03/2026 15:50, Björn Töpel wrote:
> Add the dump_one_dev callback to ethnl_request_ops, allowing commands
> to provide custom per-device dump logic with sub-positioning. Extend
> ethnl_dump_ctx with ifindex and pos_sub fields.
> 
> No functional change; no command uses dump_one_dev yet.
> 
> Signed-off-by: Björn Töpel <bjorn@kernel.org>

Awesome :) I confirm that the phy dump still works just fine, and this
looks much simpler to me.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
>  net/ethtool/netlink.c | 66 ++++++++++++++++++-------------------------
>  net/ethtool/netlink.h | 31 ++++++++++++++++++++
>  2 files changed, 58 insertions(+), 39 deletions(-)
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 5046023a30b1..8d161f0882d0 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -346,36 +346,6 @@ int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
>  
>  /* GET request helpers */
>  
> -/**
> - * struct ethnl_dump_ctx - context structure for generic dumpit() callback
> - * @ops:        request ops of currently processed message type
> - * @req_info:   parsed request header of processed request
> - * @reply_data: data needed to compose the reply
> - * @pos_ifindex: saved iteration position - ifindex
> - *
> - * These parameters are kept in struct netlink_callback as context preserved
> - * between iterations. They are initialized by ethnl_default_start() and used
> - * in ethnl_default_dumpit() and ethnl_default_done().
> - */
> -struct ethnl_dump_ctx {
> -	const struct ethnl_request_ops	*ops;
> -	struct ethnl_req_info		*req_info;
> -	struct ethnl_reply_data		*reply_data;
> -	unsigned long			pos_ifindex;
> -};
> -
> -/**
> - * struct ethnl_perphy_dump_ctx - context for dumpit() PHY-aware callbacks
> - * @ethnl_ctx: generic ethnl context
> - * @ifindex: For Filtered DUMP requests, the ifindex of the targeted netdev
> - * @pos_phyindex: iterator position for multi-msg DUMP
> - */
> -struct ethnl_perphy_dump_ctx {
> -	struct ethnl_dump_ctx	ethnl_ctx;
> -	unsigned int		ifindex;
> -	unsigned long		pos_phyindex;
> -};
> -
>  static const struct ethnl_request_ops *
>  ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
>  	[ETHTOOL_MSG_STRSET_GET]	= &ethnl_strset_request_ops,
> @@ -618,6 +588,7 @@ 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;
> @@ -625,10 +596,20 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
>  
>  	rcu_read_lock();
>  	for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
> +		if (ctx->ifindex && ctx->ifindex != ctx->pos_ifindex)
> +			break;
> +
>  		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);
> @@ -674,19 +655,26 @@ static int ethnl_default_start(struct netlink_callback *cb)
>  	ret = ethnl_default_parse(req_info, &info->info, ops, false);
>  	if (ret < 0)
>  		goto free_reply_data;
> -	if (req_info->dev) {
> -		/* We ignore device specification in dump requests but as the
> -		 * same parser as for non-dump (doit) requests is used, it
> -		 * would take reference to the device if it finds one
> -		 */
> -		netdev_put(req_info->dev, &req_info->dev_tracker);
> -		req_info->dev = NULL;
> -	}
>  
>  	ctx->ops = ops;
>  	ctx->req_info = req_info;
>  	ctx->reply_data = reply_data;
>  	ctx->pos_ifindex = 0;
> +	ctx->ifindex = 0;
> +	ctx->pos_sub = 0;
> +
> +	if (req_info->dev) {
> +		if (ops->dump_one_dev) {
> +			/* Sub-iterator dumps keep track of the dev's ifindex
> +			 * so the dumpit handler can grab/release the netdev
> +			 * per iteration.
> +			 */
> +			ctx->ifindex = req_info->dev->ifindex;
> +			ctx->pos_ifindex = ctx->ifindex;
> +		}
> +		netdev_put(req_info->dev, &req_info->dev_tracker);
> +		req_info->dev = NULL;
> +	}
>  
>  	return 0;
>  
> diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
> index aaf6f2468768..e01adc5db02f 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -10,6 +10,28 @@
>  
>  struct ethnl_req_info;
>  
> +/**
> + * struct ethnl_dump_ctx - context structure for generic dumpit() callback
> + * @ops:        request ops of currently processed message type
> + * @req_info:   parsed request header of processed request
> + * @reply_data: data needed to compose the reply
> + * @pos_ifindex: saved iteration position - ifindex
> + * @ifindex:    for filtered dump requests, the ifindex of the targeted netdev
> + * @pos_sub:    iterator position for per-device iteration
> + *
> + * These parameters are kept in struct netlink_callback as context preserved
> + * between iterations. They are initialized by ethnl_default_start() and used
> + * in ethnl_default_dumpit() and ethnl_default_done().
> + */
> +struct ethnl_dump_ctx {
> +	const struct ethnl_request_ops	*ops;
> +	struct ethnl_req_info		*req_info;
> +	struct ethnl_reply_data		*reply_data;
> +	unsigned long			pos_ifindex;
> +	unsigned int			ifindex;
> +	unsigned long			pos_sub;
> +};
> +
>  u32 ethnl_bcast_seq_next(void);
>  int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
>  			       const struct nlattr *nest, struct net *net,
> @@ -365,6 +387,10 @@ int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
>   *	used e.g. to free any additional data structures outside the main
>   *	structure which were allocated by ->prepare_data(). When processing
>   *	dump requests, ->cleanup() is called for each message.
> + * @dump_one_dev:
> + *	Optional callback for dumping data for a single device. When set,
> + *	overrides the default dump behavior for GET requests, allowing
> + *	per-device iteration with sub-positioning via @pos_sub.
>   * @set_validate:
>   *	Check if set operation is supported for a given device, and perform
>   *	extra input checks. Expected return values:
> @@ -409,6 +435,11 @@ struct ethnl_request_ops {
>  			  const struct ethnl_reply_data *reply_data);
>  	void (*cleanup_data)(struct ethnl_reply_data *reply_data);
>  
> +	int (*dump_one_dev)(struct sk_buff *skb,
> +			    struct ethnl_dump_ctx *ctx,
> +			    unsigned long *pos_sub,
> +			    const struct genl_info *info);
> +
>  	int (*set_validate)(struct ethnl_req_info *req_info,
>  			    struct genl_info *info);
>  	int (*set)(struct ethnl_req_info *req_info,