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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Jakub,

Thanks a lot for taking a look at this :)

On 13/03/2026 03:07, Jakub Kicinski wrote:
> 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;
>> +}

I agree with all you comments, I'll address for next round :)

Maxime