Re: [PATCH net-next v4 4/8] net: ethtool: netlink: Introduce command-specific dump_one_dev

[Kory Maincent <kory.maincent@bootlin.com>]

On Mon, 24 Mar 2025 11:40:06 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Prepare more generic ethnl DUMP hanldling, by allowing netlink commands
> to register their own dump_one_dev() callback. This avoids having to
> roll with a fully custom genl ->dumpit callback, allowing the re-use
> of some ethnl plumbing.
> 
> Fallback to the default dump_one_dev behaviour when no custom callback
> is found.
> 
> The command dump context is maintained within the ethnl_dump_ctx, that
> we move in netlink.h so that command handlers can access it.
> 
> This context can be allocated/freed in new ->dump_start() and
> ->dump_done() callbacks.  
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> V4 : No changes
> 
>  net/ethtool/netlink.c | 61 +++++++++++++++++++++++++------------------
>  net/ethtool/netlink.h | 35 +++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 63ede3638708..0345bffa0678 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -339,24 +339,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;
> -};
> -
>  static const struct ethnl_request_ops *
>  ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
>  	[ETHTOOL_MSG_STRSET_GET]	= &ethnl_strset_request_ops,
> @@ -540,9 +522,9 @@ static int ethnl_default_doit(struct sk_buff *skb, struct
> genl_info *info) return ret;
>  }
>  
> -static int ethnl_default_dump_one_dev(struct sk_buff *skb, struct net_device
> *dev,
> -				      const struct ethnl_dump_ctx *ctx,
> -				      const struct genl_info *info)
> +static int ethnl_default_dump_one(struct sk_buff *skb,
> +				  const struct ethnl_dump_ctx *ctx,
> +				  const struct genl_info *info)
>  {
>  	void *ehdr;
>  	int ret;
> @@ -553,15 +535,15 @@ static int ethnl_default_dump_one_dev(struct sk_buff
> *skb, struct net_device *de if (!ehdr)
>  		return -EMSGSIZE;
>  
> -	ethnl_init_reply_data(ctx->reply_data, ctx->ops, dev);
>  	rtnl_lock();
> -	netdev_lock_ops(dev);
> +	netdev_lock_ops(ctx->reply_data->dev);
>  	ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, info);
> -	netdev_unlock_ops(dev);
> +	netdev_unlock_ops(ctx->reply_data->dev);
>  	rtnl_unlock();
>  	if (ret < 0)
>  		goto out;
> -	ret = ethnl_fill_reply_header(skb, dev, ctx->ops->hdr_attr);
> +	ret = ethnl_fill_reply_header(skb, ctx->reply_data->dev,
> +				      ctx->ops->hdr_attr);

Instead of modifying all these lines you could simply add this at the beginning:
struct net_device *dev = ctx->reply_data->dev;

With that:
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com