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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e4de7c23-ffee-42f6-aba8-b10f3d44f22c@intel.com>
Date: Mon, 29 Jul 2024 11:08:01 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Kory Maincent <kory.maincent@...tlin.com>
CC: Florian Fainelli <florian.fainelli@...adcom.com>, "Broadcom internal
 kernel review list" <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn
	<andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
	<linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Richard Cochran <richardcochran@...il.com>, Radu Pirea
	<radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, "Andy
 Gospodarek" <andy@...yhouse.net>, Nicolas Ferre
	<nicolas.ferre@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jonathan Corbet
	<corbet@....net>, "Horatiu Vultur" <horatiu.vultur@...rochip.com>,
	<UNGLinuxDriver@...rochip.com>, "Simon Horman" <horms@...nel.org>, Vladimir
 Oltean <vladimir.oltean@....com>, <donald.hunter@...il.com>,
	<danieller@...dia.com>, <ecree.xilinx@...il.com>, Thomas Petazzoni
	<thomas.petazzoni@...tlin.com>, <linux-kernel@...r.kernel.org>,
	<netdev@...r.kernel.org>, <linux-doc@...r.kernel.org>, Maxime Chevallier
	<maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>,
	Willem de Bruijn <willemb@...gle.com>, Shannon Nelson
	<shannon.nelson@....com>, Alexandra Winter <wintera@...ux.ibm.com>
Subject: Re: [PATCH net-next v17 04/14] net: Change the API of PHY default
 timestamp to MAC



On 7/27/2024 6:44 AM, Kory Maincent wrote:
> On Mon, 15 Jul 2024 16:37:01 -0700
> Jacob Keller <jacob.e.keller@...el.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,