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

Maxime Chevallier
2024-09-04T17:45:15Z

Hello Michal,

On Mon, 2 Sep 2024 00:04:39 +0200
Michal Kubecek <mkubecek@suse.cz> wrote:

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

Good point, I'll add one.

>
> > @@ -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.

Indeed, I'll update accordingly.

[...]

> > @@ -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)?

I should better document that, sorry... The phy index assigned starts
at 1 at wraps-around back to 1 when we exhausted the indices, so it
can't ever be 0.

The netlink code in the kernel side interprets the fact that userspace
passes 0 as "use the default PHY", as if you didn't pass the parameter :

net/ethtool/netlink.c:

if (!req_info->phy_index)
return req_info->dev->phydev;

I'll send a patch to document that behaviour, thanks for pointing this
out.

Maxime