Re: [PATCH RFC net-next v8 04/13] net: Change the API of PHY default timestamp to MAC

Florian Fainelli
2024-02-16T18:52:55Z

On 2/16/24 07:52, 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.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>
> 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.
> ---
> 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;
> +

This probably does not matter too much given that the mii_ts is not
visible until we fully probed the PHY, though for consistency and to be
on the safe side, it would be more prudent to set default_timestamp
before finishing the mii_ts assignment, in case we ever become more
aggressive at exposing objects to user-space/kernel-space. Probably over
thinking this.

More comments below:

[snip]

>
> - if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
> + if (!skb->dev)
> + return false;
> +
> + if (!phy_is_default_hwtstamp(skb->dev->phydev))

Was not obvious that we could remove the phydev NULL check, but it's
fine because phy_is_default_hwtstamp() calls phy_has_hwtstamp() first,
and that function has that check. Seems a bit brittle, but fair enough.
--
Florian