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

[Jakub Kicinski <kuba@kernel.org>]

On Thu,  6 Apr 2023 19:33:05 +0200 Köry Maincent wrote:
> +Gets transceiver module parameters.
> +
> +Request contents:
> +
> +  =================================  ======  ==========================
> +  ``ETHTOOL_A_TS_HEADER``            nested  request header
> +  =================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +  =======================  ======  ====================================
> +  ``ETHTOOL_A_TS_HEADER``  nested  reply header
> +  ``ETHTOOL_A_TS_LAYER``   u32     current hardware timestamping
> +  =======================  ======  ====================================
> +
> +TSLIST_GET
> +==========
> +
> +Gets transceiver module parameters.
> +
> +Request contents:
> +
> +  =================================  ======  ==========================
> +  ``ETHTOOL_A_TS_HEADER``            nested  request header
> +  =================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +  =======================  ======  ===================================
> +  ``ETHTOOL_A_TS_HEADER``  nested  reply header
> +  ``ETHTOOL_A_TS_LAYER``   u32     available hardware timestamping
> +  =======================  ======  ===================================

Please put some words in here :) Documentation matters to the users.

Also let me advertise tools/net/ynl/cli.py to you:
https://docs.kernel.org/next/userspace-api/netlink/intro-specs.html

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

We need a value for DMA timestamps here.

> +/* 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.

Also don't we need some way to identify the device / phc from which 
the timestamp at the given layer will come?