Messages in this thread | | | Date | Fri, 16 Feb 2024 10:52:50 -0800 | Subject | Re: [PATCH RFC net-next v8 04/13] net: Change the API of PHY default timestamp to MAC | From | Florian Fainelli <> |
| |
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
| |