Re: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.

From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "Michael Walle" <michael@walle.cc>,
	"Köry Maincent" <kory.maincent@bootlin.com>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-omap@vger.kernel.org,
	"Maxime Chevallier" <maxime.chevallier@bootlin.com>,
	"Richard Cochran" <richardcochran@gmail.com>,
	thomas.petazzoni@bootlin.com,
	"Russell King" <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Jay Vosburgh" <j.vosburgh@gmail.com>,
	"Veaceslav Falico" <vfalico@gmail.com>,
	"Andy Gospodarek" <andy@greyhouse.net>,
	"Claudiu Manoil" <claudiu.manoil@nxp.com>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	UNGLinuxDriver@microchip.com,
	"Minghao Chi" <chi.minghao@zte.com.cn>,
	"Jie Wang" <wangjie125@huawei.com>,
	"Oleksij Rempel" <linux@rempel-privat.de>,
	"Sean Anderson" <sean.anderson@seco.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	"Wolfram Sang" <wsa+renesas@sang-engineering.com>,
	"Alexander Lobakin" <alexandr.lobakin@intel.com>,
	"Marco Bonelli" <marco@mebeim.net>
Subject: Re: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.
Date: Mon, 13 Mar 2023 09:17:00 +0100	[thread overview]
Message-ID: <20230313081700.ie7cwjvrlky4e27b@soft-dev3-1> (raw)
In-Reply-To: <20230310164451.ls7bbs6pdzs4m6pw@skbuf>

The 03/10/2023 18:44, Vladimir Oltean wrote:
> 
> On Fri, Mar 10, 2023 at 02:15:29PM +0100, Horatiu Vultur wrote:
> > I was thinking about another scenario (I am sorry if this was already
> > discussed).
> > Currently when setting up to do the timestamp, the MAC will check if the
> > PHY has timestamping support if that is the case the PHY will do the
> > timestamping. So in case the switch was supposed to be a TC then we had
> > to make sure that the HW was setting up some rules not to forward PTP
> > frames by HW but to copy these frames to CPU.
> > With this new implementation, this would not be possible anymore as the
> > MAC will not be notified when doing the timestamping in the PHY.
> > Does it mean that now the switch should allocate these rules at start
> > time?
> 
> I would say no (to the allocation of trapping rules at startup time).
> It was argued before by people present in this thread that it should be
> possible (and default behavior) for switches to forward PTP frames as if
> they were PTP-unaware:
> https://patchwork.ozlabs.org/project/netdev/patch/20190813025214.18601-5-yangbo.lu@nxp.com/

Thanks for the explanation!

> 
> But it raises a really good point about how much care a switch driver
> needs to take, such that with PTP timestamping, it must trap but not
> timestamp the PTP frames.
> 
> There is a huge amount of variability here today.
> 
> The ocelot driver would be broken with PHY timestamping, since it would
> flood the PTP messages (it installs the traps only if it is responsible
> for taking the timestamps too).
> 
> The lan966x driver is very fine-tuned to call lan966x_ptp_setup_traps()
> regardless of what phy_has_hwtstamp() says.
> 
> The sparx5 driver doesn't even seem to install traps at all (unclear if
> they are predefined in hardware or not).

They are not predefined in HW, I have on my TODO list to add those
traps I just need to get the time to do this.

> 
> I guess that we want something like lan966x to keep working, since it
> sounds like it's making the sanest decision about what to do.
> 
> But, as you point out, with Köry's/Richard's proposal, the MAC driver
> will be bypassed when the selected timestamping layer is the PHY, and
> that's a problem currently.
> 
> May I suggest the following? There was another RFC which proposed the
> introduction of a netdev notifier when timestamping is turned on/off:
> https://lore.kernel.org/netdev/20220317225035.3475538-1-vladimir.oltean@nxp.com/
> 
> It didn't go beyond RFC status, because I started doing what Jakub
> suggested (converting the raw ioctls handlers to NDOs) but quickly got
> absolutely swamped into the whole mess.
> 
> If we have a notifier, then we can make switch drivers do things
> differently. They can activate timestamping per se in the timestamping
> NDO (which is only called when the MAC is the active timestamping layer),
> and they can activate PTP traps in the netdev notifier (which is called
> any time a timestamping status change takes place - the notifier info
> should contain details about which net_device and timestamping layer
> this is, for example).
> 
> It's just a proposal of how to create an alternative notification path
> that doesn't disturb the goals of this patch set.

-- 
/Horatiu