Re: [PATCH net-next v15 13/14] net: ethtool: tsinfo: Add support for hwtstamp provider and get/set hwtstamp config

[Jakub Kicinski]

On Fri, 21 Jun 2024 10:54:08 +0200 Kory Maincent wrote:
> > > +const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1]
> > > = { [ETHTOOL_A_TSINFO_HEADER]		=
> > >  		NLA_POLICY_NESTED(ethnl_header_policy_stats),
> > > +	[ETHTOOL_A_TSINFO_GHWTSTAMP] =
> > > +		NLA_POLICY_MAX(NLA_U8, 1),    
> > 
> > I think this can be an NLA_FLAG, but TBH I'm also confused about 
> > the semantics. Can you explain what it does from user perspective?  
> 
> As I described it in the documentation it replaces SIOCGHWTSTAMP:
> "Any process can read the actual configuration by requesting tsinfo netlink
> socket ETHTOOL_MSG_TSINFO_GET with ETHTOOL_MSG_TSINFO_GHWTSTAMP netlink
> attribute set.
> 
> The legacy usage is to pass this structure to ioctl(SIOCGHWTSTAMP) in the       
> same way as the ioctl(SIOCSHWTSTAMP).  However, this has not been implemented   
> in all drivers."

I did see the words, just didn't get the meaning :> Couple of years
from now hopefully newcomers won't even know ioctls exited, and
therefore what they did. From the user perspective the gist AFAIU is
that instead of *supported* we'll return what's currently *configured*.

This feels a little bit too much like a muxed operation for me :(
Can we create a separate commands for TSCONFIG_GET / _SET ?
Granted it will be higher LOC, but hopefully cleaner ?
Or we can add the configured as completely new attrs, but changing
meaning of existing attrs based on a request flag.. 🙂‍↔️️

> > > +	[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER] =
> > > +		NLA_POLICY_NESTED(ethnl_tsinfo_hwtstamp_provider_policy),
> > >  };
> > >  
> > > +static int tsinfo_parse_hwtstamp_provider(const struct nlattr *nest,
> > > +					  struct hwtst_provider *hwtst,
> > > +					  struct netlink_ext_ack *extack,
> > > +					  bool *mod)
> > > +{
> > > +	struct nlattr
> > > *tb[ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy)];    
> > 
> > Could you find a more sensible name for this policy?  
> 
> I am not a naming expert but "hwtstamp_provider" is the struct name I have used
> to describe hwtstamp index + qualifier and the prefix of the netlink nested
> attribute, so IMHO it fits well.
> Have you another proposition to clarify what you would expect?

Oh, I just meant that it's way to long. I know y'all youngsters use
IDEs but I have it on good authority that there's still people in
this community who use text editors they wrote themselves, and those
lack auto-completion.. It's good to be more concise.