On Fri, 16 Feb, 2024 16:52:22 +0100 Kory Maincent <kory.maincent@xxxxxxxxxxx> 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.
>
> Signed-off-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> ---
Overall, I agree with the motivation and reasoning behind the patch. It
takes dedicated effort to build a good phy timestamping mechanism, so
this approach is good. I do have a question though. In this patch if we
set the phy as the default timestamp mechanism, does that mean for even
non-PTP applications, the phy will be used for timestamping when
hardware timestamping is enabled? If so, I think this might need some
thought because there are timing applications in general when a
timestamp closest to the MAC layer would be best.
>
> Changes in v5:
> - Extract the API change in this patch.
> - Rename whitelist to allowlist.
> - Set NETDEV_TIMESTAMPING in register_netdevice function.
> - Add software timestamping case description in ts_info.
>
> Change in v6:
> - Replace the allowlist phy with a default_timestamp flag to know which
> phy is using old API behavior.
> - Fix dereferenced of a possible null pointer.
> - Follow timestamping layer naming update.
> - Update timestamp default set between MAC and software.
> - Update ts_info returned in case of software timestamping.
>
> Change in v8:
> - Reform the implementation to use a simple phy_is_default_hwtstamp helper
> instead of saving the hwtstamp in the net_device struct.
> ---
One general concern
> drivers/net/phy/bcm-phy-ptp.c | 3 +++
> drivers/net/phy/dp83640.c | 3 +++
> drivers/net/phy/micrel.c | 6 ++++++
> drivers/net/phy/mscc/mscc_ptp.c | 3 +++
> drivers/net/phy/nxp-c45-tja11xx.c | 3 +++
> include/linux/phy.h | 17 +++++++++++++++++
> net/core/dev_ioctl.c | 8 +++-----
> net/core/timestamping.c | 10 ++++++++--
> net/ethtool/common.c | 2 +-
> 9 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
> index 617d384d4551..d3e825c951ee 100644
> --- a/drivers/net/phy/bcm-phy-ptp.c
> +++ b/drivers/net/phy/bcm-phy-ptp.c
> @@ -931,6 +931,9 @@ struct bcm_ptp_private *bcm_ptp_probe(struct phy_device *phydev)
> return ERR_CAST(clock);
> priv->ptp_clock = clock;
>
> + /* Timestamp selected by default to keep legacy API */
> + phydev->default_timestamp = true;
> +
> priv->phydev = phydev;
> bcm_ptp_init(priv);
>
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index 5c42c47dc564..64fd1a109c0f 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -1450,6 +1450,9 @@ static int dp83640_probe(struct phy_device *phydev)
> phydev->mii_ts = &dp83640->mii_ts;
> phydev->priv = dp83640;
>
> + /* Timestamp selected by default to keep legacy API */
> + phydev->default_timestamp = true;
> +
> spin_lock_init(&dp83640->rx_lock);
> skb_queue_head_init(&dp83640->rx_queue);
> skb_queue_head_init(&dp83640->tx_queue);
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 9b6973581989..1c9eba331b01 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -3177,6 +3177,9 @@ static void lan8814_ptp_init(struct phy_device *phydev)
> ptp_priv->mii_ts.ts_info = lan8814_ts_info;
>
> phydev->mii_ts = &ptp_priv->mii_ts;
> +
> + /* Timestamp selected by default to keep legacy API */
> + phydev->default_timestamp = true;
> }
>
> static int lan8814_ptp_probe_once(struct phy_device *phydev)
> @@ -4613,6 +4616,9 @@ static int lan8841_probe(struct phy_device *phydev)
>
> phydev->mii_ts = &ptp_priv->mii_ts;
>
> + /* Timestamp selected by default to keep legacy API */
> + phydev->default_timestamp = true;
> +
> return 0;
> }
>
> diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> index eb0b032cb613..e66d20eff7c4 100644
> --- a/drivers/net/phy/mscc/mscc_ptp.c
> +++ b/drivers/net/phy/mscc/mscc_ptp.c
> @@ -1570,6 +1570,9 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
> return PTR_ERR(vsc8531->load_save);
> }
>
> + /* Timestamp selected by default to keep legacy API */
> + phydev->default_timestamp = true;
> +
> vsc8531->ptp->phydev = phydev;
>
> return 0;
> diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
> index 3cf614b4cd52..d18c133e6013 100644
> --- a/drivers/net/phy/nxp-c45-tja11xx.c
> +++ b/drivers/net/phy/nxp-c45-tja11xx.c
> @@ -1660,6 +1660,9 @@ static int nxp_c45_probe(struct phy_device *phydev)
> priv->mii_ts.ts_info = nxp_c45_ts_info;
> phydev->mii_ts = &priv->mii_ts;
> ret = nxp_c45_init_ptp_clock(priv);
> +
> + /* Timestamp selected by default to keep legacy API */
> + phydev->default_timestamp = true;
> } else {
> phydev_dbg(phydev, "PTP support not enabled even if the phy supports it");
> }
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index c2dda21b39e1..9a31243e9f7e 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -607,6 +607,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
> * @interface: enum phy_interface_t value
> * @possible_interfaces: bitmap if interface modes that the attached PHY
> * will switch between depending on media speed.
> @@ -672,6 +674,8 @@ struct phy_device {
> unsigned irq_suspended:1;
> unsigned irq_rerun:1;
>
> + unsigned default_timestamp:1;
> +
> int rate_matching;
>
> enum phy_state state;
> @@ -1613,6 +1617,19 @@ static inline void phy_txtstamp(struct phy_device *phydev, struct sk_buff *skb,
> phydev->mii_ts->txtstamp(phydev->mii_ts, skb, type);
> }
>
> +/**
> + * phy_is_default_hwtstamp - return true if phy is the default hw timestamp
> + * @phydev: Pointer to phy_device
> + *
> + * This is used to get default timestamping device taking into account
> + * the new API choice, which is selecting the timestamping from MAC by
> + * default if the phydev does not have default_timestamp flag enabled.
> + */
> +static inline bool phy_is_default_hwtstamp(struct phy_device *phydev)
> +{
> + return phy_has_hwtstamp(phydev) && phydev->default_timestamp;
> +}
> +
> /**
> * phy_is_internal - Convenience function for testing if a PHY is internal
> * @phydev: the phy_device struct
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 847254fd7f13..3342834597cd 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -260,9 +260,7 @@ static int dev_eth_ioctl(struct net_device *dev,
> * @dev: Network device
> * @cfg: Timestamping configuration structure
> *
> - * Helper for enforcing a common policy that phylib timestamping, if available,
> - * should take precedence in front of hardware timestamping provided by the
> - * netdev.
> + * Helper for calling the default hardware provider timestamping.
> *
> * Note: phy_mii_ioctl() only handles SIOCSHWTSTAMP (not SIOCGHWTSTAMP), and
> * there only exists a phydev->mii_ts->hwtstamp() method. So this will return
> @@ -272,7 +270,7 @@ static int dev_eth_ioctl(struct net_device *dev,
> int dev_get_hwtstamp_phylib(struct net_device *dev,
> struct kernel_hwtstamp_config *cfg)
> {
> - if (phy_has_hwtstamp(dev->phydev))
> + if (phy_is_default_hwtstamp(dev->phydev))
> return phy_hwtstamp_get(dev->phydev, cfg);
>
> return dev->netdev_ops->ndo_hwtstamp_get(dev, cfg);
> @@ -329,7 +327,7 @@ int dev_set_hwtstamp_phylib(struct net_device *dev,
> struct netlink_ext_ack *extack)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
> - bool phy_ts = phy_has_hwtstamp(dev->phydev);
> + bool phy_ts = phy_is_default_hwtstamp(dev->phydev);
> struct kernel_hwtstamp_config old_cfg = {};
> bool changed = false;
> int err;
> diff --git a/net/core/timestamping.c b/net/core/timestamping.c
> index 04840697fe79..891bfc2f62fd 100644
> --- a/net/core/timestamping.c
> +++ b/net/core/timestamping.c
> @@ -25,7 +25,10 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
> struct sk_buff *clone;
> unsigned int type;
>
> - if (!skb->sk)
> + if (!skb->sk || !skb->dev)
> + return;
> +
> + if (!phy_is_default_hwtstamp(skb->dev->phydev))
Really minor but any reason to not just keep the conditional chaining
with a single if statement?
> return;
>
> type = classify(skb);
> @@ -47,7 +50,10 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
> struct mii_timestamper *mii_ts;
> unsigned int type;
>
> - if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
> + if (!skb->dev)
> + return false;
> +
> + if (!phy_is_default_hwtstamp(skb->dev->phydev))
Same here
if (!skb->dev || !phy_is_default_hwtstamp(skb->dev->phydev))
> return false;
>
> if (skb_headroom(skb) < ETH_HLEN)
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index ce486cec346c..e56bde53cd5c 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -637,7 +637,7 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
> memset(info, 0, sizeof(*info));
> info->cmd = ETHTOOL_GET_TS_INFO;
>
> - if (phy_has_tsinfo(phydev))
> + if (phy_is_default_hwtstamp(phydev) && phy_has_tsinfo(phydev))
> return phy_ts_info(phydev, info);
> if (ops->get_ts_info)
> return ops->get_ts_info(dev, info);
--
Thanks,
Rahul Rameshbabu
