Message-ID: <20231120155344.14cd69d9@kmaincent-XPS-13-7390>
Date: Mon, 20 Nov 2023 15:53:44 +0100
From: Köry Maincent <kory.maincent@...tlin.com>
To: Vladimir Oltean <vladimir.oltean@....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, 20 Nov 2023 16:23:16 +0200
Vladimir Oltean <vladimir.oltean@....com> wrote:

> On Mon, Nov 20, 2023 at 02:49:29PM +0100, Köry Maincent wrote:
> > > The next question would be: if a driver performs PHY management in
> > > firmware, and does not use phylib, how should user space interact with it?
> > > What timestamping layer and upon what should the ID be chosen?  
> > 
> > In that case it could be the second options I refereed to.
> > Using the id to select the right timestamp within the NIC driver.
> > It indeed won't be called PHY timestamping as it is managed by the NIC
> > firmware but as it is managed by only one firmware and driver using the id
> > to separate the available timestamp seems a good idea.
> > 
> > Another solution would be to create another value in the layer enumeration.
> > PHY_NIC_TIMESTAMPING? Better idea? I am not good at naming.  
> 
> The point I was trying to make is that your current choice of exposing
> PHY_TIMESTAMPING in UAPI, when it really only refers to phylib PHYs,
> would lead exactly to this sort of UAPI balkanization where everyone
> wants to add more timestamping layers, and to define IDs to be specific
> to their own invented layer. Maybe the concept of timestamping layers is
> not what user space should see at all.
> 
> In previous email discussions, I was proposing to Jakub and you "what if
> we didn't let user space select a specific layer like PHY_TIMESTAMPING
> or MAC_TIMESTAMPING at all, but just select a specific phc_index as the
> provider of hardware timestamps"?
> 
> The limitation we're trying to lift is that currently, there can be only
> a single provider of hardware timestamps. We make that provider customizable.
> There is already a good understanding from user space that, if "ethtool -T"
> on an interface says there is no PHC, then there are going to be no
> hardware timestamps. So I thought it would be much more intuitive if the
> timestamping layer could be selected by the user merely by an unified
> phc_index (provided by a phylib phy or firmware based driver or whatever),
> and everything else would just be an implementation detail of the kernel.
> No one should care that it's a phylib phy, and shouldn't use a different
> procedure to identify its ID based on whether it's a phylib or firmware
> PHY.
> 
> It's a bit hard to align my expectation of what this series should offer
> with yours. I think we're talking past each other, which unfortunately
> makes me lose track and interest. I wish you could have answered my
> earlier question about this alternative proposal.
> https://lore.kernel.org/netdev/20231013170903.p3ycicebnfrsmoks@skbuf/

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?

> > > Finally (and unrelated to the question above), why is
> > > SOFTWARE_TIMESTAMPING even a layer exposed in the UAPI? My understanding
> > > of this patch set is that it is meant to select the source of hardware
> > > timestamps that are given to a socket. What gap in the UAPI does the
> > > introduction of a SOFTWARE_TIMESTAMPING hwtstamping layer cover?  
> > 
> > As I explained to Jakub:
> > The software timestamping comes from the MAC driver capabilities and I
> > decided to separate software and MAC timestamping.  
> 
> Why? What was the problem? This confuses me because I don't understand
> what is the problem that the solution is trying to address, and whether
> the solution is orthogonal to all the other UAPI that exists for
> software and hardware timestamping at the socket layer - which AFAIK can
> happily coexist.
> 
> > If we select PHY timestamping we can't use software timestamping and
> > for an user, selecting the MAC as timestamping seems not logical to
> > use software timestamping (I got confused myself when I first dig into
> > it long time ago). Be able to select directly Software timestamping
> > seems appropriate and won't bring any harm. What do you think?  
> 
> 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.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists