Re: [PATCH net-next v17 04/14] net: Change the API of PHY default timestamp to MAC

[Jacob Keller <jacob.e.keller@intel.com>]

On 7/27/2024 6:44 AM, Kory Maincent wrote:
> On Mon, 15 Jul 2024 16:37:01 -0700
> Jacob Keller <jacob.e.keller@intel.com> 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@shell.armlinux.org.uk/
> https://lore.kernel.org/netdev/Y%2F4DZIDm1d74MuFJ@shell.armlinux.org.uk/
> 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,