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

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

On 10/1/26 00:53, Jakub Kicinski wrote:
> 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?

From my memories on early tries to get it done another way, this was
tricky as I was struggling to find the correct place to put the array
in question.

phy_names() is required even when CONFIG_NET=n as it's used for devicetree
parsing, so we can't put that in phylib nor in any networking code without
doing some trickery to get some of it to build inconditionally. I'll try
harder :)

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

Ack, will do

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

ah indeed yes :(

Thanks for reviewing,

Maxime