Re: [PATCH ethtool-next v2 2/3] ethtool: Allow passing a PHY index for phy-targetting commands

[Michal Kubecek <mkubecek@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 3654 bytes --]

On Wed, Aug 28, 2024 at 05:25:09PM +0200, Maxime Chevallier wrote:
> With the introduction of PHY topology and the ability to list PHYs, we
> can now target some netlink commands to specific PHYs. This is done by
> passing a PHY index as a request parameter in the netlink GET command.
> 
> This is useful for PSE-PD, PLCA and Cable-testing operations when
> multiple PHYs are on the link (e.g. when a PHY is used as an SFP
> upstream controller, and when there's another PHY within the SFP
> module).
> 
> Introduce a new, generic, option "--phy N" that can be used in
> conjunction with PHY-targetting commands to pass the PHY index for the
> targetted PHY.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  ethtool.8.in         | 20 +++++++++++++++++
>  ethtool.c            | 25 ++++++++++++++++++++-
>  internal.h           |  1 +
>  netlink/cable_test.c |  4 ++--
>  netlink/msgbuff.c    | 52 ++++++++++++++++++++++++++++++++++----------
>  netlink/msgbuff.h    |  3 +++
>  netlink/nlsock.c     |  3 ++-
>  netlink/plca.c       |  4 ++--
>  netlink/pse-pd.c     |  4 ++--
>  9 files changed, 96 insertions(+), 20 deletions(-)
> 
[...]
> @@ -6550,6 +6559,16 @@ int main(int argc, char **argp)
>  			argc -= 1;
>  			continue;
>  		}
> +		if (*argp && !strcmp(*argp, "--phy")) {
> +			char *eptr;
> +
> +			ctx.phy_index = strtoul(argp[1], &eptr, 0);
> +			if (!argp[1][0] || *eptr)
> +				exit_bad_args();
> +			argp += 2;
> +			argc -= 2;
> +			continue;
> +		}
>  		break;
>  	}
>  	if (*argp && !strcmp(*argp, "--monitor")) {

Could we have a meaningful error message that would tell user what was
wrong instead?

> @@ -6585,6 +6604,10 @@ int main(int argc, char **argp)
>  	}
>  	if (ctx.json && !args[k].json)
>  		exit_bad_args_info("JSON output not available for this subcommand");
> +
> +	if (!args[k].targets_phy && ctx.phy_index)
> +		exit_bad_args();
> +
>  	ctx.argc = argc;
>  	ctx.argp = argp;
>  	netlink_run_handler(&ctx, args[k].nlchk, args[k].nlfunc, !args[k].func);

Same here.

[...]
> diff --git a/netlink/msgbuff.c b/netlink/msgbuff.c
> index 216f5b9..2275840 100644
> --- a/netlink/msgbuff.c
> +++ b/netlink/msgbuff.c
> @@ -138,17 +138,9 @@ struct nlattr *ethnla_nest_start(struct nl_msg_buff *msgbuff, uint16_t type)
>  	return NULL;
>  }
>  
> -/**
> - * ethnla_fill_header() - write standard ethtool request header to message
> - * @msgbuff: message buffer
> - * @type:    attribute type for header nest
> - * @devname: device name (NULL to omit)
> - * @flags:   request flags (omitted if 0)
> - *
> - * Return: pointer to the nest attribute or null of error
> - */
> -bool ethnla_fill_header(struct nl_msg_buff *msgbuff, uint16_t type,
> -			const char *devname, uint32_t flags)
> +static bool __ethnla_fill_header_phy(struct nl_msg_buff *msgbuff, uint16_t type,
> +				     const char *devname, uint32_t phy_index,
> +				     uint32_t flags)
>  {
>  	struct nlattr *nest;
>  
> @@ -159,7 +151,9 @@ bool ethnla_fill_header(struct nl_msg_buff *msgbuff, uint16_t type,
>  	if ((devname &&
>  	     ethnla_put_strz(msgbuff, ETHTOOL_A_HEADER_DEV_NAME, devname)) ||
>  	    (flags &&
> -	     ethnla_put_u32(msgbuff, ETHTOOL_A_HEADER_FLAGS, flags)))
> +	     ethnla_put_u32(msgbuff, ETHTOOL_A_HEADER_FLAGS, flags)) ||
> +	    (phy_index &&
> +	     ethnla_put_u32(msgbuff, ETHTOOL_A_HEADER_PHY_INDEX, phy_index)))
>  		goto err;
>  
>  	ethnla_nest_end(msgbuff, nest);

Just to be sure: are we sure the PHY index cannot ever be zero (or that
we won't need to pass 0 index to kernel)?

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]