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

["Köry Maincent" <kory.maincent@bootlin.com>]

Thanks for the review.

On Thu, 6 Apr 2023 18:46:46 -0700
Jakub Kicinski <kuba@kernel.org> wrote:
 
> Please put some words in here :) Documentation matters to the users.

Ok.

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


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

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

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

Köry