Message-ID: <20231122140850.li2mvf6tpo3f2fhh@skbuf>
Date: Wed, 22 Nov 2023 16:08:50 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Köry Maincent <kory.maincent@...tlin.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 Wed, Nov 22, 2023 at 02:44:53PM +0100, Köry Maincent wrote:
> On Tue, 21 Nov 2023 09:43:54 -0800
> Jakub Kicinski <kuba@...nel.org> wrote:
> 
> > On Tue, 21 Nov 2023 18:31:14 +0100 Köry Maincent wrote:
> > > - Expand struct hwtstamp_config with a phc_index member for the
> > > SIOCG/SHWTSTAMP commands.
> > >   To keep backward compatibility if phc_index is not set in the
> > > hwtstamp_config data from userspace use the default hwtstamp (the default
> > > being selected as done in my patch series).
> > >   Is this possible, would it breaks things?  
> > 
> > I'd skip this bit, and focus on the ETHTOOL_TSINFO. Keep the ioctl as
> > "legacy" and do all the extensions in ethtool. TSINFO_GET can serve
> > as GET, to avoid adding 3rd command for the same thing. TSINFO_SET
> > would be new (as you indicate below).
> 
> You say this patch series should simply add TSINFO_SET command to set the
> current phc_index?
> 
> It won't solve your requirement of having simultaneous hwtimestamp and
> enabling/disabling them through rx_filter and tx_types.
> You want to do this in another patch series alongside a new SIOCG/SHWTSTAMP_2
> ABI?
> 
> > > - In netlink part, send one netlink tsinfo skb for each phc_index.  
> > 
> > phc_index and netdev combination. A DO command can only generate one
> > answer (or rather, it should generate only one answer, there are few
> > hard rules in netlink). So we need to move that functionality to DUMP.
> > We can filter the DUMP based on user-provided ifindex and/or phc_index.
> 
> Currently, the dumpit function is assigned to ethnl_default_dumpit. Wouldn't
> the behavior change of the dumpit callback break the ABI?
> 
> 
> > > Could be done in a later patch series:
> > > - Expand netlink TSINFO with ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER.
> > >   Describing this struct:
> > > enum ethtool_hwstamp_provider_qualifier {
> > >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PRECISE,
> > >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_APPROX,
> > > }; 
> > > 
> > >   Set the desired qualifier through TSINFO_SET or through SIOCSHWTSTAMP by
> > >   expanding again the struct hwtstamp_config.
> 
> Just wondering to have a insight of future support, in the case of several
> provider qualifier and the SIOCG/SHWTSTAMP_2 layout containing the phc_index.
> Will we be able to talk to the two providers qualifiers simultaneously or is it
> not possible. To know if the SIOCG/SHWTSTAMP_2 layout would contain the
> description of the qualifier provider.
> If I understand well your mail in the thread it will be the case right?
> 
> Regards,
> -- 
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com/

My understanding of Jakub's email was that he wants to see the functionality
offered by SIOCGHWTSTAMP and SIOCSHWTSTAMP converted to netlink. I don't
think that ethtool is the correct netlink family for that, given that
these aren't ethtool ioctls to begin with. Maybe the new netdev netlink
family. The conversion in its basic form would offer exactly the same
functionality. The extended netlink messages would have extra attributes
to identify the targeted hwtstamp provider. In the lack of those
attributes, the default hwtstamp provider is targeted. The definition of
the default hwtstamp provider should be as per your current patch set
(netdev, with a whitelist for current phylib PHYs).

The _listing_ of hwtstamp providers is what could be done through ethtool
netlink, similar but not identical to the way in which you are proposing
today (you are presenting blanket "layers" which correspond to netdev and
phylib, rather than individual providers).

The concept of an "active phc_index" would not explicitly exist in the
UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
The only thing would exist is a configurable rx_filter and tx_type per
hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
to learn to select the hwtstamp provider it wants to configure through
netlink, and use for its class of traffic.

This is why I mentioned by ndo_hwtstamp_set() conversion, because
suddenly it is a prerequisite for any further progress to be done.
You can't convert SIOCSHWTSTAMP to netlink if there are some driver
implementations which still use ndo_eth_ioctl(). They need to be
UAPI-agnostic.

I'm not sure what's with Richard's mention of the "_2" variants of the
ioctls. Probably a low-effort suggestion which was a bit out of context.
His main point, that you cannot extend struct hwtstamp_config as that
has a fixed binary format, is perfectly valid though. This is why
netlink is preferable, because if done correctly (meaning not with
NLA_BINARY attributes), then it is much more extensible because all
attributes are TLVs. Use NLA_BINARY, and you will run into the exact
extensibility issues that the ioctl interface has.

Powered by blists - more mailing lists