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

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

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