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@xxxxxxxxxxx>
---
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
