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

Michal Kubecek
2024-09-01T22:04:42Z

[-- 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 --]