Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 22 Apr 2025 18:17:14 +0200 Maxime Chevallier wrote:
> +/* perphy ->start() handler for GET requests */

Just because I think there are real bugs, I will allow myself an
uber-nit of asking to spell the perphy as per-PHY or such in the
comment? :)

> +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 */

another nit, this comment wasn't super clear without looking at the dump
for non-per-phy case. Maybe:

	/* Unlike per-dev dump, don't ignore dev. The dump handler
	 * will notice it and dump PHYs from given dev.
	 */
?

> +	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;

Now for the bugs..

> +	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;

> +	}

	ctx->pos_phyindex = 0;

	return 0;

IOW I don't see you resetting the pos_phyindex, so I think you'd only
dump correctly the first device? The next device will try to dump its
PHYs starting from the last index of the previous dev's PHY? [1]

> +	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);
> +	netdevice_tracker dev_tracker;
> +	struct net_device *dev;
> +	int ret = 0;
> +
> +	rcu_read_lock();
> +	for_each_netdev_dump(net, dev, ethnl_ctx->pos_ifindex) {
> +		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
> +		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();
> +		netdev_put(dev, &dev_tracker);

missing

		ethnl_ctx->req_info->dev = NULL;

right? Otherwise if we need to send multiple skbs the "continuation"
one will think we're doing a filtered dump.

Looking at commits 7c93a88785dae6 and c0111878d45e may be helpful,
but I doubt you can test it on a real system, filling even 4kB
may be hard for small messages :(

> +		if (ret < 0 && ret != -EOPNOTSUPP) {
> +			if (likely(skb->len))
> +				ret = skb->len;
> +			break;
> +		}
> +		ret = 0;

[1] or you can clear the pos_index here

> +	}
> +	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);

You have to release this in .done
dumpit gets called multiple times until we run out of objects to dump.
OTOH user may close the socket without finishing the dump operation.
So all .dumpit implementations must be "balanced". The only state we
should touch in them is the dump context to know where to pick up from
next time.

> +	} else {
> +		ret = ethnl_perphy_dump_all_dev(skb, ctx, genl_info_dump(cb));
> +	}
> +
> +	return ret;
> +}