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

[Jakub Kicinski <kuba@kernel.org>]

On Sun, 27 Sep 2026 15:36:18 +0200 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.
> 
> Tested-by: Aleksei Sviridkin <f@lex.la>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

> +static void __init ethnl_phy_names_populate(void)
> +{
> +	const char *name;
> +	int i;
> +
> +	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
> +		name = phy_modes(i);
> +		strscpy(phy_interface_names[i], name, ETH_GSTRING_LEN);

Feels a bit backward, we run thru a switch statement to dump it down
to an array.. Why not convert to an array and then make phy_modes()
also use it?

> +	}
> +}

> +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 ret;
> +	} else {
> +		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_INTERFACES,
> +				       reply->interfaces, NULL,
> +				       PHY_INTERFACE_MODE_MAX,
> +				       phy_interface_names, compact);
> +		if (ret < 0)
> +			return ret;
> +	}
> +

please move the port_type init from inline up to to here.
initializing things inline often lowers readability

> +	if (reply->mii || reply->sfp)
> +		port_type = ETHTOOL_PORT_TYPE_SFP;
> +
> +	if (nla_put_u32(skb, ETHTOOL_A_PORT_TYPE, port_type))
> +		return -EMSGSIZE;
> +
> +	if (reply->upstream_port &&
> +	    nla_put_u32(skb, ETHTOOL_A_PORT_UPSTREAM_PORT,
> +			reply->upstream_port))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}

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

smells of pre-commit b5a899154 netlink
Can we delete the skb->len check and override here?
There's one more such construct below

> +			break;
> +		}
> +
> +		ret = 0;