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

[Horatiu Vultur <horatiu.vultur@microchip.com>]

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