Re: [PATCH net-next v7 10/10] net: ethtool: Introduce ethtool command to list ports

[Jakub Kicinski <kuba@kernel.org>]

On Mon,  9 Mar 2026 16:27:46 +0100 Maxime Chevallier wrote:
> Expose the phy_port information to userspace, so that we can know how
> many ports are available on a given interface, as well as their
> capabilities. For MDI ports, we report the list of supported linkmodes
> based on what the PHY that drives this port says.
> For MII ports, i.e. empty SFP cages, we report the MII linkmodes that we
> can output on this port.

> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 6e5f0f4f815a..90674aed7777 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -18,6 +18,8 @@ static u32 ethnl_bcast_seq;
>  			     ETHTOOL_FLAG_OMIT_REPLY)
>  #define ETHTOOL_FLAGS_STATS (ETHTOOL_FLAGS_BASIC | ETHTOOL_FLAG_STATS)
>  
> +char phy_interface_names[PHY_INTERFACE_MODE_MAX][ETH_GSTRING_LEN] __ro_after_init;
> +
>  const struct nla_policy ethnl_header_policy[] = {
>  	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
>  	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
> @@ -421,6 +423,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
>  	[ETHTOOL_MSG_TSCONFIG_SET]	= &ethnl_tsconfig_request_ops,
>  	[ETHTOOL_MSG_PHY_GET]		= &ethnl_phy_request_ops,
>  	[ETHTOOL_MSG_MSE_GET]		= &ethnl_mse_request_ops,
> +	[ETHTOOL_MSG_PORT_GET]		= &ethnl_port_request_ops,
>  };
>  
>  static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
> @@ -1544,6 +1547,16 @@ static const struct genl_ops ethtool_genl_ops[] = {
>  		.policy = ethnl_mse_get_policy,
>  		.maxattr = ARRAY_SIZE(ethnl_mse_get_policy) - 1,
>  	},
> +	{
> +		.cmd	= ETHTOOL_MSG_PORT_GET,
> +		.doit	= ethnl_default_doit,
> +		.start	= ethnl_port_dump_start,
> +		.dumpit	= ethnl_port_dumpit,
> +		.done	= ethnl_port_dump_done,
> +		.policy = ethnl_port_get_policy,
> +		.maxattr = ARRAY_SIZE(ethnl_port_get_policy) - 1,
> +	},
> +

spurious nl

>  };
>  

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2026 Bootlin
> + *
> + */

so many lines for a copyright :(

> +#include "common.h"
> +#include "bitset.h"
> +#include "netlink.h"

alpha sort

> +#define PORT_REQINFO(__req_base) \
> +	container_of(__req_base, struct port_req_info, base)
> +
> +#define PORT_REPDATA(__reply_base) \
> +	container_of(__reply_base, struct port_reply_data, base)
> +
> +const struct nla_policy ethnl_port_get_policy[ETHTOOL_A_PORT_ID + 1] = {
> +	[ETHTOOL_A_PORT_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_PORT_ID] = { .type = NLA_U32},

misaligned bracket

> +};
> +
> +static int port_parse_request(struct ethnl_req_info *req_info,
> +			      struct nlattr **tb,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct port_req_info *request = PORT_REQINFO(req_info);
> +
> +	/* PORT id is required for GET requests */
> +	if (tb[ETHTOOL_A_PORT_ID])

GENL_REQ_ATTR_CHECK ?
I guess you'll have to move it to prepare cause we don't get info 
here :(

> +		request->port_id = nla_get_u32(tb[ETHTOOL_A_PORT_ID]);
> +
> +	if (!request->port_id) {

NLA_POLICY_MIN(, 1)

> +		NL_SET_ERR_MSG(extack, "port id missing");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}

> +	/* ETHTOOL_A_PORT_TYPE */
> +	size += nla_total_size(sizeof(u8));

please don't use u8, it wastes 3B on padding

> +	/* ETHTOOL_A_PORT_VACANT */
> +	size += nla_total_size(sizeof(u8));
> +
> +	return size;
> +}
> +
> +static int port_fill_reply(struct sk_buff *skb,
> +			   const struct ethnl_req_info *req_info,
> +			   const struct ethnl_reply_data *reply_data)
> +{
> +	bool compact = req_info->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
> +	struct port_reply_data *reply = PORT_REPDATA(reply_data);
> +	int ret, port_type = ETHTOOL_PORT_TYPE_MDI;
> +
> +	if (nla_put_u32(skb, ETHTOOL_A_PORT_ID, reply->port_id))
> +		return -EMSGSIZE;
> +
> +	if (!reply->mii) {
> +		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_MODES,
> +				       reply->supported, NULL,
> +				       __ETHTOOL_LINK_MODE_MASK_NBITS,
> +				       link_mode_names, compact);
> +		if (ret < 0)
> +			return -EMSGSIZE;

return ret?

> +static int port_dump_all_dev(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
> +	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, ctx->ifindex) {
> +		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
> +		rcu_read_unlock();
> +
> +		ctx->req_info->base.dev = dev;
> +		ret = port_dump_one_dev(skb, cb);
> +
> +		rcu_read_lock();
> +		netdev_put(dev, &dev_tracker);
> +		ctx->req_info->base.dev = NULL;
> +
> +		if (ret < 0 && ret != -EOPNOTSUPP) {
> +			if (likely(skb->len))
> +				ret = skb->len;

Why the skb->len check? netlink plumbing should handle that these days
if err = -EMSGSIZE during dump.

> +			break;
> +		}
> +		ret = 0;
> +	}
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +
> +int ethnl_port_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
> +	int ret = 0;
> +
> +	if (ctx->ifindex) {
> +		netdevice_tracker dev_tracker;
> +		struct net_device *dev;
> +
> +		dev = netdev_get_by_index(genl_info_net(&info->info),
> +					  ctx->ifindex, &dev_tracker,
> +					  GFP_KERNEL);
> +		if (!dev)
> +			return -ENODEV;
> +
> +		ctx->req_info->base.dev = dev;
> +		ret = port_dump_one_dev(skb, cb);
> +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> +			ret = skb->len;

ditto

> +		netdev_put(dev, &dev_tracker);
> +	} else {
> +		ret = port_dump_all_dev(skb, cb);
> +	}
> +
> +	return ret;
> +}