Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 29, 2023 at 09:09:59PM +0100, Köry Maincent wrote:
> On Mon, 20 Nov 2023 13:45:51 -0800
> Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> 
> > On Mon, 20 Nov 2023 21:58:58 +0200 Vladimir Oltean wrote:
> > > I'm still waiting for you to fully clarify the "per socket vs global"
> > > aspect, but independently of that, at least I understand why this is a
> > > counter-argument to my proposal. I need to tune it a bit (ASSUMING that
> > > we want DMA timestamps to "look like" hwtimestamps, and not like their
> > > own thing, to user space), because the PHC index would no longer fully
> > > identify a hwtstamp provider, so we need something more.
> > > 
> > > I imagine both ETHTOOL_MSG_TSINFO_GET and ETHTOOL_MSG_TSINFO_SET to
> > > support a new (nest) nlattr called ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER.
> > > 
> > > This would contain (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_PHC_INDEX
> > > and (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER. It could be
> > > extensible in the future, but this is the baseline and forms the key.
> > > 
> > > The latter takes values from an:
> > > 
> > > enum ethtool_hwstamp_provider_qualifier {
> > > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC,
> > > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY,
> > > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_DMA,
> > > };  
> > 
> > Sounds reasonable. Having more attributes than just PHC index works.
> > Given the lack of distinction between MAC and PHY for integrated NICs
> > I'd lean towards ditching the "layers" completely and exposing 
> > an "approximate" vs "precise" boolean. Approximate being the DMA point
> > for NICs, but more generically a point that is separated from the wire
> > by buffering or other variable length delay. Precise == IEEE 1588
> > quality.
> 
> Hello Jakub, just wondering.
> I can add this hwtstamp provider qualifier in the next series version but it
> won't be used as it is set and used at the driver level and no driver is using
> it for now. It would not be accepted if I use something that is not used, right?
> Do you still think I should add this in v8?

Not sure why you say "not used", though. Are you not planning to expose
the qualifier as an attribute to the listing of hwtstamp providers
offered to user space by ETHTOOL_MSG_TSINFO_GET?

Personally, I worry that if the qualifier gets added later (not now) to
the UAPI, we will end up having user space software (written now) that
iterates through the provider listing thinking that there may only ever
be one provider offered by one PHC, and will stop at the first such
provider found, whichever that may be.

With the added qualifier, there's a higher chance that user space
searches will be for a {phc, qualifier} pair (even if there will only be
1 possible qualifier type), and the future addition of a new hwtstamp
provider will keep existing software working in the same way as before,
i.e. user space won't select the DMA provider by mistake, by ignoring
the qualifier attribute altogether.

Generally I'm against adding things upfront that can only be in a certain
way, but in this case I believe that it is necessary in order for the
future extensions that were discussed to be possible. The qualifier is
part of the user space search key and thus pretty important.

My 2 cents, Jakub can absolutely disagree.