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

Kory Maincent
2025-03-25T13:23:17Z

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