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

[Kory Maincent <kory.maincent@bootlin.com>]

Thanks for the review!

On Tue, 29 Apr 2025 15:06:57 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

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

Ack.

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

Indeed it is not used. I think it get merged in one of the several version
tackling the "make PTP selectable support" work. As it is finally not used we
could drop it.
 
> > -	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?

Yes, I will.

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

Yes indeed.

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

Ack.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com