diff mbox series

[net-next,v5,1/4] net: ethtool: Introduce per-PHY DUMP operations

Message ID 20250410123350.174105-2-maxime.chevallier@bootlin.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: ethtool: Introduce ethnl dump helpers | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 0 this patch: 4
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api warning Found: 'dev_hold(' was: 0 now: 1; 'dev_put(' was: 0 now: 1
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 180 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Maxime Chevallier April 10, 2025, 12:33 p.m. UTC
ethnl commands that target a phy_device need a DUMP implementation that
will fill the reply for every PHY behind a netdev. We therefore need to
iterate over the dev->topo to list them.

When multiple PHYs are behind the same netdev, it's also useful to
perform DUMP with a filter on a given netdev, to get the capability of
every PHY.

Implement dedicated genl ->start(), ->dumpit() and ->done() operations
for PHY-targetting command, allowing filtered dumps and using a dump
context that keep track of the PHY iteration for multi-message dump.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/netlink.c | 162 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)

Comments

Jakub Kicinski April 11, 2025, 12:21 a.m. UTC | #1
On Thu, 10 Apr 2025 14:33:46 +0200 Maxime Chevallier wrote:
> ethnl commands that target a phy_device need a DUMP implementation that
> will fill the reply for every PHY behind a netdev. We therefore need to
> iterate over the dev->topo to list them.
> 
> When multiple PHYs are behind the same netdev, it's also useful to
> perform DUMP with a filter on a given netdev, to get the capability of
> every PHY.
> 
> Implement dedicated genl ->start(), ->dumpit() and ->done() operations
> for PHY-targetting command, allowing filtered dumps and using a dump
> context that keep track of the PHY iteration for multi-message dump.

Transient warnings here that the new functions are not used :(
Would it work to squash the 3rd and 4th patches in?
Not ideal but better than having a warning.
Maxime Chevallier April 11, 2025, 7:26 a.m. UTC | #2
Hi Jakub,

On Thu, 10 Apr 2025 17:21:55 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 10 Apr 2025 14:33:46 +0200 Maxime Chevallier wrote:
> > ethnl commands that target a phy_device need a DUMP implementation that
> > will fill the reply for every PHY behind a netdev. We therefore need to
> > iterate over the dev->topo to list them.
> > 
> > When multiple PHYs are behind the same netdev, it's also useful to
> > perform DUMP with a filter on a given netdev, to get the capability of
> > every PHY.
> > 
> > Implement dedicated genl ->start(), ->dumpit() and ->done() operations
> > for PHY-targetting command, allowing filtered dumps and using a dump
> > context that keep track of the PHY iteration for multi-message dump.  
> 
> Transient warnings here that the new functions are not used :(
> Would it work to squash the 3rd and 4th patches in?
> Not ideal but better than having a warning.

ah damn yes indeed... meh sorry about that, I'll squash the other
patches in (except for the net/ethtool/phy.c patch). That said are you
more comfortable with this approach ? I was unsure this was what you
were expecting from the previous iteration.

Thanks,

Maxime
Jakub Kicinski April 12, 2025, 1:52 a.m. UTC | #3
On Fri, 11 Apr 2025 09:26:19 +0200 Maxime Chevallier wrote:
> > Transient warnings here that the new functions are not used :(
> > Would it work to squash the 3rd and 4th patches in?
> > Not ideal but better than having a warning.  
> 
> ah damn yes indeed... meh sorry about that, I'll squash the other
> patches in (except for the net/ethtool/phy.c patch). That said are you
> more comfortable with this approach ? I was unsure this was what you
> were expecting from the previous iteration.

I'm not sure if I did expect anything in particular after the idea I had
proved to not be feasible :) This looks good at a quick look.
diff mbox series

Patch

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index a163d40c6431..343faa6886a3 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -357,6 +357,16 @@  struct ethnl_dump_ctx {
 	unsigned long			pos_ifindex;
 };
 
+/**
+ * struct ethnl_perphy_dump_ctx - context for dumpit() PHY-aware callbacks
+ * @ethnl_ctx: generic ethnl context
+ * @pos_phyindex: iterator position for multi-msg DUMP
+ */
+struct ethnl_perphy_dump_ctx {
+	struct ethnl_dump_ctx	ethnl_ctx;
+	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,
@@ -407,6 +417,12 @@  static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
 	return (struct ethnl_dump_ctx *)cb->ctx;
 }
 
+static struct ethnl_perphy_dump_ctx *
+ethnl_perphy_dump_context(struct netlink_callback *cb)
+{
+	return (struct ethnl_perphy_dump_ctx *)cb->ctx;
+}
+
 /**
  * ethnl_default_parse() - Parse request message
  * @req_info:    pointer to structure to put data into
@@ -661,6 +677,152 @@  static int ethnl_default_start(struct netlink_callback *cb)
 	return ret;
 }
 
+/* perphy ->start() handler for GET requests */
+static int ethnl_perphy_start(struct netlink_callback *cb)
+{
+	struct ethnl_perphy_dump_ctx *phy_ctx = ethnl_perphy_dump_context(cb);
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct ethnl_dump_ctx *ctx = &phy_ctx->ethnl_ctx;
+	struct ethnl_reply_data *reply_data;
+	const struct ethnl_request_ops *ops;
+	struct ethnl_req_info *req_info;
+	struct genlmsghdr *ghdr;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
+
+	ghdr = nlmsg_data(cb->nlh);
+	ops = ethnl_default_requests[ghdr->cmd];
+	if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", ghdr->cmd))
+		return -EOPNOTSUPP;
+	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
+	if (!req_info)
+		return -ENOMEM;
+	reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL);
+	if (!reply_data) {
+		ret = -ENOMEM;
+		goto free_req_info;
+	}
+
+	/* Don't ignore the dev even for DUMP requests */
+	ret = ethnl_default_parse(req_info, &info->info, ops, false);
+	if (ret < 0)
+		goto free_reply_data;
+
+	ctx->ops = ops;
+	ctx->req_info = req_info;
+	ctx->reply_data = reply_data;
+	ctx->pos_ifindex = 0;
+
+	return 0;
+
+free_reply_data:
+	kfree(reply_data);
+free_req_info:
+	kfree(req_info);
+
+	return ret;
+}
+
+static int ethnl_perphy_dump_one_dev(struct sk_buff *skb,
+				     struct net_device *dev,
+				     struct ethnl_perphy_dump_ctx *ctx,
+				     const struct genl_info *info)
+{
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+	struct phy_device_node *pdn;
+	int ret = 0;
+
+	if (!dev->link_topo)
+		return 0;
+
+	xa_for_each_start(&dev->link_topo->phys, ctx->pos_phyindex, pdn,
+			  ctx->pos_phyindex) {
+		ethnl_ctx->req_info->phy_index = ctx->pos_phyindex;
+
+		/* We can re-use the original dump_one as ->prepare_data in
+		 * commands use ethnl_req_get_phydev(), which gets the PHY from
+		 * the req_info->phy_index
+		 */
+		ret = ethnl_default_dump_one(skb, dev, ethnl_ctx, info);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int ethnl_perphy_dump_all_dev(struct sk_buff *skb,
+				     struct ethnl_perphy_dump_ctx *ctx,
+				     const struct genl_info *info)
+{
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
+	int ret = 0;
+
+	rcu_read_lock();
+	for_each_netdev_dump(net, dev, ethnl_ctx->pos_ifindex) {
+		dev_hold(dev);
+		rcu_read_unlock();
+
+		/* per-PHY commands use ethnl_req_get_phydev(), which needs the
+		 * net_device in the req_info
+		 */
+		ethnl_ctx->req_info->dev = dev;
+		ret = ethnl_perphy_dump_one_dev(skb, dev, ctx, info);
+
+		rcu_read_lock();
+		dev_put(dev);
+
+		if (ret < 0 && ret != -EOPNOTSUPP) {
+			if (likely(skb->len))
+				ret = skb->len;
+			break;
+		}
+		ret = 0;
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
+/* perphy ->dumpit() handler for GET requests. */
+static int ethnl_perphy_dumpit(struct sk_buff *skb,
+			       struct netlink_callback *cb)
+{
+	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+	int ret = 0;
+
+	if (ethnl_ctx->req_info->dev) {
+		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
+						ctx, genl_info_dump(cb));
+
+		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
+			ret = skb->len;
+
+		netdev_put(ethnl_ctx->req_info->dev,
+			   &ethnl_ctx->req_info->dev_tracker);
+	} else {
+		ret = ethnl_perphy_dump_all_dev(skb, ctx, genl_info_dump(cb));
+	}
+
+	return ret;
+}
+
+/* perphy ->done() handler for GET requests */
+static int ethnl_perphy_done(struct netlink_callback *cb)
+{
+	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+
+	kfree(ethnl_ctx->reply_data);
+	kfree(ethnl_ctx->req_info);
+
+	return 0;
+}
+
 /* default ->done() handler for GET requests */
 static int ethnl_default_done(struct netlink_callback *cb)
 {