Re: [net-next,v6,10/10] net: ethtool: Introduce ethtool command to list ports

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: ethtool: Introduce ethtool command to list ports

This commit introduces a new ethtool netlink command to expose phy_port
information to userspace, allowing enumeration of available ports and
their capabilities on a given interface.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2265e2c9bfbe..668af8739c21 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18421,6 +18421,7 @@ F:	Documentation/devicetree/bindings/net/ethernet-connector.yaml
>  F:	Documentation/networking/phy-port.rst
>  F:	drivers/net/phy/phy_port.c
>  F:	include/linux/phy_port.h
> +F:	net/ethtool/port.c
>  K:	struct\s+phy_port|phy_port_
>
>  NETWORKING [GENERAL]

[ ... ]

> diff --git a/net/ethtool/port.c b/net/ethtool/port.c
> new file mode 100644
> index 000000000000..d870ed70769e
> --- /dev/null
> +++ b/net/ethtool/port.c
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2026 Bootlin
> + *
> + */
> +#include "common.h"
> +#include "bitset.h"
> +#include "netlink.h"
> +
> +#include <linux/phy.h>
> +#include <linux/phy_link_topology.h>
> +#include <linux/phy_port.h>
> +#include <net/netdev_lock.h>

[ ... ]

> +static int port_dump_one(struct sk_buff *skb, struct net_device *dev,
> +			 struct netlink_callback *cb)
> +{
> +	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
> +	void *ehdr;
> +	int ret;
> +
> +	ehdr = ethnl_dump_put(skb, cb, ETHTOOL_A_PORT_HEADER);
> +	if (!ehdr)
> +		return -EMSGSIZE;

Is ETHTOOL_A_PORT_HEADER the correct constant here?

The third parameter to ethnl_dump_put() should be the genetlink command
ID that gets stored in the message header's cmd field. Looking at other
dump implementations in net/ethtool/:

net/ethtool/tunnels.c:
    ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY);

net/ethtool/tsinfo.c:
    ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_TSINFO_GET_REPLY);

net/ethtool/rss.c:
    ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_RSS_GET_REPLY);

These all pass ETHTOOL_MSG_*_GET_REPLY command constants, not
ETHTOOL_A_* attribute constants.

ETHTOOL_A_PORT_HEADER is a netlink attribute with value 1 (from
include/uapi/linux/ethtool_netlink.h). Meanwhile, this file's
ethnl_port_request_ops structure correctly uses
ETHTOOL_MSG_PORT_GET_REPLY for .reply_cmd, suggesting the same
constant should be used here. Can this cause the wrong command ID
to be sent in every dump response message?

> +
> +	memset(ctx->reply_data, 0, sizeof(struct port_reply_data));
> +	ctx->reply_data->base.dev = dev;
> +
> +	rtnl_lock();
> +	netdev_lock_ops(dev);
> +
> +	ret = port_prepare_data(&ctx->req_info->base, &ctx->reply_data->base,
> +				genl_info_dump(cb));
> +
> +	netdev_unlock_ops(dev);
> +	rtnl_unlock();
> +
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_PORT_HEADER);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = port_fill_reply(skb, &ctx->req_info->base, &ctx->reply_data->base);
> +
> +out:
> +	ctx->reply_data->base.dev = NULL;
> +	if (ret < 0)
> +		genlmsg_cancel(skb, ehdr);
> +	else
> +		genlmsg_end(skb, ehdr);
> +
> +	return ret;
> +}

[ ... ]

> +const struct ethnl_request_ops ethnl_port_request_ops = {
> +	.request_cmd		= ETHTOOL_MSG_PORT_GET,
> +	.reply_cmd		= ETHTOOL_MSG_PORT_GET_REPLY,
> +	.hdr_attr		= ETHTOOL_A_PORT_HEADER,
> +	.req_info_size		= sizeof(struct port_req_info),
> +	.reply_data_size	= sizeof(struct port_reply_data),
> +
> +	.parse_request		= port_parse_request,
> +	.prepare_data		= port_prepare_data,
> +	.reply_size		= port_reply_size,
> +	.fill_reply		= port_fill_reply,
> +};