Re: [PATCH RFC net-next v8 04/13] net: Change the API of PHY default timestamp to MAC

[Rahul Rameshbabu]

On Mon, 19 Feb, 2024 16:11:16 +0000 "Russell King (Oracle)" <linux@xxxxxxxxxxxxxx> wrote:
> On Mon, Feb 19, 2024 at 02:29:36PM +0100, Köry Maincent wrote:
>> On Fri, 16 Feb 2024 10:09:36 -0800
>> Rahul Rameshbabu <rrameshbabu@xxxxxxxxxx> wrote:
>> 
>> > On Fri, 16 Feb, 2024 16:52:22 +0100 Kory Maincent <kory.maincent@xxxxxxxxxxx>
>> > 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>
>> > > ---  
>> > 
>> > Overall, I agree with the motivation and reasoning behind the patch. It
>> > takes dedicated effort to build a good phy timestamping mechanism, so
>> > this approach is good. I do have a question though. In this patch if we
>> > set the phy as the default timestamp mechanism, does that mean for even
>> > non-PTP applications, the phy will be used for timestamping when
>> > hardware timestamping is enabled? If so, I think this might need some
>> > thought because there are timing applications in general when a
>> > timestamp closest to the MAC layer would be best.
>> 
>> This patch comes from a request from Russell due to incompatibility between MAC
>> and PHY timestamping when both were supported.
>> https://lore.kernel.org/netdev/Y%2F4DZIDm1d74MuFJ@xxxxxxxxxxxxxxxxxxxxx/
>> 
>> His point was adding PTP support to a PHY driver would select timestamp from it
>> by default even if we had a better timestamp with the MAC which is often the
>> case. This is an unwanted behavior.
>> https://lore.kernel.org/netdev/Y%2F6Cxf6EAAg22GOL@xxxxxxxxxxxxxxxxxxxxx/

Thanks for providing the additional context. This was helpful. Btw, I
absolutely agree that all PHYs should not be defaulted to for
timestamping applications. At best, a driver implementer can select that
the PHY will primarily be used in timestamping applications that benefit
from it doing the timestamp work (which is what this patch does).

>>
>> In fact, with the new support of NDOs hwtstamp and the
>> dev_get/set_hwtstamp_phylib functions, alongside this series which make
>> timestamp selectable, changing the default timestamp may be not necessary
>> anymore.
>> 
>> Russell any thought about it? 
>
> My position remains: in the case of Marvell PP2 network driver with a
> Marvell PHY, when we add PTP support for the Marvell PHYs (I have
> patches for it for years) then we must _not_ regress the existing
> setup where the PP2 timestamps are the default.

I agree 100% that the previous default of the PP2 (DMA) timestamps for
the Marvell PP2 network driver should not break from this work. We
similarly would not want the same in mlx5 in general (today the default
is DMA timestamping except for PTP traffic which we have selection rules
based on the packet). This patch is essentially to avoid all PHYs
starting to default to being the timestamping source which I agree with.
I guess Kory explicitly defaulted PHYs that were used primarily for PTP
applications/timestamping applications that primarily benefit from the
PHY timestamp. With this, I think it's safe to say that I agree with
this patch personally.

Reviewed-by: Rahul Rameshbabu <rrameshbabu@xxxxxxxxxx>