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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240621085600.5b7aa934@kernel.org>
Date: Fri, 21 Jun 2024 08:56:00 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>, Broadcom internal
 kernel review list <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn
 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Richard
 Cochran <richardcochran@...il.com>, Radu Pirea
 <radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, Andy
 Gospodarek <andy@...yhouse.net>, Nicolas Ferre
 <nicolas.ferre@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jonathan Corbet
 <corbet@....net>, Horatiu Vultur <horatiu.vultur@...rochip.com>,
 UNGLinuxDriver@...rochip.com, Simon Horman <horms@...nel.org>, Vladimir
 Oltean <vladimir.oltean@....com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>
Subject: Re: [PATCH net-next v15 13/14] net: ethtool: tsinfo: Add support
 for hwtstamp provider and get/set hwtstamp config

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.