linux-kernel - Re: [PATCH net-next] net: Add support for providing the PTP hardware source in tsinfo

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] [day] [month] [year] [list]

Message-ID: <20250429150657.1f32a10c@kernel.org>
Date: Tue, 29 Apr 2025 15:06:57 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, Donald Hunter <donald.hunter@...il.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
 <horms@...nel.org>, Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Jason Xing <kernelxing@...cent.com>, Richard Cochran
 <richardcochran@...il.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, "Russell King (Oracle)"
 <linux@...linux.org.uk>
Subject: Re: [PATCH net-next] net: Add support for providing the PTP
 hardware source in tsinfo

On Fri, 25 Apr 2025 19:42:43 +0200 Kory Maincent wrote:
> Multi-PTP source support within a network topology has been merged,
> but the hardware timestamp source is not yet exposed to users.
> Currently, users only see the PTP index, which does not indicate
> whether the timestamp comes from a PHY or a MAC.
> 
> Add support for reporting the hwtstamp source using a
> hwtstamp-source field, alongside hwtstamp-phyindex, to describe
> the origin of the hardware timestamp.
> 
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
> ---
> Not sure moving the hwtstamp_source enum to uapi/linux/net_tstamp.h and
> adding this header to ynl/Makefile.deps is the best choice. Maybe it is
> better to move the enum directly to ethtool.h header.

Weak preference for the YAML and therefore ethtool.h from my side.
That way the doc strings will propagate to more places, like the HTML
docs.

> diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> index ff0758e88ea1008efe533cde003b12719bf4fcd3..1414aed0b6adeae15b56e7a99a7d9eeb43ba0b6c 100644
> --- a/include/linux/net_tstamp.h
> +++ b/include/linux/net_tstamp.h
> @@ -13,12 +13,6 @@
>  					 SOF_TIMESTAMPING_TX_HARDWARE | \
>  					 SOF_TIMESTAMPING_RAW_HARDWARE)
>  
> -enum hwtstamp_source {
> -	HWTSTAMP_SOURCE_UNSPEC,

when is unspec used in practice? Only path I could spot that may not
set it is if we fetch the data by PHC index?

> -	HWTSTAMP_SOURCE_NETDEV,
> -	HWTSTAMP_SOURCE_PHYLIB,
> -};
> -
>  /**
>   * struct hwtstamp_provider_desc - hwtstamp provider description
>   *

> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index a93e6ea37fb3a69f331b1c90851d4e68cb659a83..bf5fb9f7acf5c03aaa121e0cda3c0b1d83e49f71 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -13,6 +13,19 @@
>  #include <linux/types.h>
>  #include <linux/socket.h>   /* for SO_TIMESTAMPING */
>  
> +/**
> + * enum hwtstamp_source - Source of the hardware timestamp
> + * @HWTSTAMP_SOURCE_UNSPEC: Source not specified or unknown
> + * @HWTSTAMP_SOURCE_NETDEV: Hardware timestamp comes from the net device

We should probably document that netdev here means that the timestamp
comes from a MAC or device which has MAC and PHY integrated together?

> + * @HWTSTAMP_SOURCE_PHYLIB: Hardware timestamp comes from one of the PHY
> + *			    devices of the network topology
> + */
> +enum hwtstamp_source {
> +	HWTSTAMP_SOURCE_UNSPEC,
> +	HWTSTAMP_SOURCE_NETDEV,
> +	HWTSTAMP_SOURCE_PHYLIB,
> +};

> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -920,12 +920,20 @@ int ethtool_get_ts_info_by_phc(struct net_device *dev,
>  		struct phy_device *phy;
>  
>  		phy = ethtool_phy_get_ts_info_by_phc(dev, info, hwprov_desc);
> -		if (IS_ERR(phy))
> +		if (IS_ERR(phy)) {
>  			err = PTR_ERR(phy);
> -		else
> -			err = 0;
> +			goto out;
> +		}
> +
> +		info->phc_source = HWTSTAMP_SOURCE_PHYLIB;
> +		info->phc_phyindex = phy->phyindex;
> +		err = 0;
> +		goto out;

The goto before the else looks a bit odd now.
Can we return directly in the error cases?
There is no cleanup to be done.

> +	} else {
> +		info->phc_source = HWTSTAMP_SOURCE_NETDEV;
>  	}
>  
> +out:
>  	info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE |
>  				 SOF_TIMESTAMPING_SOFTWARE;
>  
> @@ -947,10 +955,14 @@ int __ethtool_get_ts_info(struct net_device *dev,
>  
>  		ethtool_init_tsinfo(info);
>  		if (phy_is_default_hwtstamp(phydev) &&
> -		    phy_has_tsinfo(phydev))
> +		    phy_has_tsinfo(phydev)) {
>  			err = phy_ts_info(phydev, info);
> -		else if (ops->get_ts_info)
> +			info->phc_source = HWTSTAMP_SOURCE_PHYLIB;
> +			info->phc_phyindex = phydev->phyindex;
> +		} else if (ops->get_ts_info) {
>  			err = ops->get_ts_info(dev, info);
> +			info->phc_source = HWTSTAMP_SOURCE_NETDEV;

Let's move the assignment before the calls if we can?
Otherwise someone adding code below may miss the fact that err may
already be carrying an unhandled error.