Re: [PATCH net-next v17 04/14] net: Change the API of PHY default timestamp to MAC

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

On Mon, 15 Jul 2024 16:37:01 -0700
Jacob Keller <jacob.e.keller@intel.com> wrote:

> On 7/9/2024 6:53 AM, Kory Maincent wrote:
> > Change the API to select MAC default time stamping instead of the PHY.
> > Indeed the PHY is closer to the wire therefore theoretically it has less
> > delay than the MAC timestamping but the reality is different. Due to lower
> > time stamping clock frequency, latency in the MDIO bus and no PHC hardware
> > synchronization between different PHY, the PHY PTP is often less precise
> > than the MAC. The exception is for PHY designed specially for PTP case but
> > these devices are not very widespread. For not breaking the compatibility
> > default_timestamp flag has been introduced in phy_device that is set by
> > the phy driver to know we are using the old API behavior.
> >   
> 
> This description feels like it is making a pretty broad generalization
> about devices. The specifics of whether MAC or PHY timestamping is
> better will be device dependent.

As explained, except for specific PTP specialized PHY, the MAC is better in
term of PTP precision.
This patch was a requisite from Russell, who wanted to add support for the PTP
in the marvell PHY. Doing so would select the PHY PTP by default which cause a
regression as the PHY hardware timestamp is less precise than the MAC.
https://lore.kernel.org/netdev/20200729105807.GZ1551@shell.armlinux.org.uk/
https://lore.kernel.org/netdev/Y%2F4DZIDm1d74MuFJ@shell.armlinux.org.uk/
There is also discussion on how to support it in older version of this series.
 
> It looks like you introduce a default_timestamp flag to ensure existing
> devices default to PHY? I assume your goal here is to discourage this
> and not allow setting it for new devices? Or do we want to let device
> driver authors decide which is a better default?

Yes to not change the old behavior the current PHY with PTP support will still
behave as default PTP. The point is indeed to discourage future drivers to
select the PHY as default PTP.

> > Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>  
> 
> Overall this makes sense, with a couple questions I had during review.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> > ---
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index bd68f9d8e74f..e7a38137211c 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -616,6 +616,8 @@ struct macsec_ops;
> >   *                 handling shall be postponed until PHY has resumed
> >   * @irq_rerun: Flag indicating interrupts occurred while PHY was suspended,
> >   *             requiring a rerun of the interrupt handler after resume
> > + * @default_timestamp: Flag indicating whether we are using the phy
> > + *		       timestamp as the default one  
> 
> This is clearly intended to ensure existing drivers maintain legacy
> behavior. But what is our policy going forward for new devices? Do we
> want to leave it up to PHY driver authors?

Yes, new devices should not set this flag.

> > diff --git a/net/core/timestamping.c b/net/core/timestamping.c
> > index 04840697fe79..3717fb152ecc 100644
> > --- a/net/core/timestamping.c
> > +++ b/net/core/timestamping.c
> > @@ -25,7 +25,8 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
> >  	struct sk_buff *clone;
> >  	unsigned int type;
> >  
> > -	if (!skb->sk)
> > +	if (!skb->sk || !skb->dev ||
> > +	    !phy_is_default_hwtstamp(skb->dev->phydev))  
> 
> I don't follow why this check is added and its not calling something
> like "phy_is_current_hwtstamp"? I guess because we don't yet have a way
> to select between MAC/PHY at this point in the series? Ok.

skb_clone_tx_timestamp is only used for PHY timestamping so we should do nothing
if the default PTP is the MAC.

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