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

Köry Maincent
2023-04-07T08:59:10Z

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