Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 7 Apr 2023 10:58:57 +0200 Köry Maincent wrote:
> > Also let me advertise tools/net/ynl/cli.py to you:
> > https://docs.kernel.org/next/userspace-api/netlink/intro-specs.html  
> 
> Ok I will take look.
> Seems broken on net-next:
> ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/ethtool.yaml --do rings-get --json '{"header":{"dev-index": 18}}'
> Traceback (most recent call last):
>   File "./tools/net/ynl/cli.py", line 52, in <module>
>     main()
>   File "./tools/net/ynl/cli.py", line 31, in main
>     ynl = YnlFamily(args.spec, args.schema)
>   File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 361, in __init__
>     self.family = GenlFamily(self.yaml['name'])
>   File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 331, in __init__
>     self.genl_family = genl_family_name_to_id[family_name]
> KeyError: 'ethtool'

IIRC this usually means ethtool netlink is not selected by you Kconfig.
I should add a clearer error for that I guess.
Booting net-next now, I'll get back to you with a confirmation.

> > Please fill in the new commands in the ethtool spec. Feel free to ask
> > if you have any questions, it's still a bit of a rough.. clay?
> >   
> > > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > > +enum timestamping_layer {
> > > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > > +	SOF_PHY_TIMESTAMPING = (1<<1),    
> > 
> > What does SOF_ stand for? 🤔️  
> 
> It was to follow the naming reference in
> "Documentation/networking/timestamping.rst" like SOF_TIMESTAMPING_RX_HARDWARE
> or SOF_TIMESTAMPING_RX_SOFTAWRE but indeed I do not really understand the SOF
> prefix. 

My default would be to prefix it with ethtool, but who knows maybe one
day it will be used in socket (actually it may get used on the
timestamps themselves?) so it's a judgment call. 

> SocketF?

:D  Yeah, some socketFff feature? flag? 🤷️

> > We need a value for DMA timestamps here.  
> 
> Alright,
> 
> > > +/* TSLIST_GET */
> > > +static int tslist_prepare_data(const struct ethnl_req_info *req_base,
> > > +			       struct ethnl_reply_data *reply_base,
> > > +			       struct genl_info *info)
> > > +{
> > > +	struct ts_reply_data *data = TS_REPDATA(reply_base);
> > > +	struct net_device *dev = reply_base->dev;
> > > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > > +	int ret;
> > > +
> > > +	ret = ethnl_ops_begin(dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	data->ts = 0;
> > > +	if (phy_has_tsinfo(dev->phydev))
> > > +		data->ts = SOF_PHY_TIMESTAMPING;
> > > +	if (ops->get_ts_info)
> > > +		data->ts |= SOF_MAC_TIMESTAMPING;    
> > 
> > We can't make that assumption, that info must come from the driver.  
> 
> Why can't we list the available time stamp like that, can't we just test if they
> have time stamp support. 

On embedded devices this will work, but for integrated NICs there's no
standalone PHY driver. The one vendor driver takes care of all.

Maybe we can do something like:

if (dev->phydev)
	/* your code, it should work for phydev users */
else
	/* vendor driver, we need to call the driver */

Until vendor drivers fill in their implementations we may need to
report some form of unknown source. So we'll either have to add
"unknown" to the source enum or return -EOPNOTSUPP.

> > Also don't we need some way to identify the device / phc from which 
> > the timestamp at the given layer will come?  
> 
> I don't think so, the PHC will be described by the get_ts_info from each
> ethernet driver. Do I miss something?

Right, but then to discover all PHCs the user space would have to
switch the source, read the index, switch the source, read the index,
no? I may just be confused..