Message-ID: <20231120161004.flnwqv5dousiltcb@skbuf>
Date: Mon, 20 Nov 2023 18:10:04 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Köry Maincent <kory.maincent@...tlin.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
	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>, 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>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org,
	Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time
 stamping layer be selectable

On Mon, Nov 20, 2023 at 03:53:44PM +0100, Köry Maincent wrote:
> I did thought about it but I got stuck by the case of hardware timestamping
> without PHC. Richard explained the reason of its existence here:
> https://lore.kernel.org/netdev/ZS3MKWlnPqTe8gkq@hoboy.vegasvil.org/#t
> 
> Maybe I got a bit stuck in my implementation and should investigate more your
> proposition and how to deal with this case. Do you have an idea on how to
> solve it?

I would take what Richard said with a grain of salt, and interpret as
"there exists hardware with hwts but w/o PHC, and that may work for
marginal use cases", not that "we should design having that as a
first-class citizen in mind".

If there is any need for me to point out that such kind of driver isn't
a first class citizen, then here's an experiment:

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index e993ed04ab57..5755f54197b9 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -842,13 +842,7 @@ static int enetc_get_ts_info(struct net_device *ndev,
 {
 	int *phc_idx;
 	
-	phc_idx = symbol_get(enetc_phc_index);
-	if (phc_idx) {
-		info->phc_index = *phc_idx;
-		symbol_put(enetc_phc_index);
-	} else {
-		info->phc_index = -1;
-	}
+	info->phc_index = -1;
 
 #ifdef CONFIG_FSL_ENETC_PTP_CLOCK
 	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |

When elected as master, it kinda works, and does synchronize with a
slave, even if ptp4l gets confused about the phc_index being -1.

But when elected as a slave by the BMCA, ptp4l gets confused and thinks
that phc_index == -1 means that it's supposed to use software timestamping.

$ ptp4l -i eno0 -2 -P -m -s
ptp4l[1185.594]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[1185.598]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[1186.887]: port 1: new foreign master 00049f.fffe.05f628-1
ptp4l[1190.889]: selected best master clock 00049f.fffe.05f628
ptp4l[1190.890]: port 1: LISTENING to UNCALIBRATED on RS_SLAVE
ptp4l[1191.891]: master offset 37000003850 s0 freq +100000000 path delay       823
ptp4l[1192.896]: master offset 3[ 1235.693485] systemd-journald[125]: Time jumped backwards, rotating.
7000003848 s1 freq +99999998 path delay       822
ptp4l[1193.603]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1193.892]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1193.893]: master offset 37000003852 s0 freq +99999998 path delay       822
ptp4l[1194.342]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1194.604]: clockcheck: clock jumped forward or running faster than expected!

So, I guess the only thing we need to do to this kind of setup is not do
too much harm to it.

We break nothing if we make the phc_index the central aspect of hwts
layer selection - except for the fact that such a MAC won't be able to
change its timestamping layer to be a PHY.

If we wanted to add such a capability to that MAC driver, the obvious
way to solve the lack of a PHC is to create a PHC that returns
-EOPNOTSUPP for all of its ptp_clock_info operations (gettime, settime
etc). It may possibly be that, in the worst case, ptp4l needs to probe
for each syscall on the NIC's PHC being operational before deciding what
can be done with it. But that's already an improvement over the current
handling to make it more graceful, it's not to keep things on par.

> > Hmm, can you please explain what is the reason why software timestamping
> > can't coexist with PHY timestamping? It is a genuine question to which I
> > don't have an answer - I haven't used PHY timestamping. It must be
> > something specific to that, since I do know that MAC + software
> > timestamping work simultaneously just fine.
> 
> The software timestamp is managed through the MAC driver calling
> skb_tx_timestamp() function. The PHY driver does not call it, that's why there
> is no software timestamping in PHY driver capabilities. Also the PHY driver
> doesn't know if the MAC driver support it so it currently can not coexist with
> PHY timestamping.

I don't understand. Documentation/networking/timestamping.rst says that
skb_tx_timestamp() is one of the actual _mechanisms_ through which phylib
timestamping works. It's called by the parent MAC driver, and
mii_ts->txtstamp() hooks onto that. So in some situations, the PHY
timestamping core _piggybacks_ on top of software timestamping support
in the MAC.

Where I agree is that the PHY driver has no business in deciding whether
the interface should report the socket option flags for software timestamping
or not. But I still don't see a proof that they can't coexist. What you need
to explain is what makes said software timestamping unusable in the
presence of PHY timestamping - to justify this separate software
timestamping layer in your UAPI proposal.

Powered by blists - more mailing lists