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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231120195858.wpaymolv6ws4hntp@skbuf>
Date:   Mon, 20 Nov 2023 21:58:58 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Köry Maincent <kory.maincent@...tlin.com>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        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 10:51:48AM -0800, Jakub Kicinski wrote:
> On Mon, 20 Nov 2023 19:39:35 +0100 Andrew Lunn wrote:
> > Do you know of real hardware with a MAC and a PHY sharing a PHC?
> 
> mlx5 for sure, but other designs, too. PHY, NIC pipeline and PCIe PTM
> may all need to time stamp from a single time counter.

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,
};

with Jakub's comments about the various types providing various
qualities of timestamps, given here:
https://lore.kernel.org/netdev/20230511150902.57d9a437@kernel.org/

 - PHY - per spec, at the RS layer
 - MAC - "close to the wire" in the MAC, specifically the pipeline
   delay (PHY stamp vs MAC stamp) should be constant for all packets;
   there must be no variable-time buffering and (for Tx) the time
   stamping must be past the stage of the pipeline affected by pause
   frames
 - DMA - worst quality, variable delay timestamp, usually taken when
   packets DMA descriptors (Rx or completion) are being written

It _sounds_ like we've all been talking about the same thing for ages,
but we weren't.

So, a PHC could offer multiple hwtstamp providers, as many as there are
qualifiers to uniquely describe them. Each hwstamp provider is
represented by a single ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER nested nlattr.
In TSINFO_GET requests, there are as many ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER
nests as there are hwtstamp providers for the NIC.

In the "normal" case of one single hwtstamp provider per PHC, it would
be the responsibility of the driver to set its qualifier to the right
thing: phylib to ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY, and "normal" MAC
drivers to ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC. Here, the qualifier
isn't more than an extra (partially redundant) mechanism for user space
to know what it's juggling with.

As opposed to Köry's proposal (where "dev->ts_layer == PHY_TIMESTAMPING"
means actual phylib), ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY describes
the PHY-like timestamp quality of any hwtstamp provider, be it provided
by a phylib PHY or a firmware-based PHY. It doesn't describe "the layer"
itself.

Does this tick all boxes?