Re: [PATCH net-next v7 10/10] net: ethtool: Introduce ethtool command to list ports
From: Maxime Chevallier
Date: Fri Mar 13 2026 - 04:39:01 EST
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] = ðnl_tsconfig_request_ops,
>> [ETHTOOL_MSG_PHY_GET] = ðnl_phy_request_ops,
>> [ETHTOOL_MSG_MSE_GET] = ðnl_mse_request_ops,
>> + [ETHTOOL_MSG_PORT_GET] = ðnl_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