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

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

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 :(

Sorry for my comments coming apart on your review, I'm getting this new
iteration done in-between other tasks  :(

I can't move that check to the prepare_data() step, as we need the
request->port_id field to be populated at that stage for the DUMP
op (for each port, we set request->port_id, then we call prepare_data
and so on).

Maxime