On 7/27/2024 6:44 AM, Kory Maincent wrote: > On Mon, 15 Jul 2024 16:37:01 -0700 > Jacob Keller <jacob.e.keller@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx/ > https://lore.kernel.org/netdev/Y%2F4DZIDm1d74MuFJ@xxxxxxxxxxxxxxxxxxxxx/ > There is also discussion on how to support it in older version of this series. > Right. So it is a bit of a generalization, but in practice it matches up with the available hardware on the market. >> 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. > Ok great! >>> 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. > I guess my misunderstanding is what about the case where user selects PHY timestamping with the netlink command? Then it would still need to do the skb_clone_tx_timestamp even though its not the default? Or does phy_is_default_hwtstamp take that into account? In which case it would make more sense to name it phy_is_current_hwtstamp. Either way this is mostly bikeshedding and probably just some misunderstanding in my reading of the code. Thanks, Jake > Regards,
