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

  Now that the current timestamp is saved in a variable lets add the
ETHTOOL_MSG_TS_SET ethtool netlink socket to make it selectable.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move selected_timestamping_layer introduction in this patch.
- Replace strmcmp by sysfs_streq.
- Use the PHY timestamp only if available.

Changes in v3:
- Add a devicetree binding to select the preferred timestamp
- Replace the way to select timestamp through ethtool instead of sysfs
You can test it with the ethtool source on branch feature_ptp of:
https://github.com/kmaincent/ethtool

Changes in v4:
- Change the API to select MAC default time stamping instead of the PHY.
- Add a whitelist to no break the old timestamp PHY default preferences
  for current PHY.
- Replace the ethtool ioctl by netlink.
- Add a netdev notifier to allow network device to create trap on PTP
  packets. Not tested as it need to change the lan966x driver that
  implement packet traps. I will do after the hwtstamp management change
  to NDOs.

Changes in v5:
- Remove the netdev notifier added in v4.
- Extract the default timestamp API change in another patch.

Changes in v6:
- Update the error message.
- Put ndo_hwtstamp_set check first as it is most likely what most drivers
  currently do not support.
- Follow timestamping layer naming update.
- Update the timestamp layer check.
- Update timestamp set between MAC and software.

Changes in v7:
- Fix commit title typo.
---
 Documentation/networking/ethtool-netlink.rst | 17 +++++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.c                        |  8 +++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/ts.c                             | 99 ++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+)
  

On Tue, 14 Nov 2023 12:28:43 +0100 Kory Maincent wrote:
> +	if (!tb[ETHTOOL_A_TS_LAYER])
> +		return 0;

GENL_REQ_ATTR_CHECK(), not sure why anyone would issue this command
without any useful attr.

> +	/* Disable time stamping in the current layer. */
> +	if (netif_device_present(dev) &&
> +	    (dev->ts_layer == PHY_TIMESTAMPING ||
> +	    dev->ts_layer == MAC_TIMESTAMPING)) {
> +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> +		if (ret < 0)
> +			return ret;

So you only support PHYLIB?

The semantics need to be better documented :(
  

On Sat, 18 Nov 2023 18:34:33 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 14 Nov 2023 12:28:43 +0100 Kory Maincent wrote:
> > +	if (!tb[ETHTOOL_A_TS_LAYER])
> > +		return 0;  
> 
> GENL_REQ_ATTR_CHECK(), not sure why anyone would issue this command
> without any useful attr.
> 
> > +	/* Disable time stamping in the current layer. */
> > +	if (netif_device_present(dev) &&
> > +	    (dev->ts_layer == PHY_TIMESTAMPING ||
> > +	    dev->ts_layer == MAC_TIMESTAMPING)) {
> > +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> > +		if (ret < 0)
> > +			return ret;  
> 
> So you only support PHYLIB?
> 
> The semantics need to be better documented :(

Yes as we don't really know how each MAC deal with the timestamping
before ndo_hwstamp_get/set. Using phylib only allows us to be sure these NDO are
implemented and the management of timestamping is coherent in the MAC. Also It
will push people to move on to these NDOs.

Ok I will add documentation.
  

Hi Köry,

On Mon, Nov 20, 2023 at 10:44:39AM +0100, Köry Maincent wrote:
> On Sat, 18 Nov 2023 18:34:33 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Tue, 14 Nov 2023 12:28:43 +0100 Kory Maincent wrote:
> > > +	if (!tb[ETHTOOL_A_TS_LAYER])
> > > +		return 0;  
> > 
> > GENL_REQ_ATTR_CHECK(), not sure why anyone would issue this command
> > without any useful attr.
> > 
> > > +	/* Disable time stamping in the current layer. */
> > > +	if (netif_device_present(dev) &&
> > > +	    (dev->ts_layer == PHY_TIMESTAMPING ||
> > > +	    dev->ts_layer == MAC_TIMESTAMPING)) {
> > > +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> > > +		if (ret < 0)
> > > +			return ret;  
> > 
> > So you only support PHYLIB?
> > 
> > The semantics need to be better documented :(
> 
> Yes as we don't really know how each MAC deal with the timestamping
> before ndo_hwstamp_get/set. Using phylib only allows us to be sure these NDO are
> implemented and the management of timestamping is coherent in the MAC. Also It
> will push people to move on to these NDOs.
> 
> Ok I will add documentation.
> 
> -- 
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com/

When Jakub says "the semantics need to be better documented", I'm also
thinking of a different direction.

From what I understand, Maxime is working on representing multiple
phylib PHYs in the UAPI:
https://patchwork.kernel.org/project/netdevbpf/cover/20231117162323.626979-1-maxime.chevallier@bootlin.com/

Does your UAPI proposal make it possible in any way to select
timestamping in phylib PHY A rather than PHY B? Or do you think it is
extensible to support that, somehow?
  

On Mon, 20 Nov 2023 12:52:55 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> > > So you only support PHYLIB?
> > > 
> > > The semantics need to be better documented :(  
> > 
> > Yes as we don't really know how each MAC deal with the timestamping
> > before ndo_hwstamp_get/set. Using phylib only allows us to be sure these
> > NDO are implemented and the management of timestamping is coherent in the
> > MAC. Also It will push people to move on to these NDOs.
> > 
> > Ok I will add documentation.
> 
> When Jakub says "the semantics need to be better documented", I'm also
> thinking of a different direction.
> 
> From what I understand, Maxime is working on representing multiple
> phylib PHYs in the UAPI:
> https://patchwork.kernel.org/project/netdevbpf/cover/20231117162323.626979-1-maxime.chevallier@bootlin.com/

Yes I am also following his patch series. 
 
> Does your UAPI proposal make it possible in any way to select
> timestamping in phylib PHY A rather than PHY B? Or do you think it is
> extensible to support that, somehow?

It does not support it for now.
I didn't want to base my work on his series as it could work without it for now
and I didn't want to wait to have his series accepted. It is more a future
possible support as I don't have anything to test it and I don't know if such
hardware exists right now.
I think it will be extensible to support that, my thinking was to create this
struct in net_device struct:

struct {
	enum layer;
	u32 id;
} ts;

With id saving the phy_index of the PHY X used when the layer PHY is selected.
This id could also be used to store the timestamp point in case of several
timestamp in a MAC.

Regards,
  

On Mon, Nov 20, 2023 at 12:14:40PM +0100, Köry Maincent wrote:
> > Does your UAPI proposal make it possible in any way to select
> > timestamping in phylib PHY A rather than PHY B? Or do you think it is
> > extensible to support that, somehow?
> 
> It does not support it for now.
> I didn't want to base my work on his series as it could work without it for now
> and I didn't want to wait to have his series accepted. It is more a future
> possible support as I don't have anything to test it and I don't know if such
> hardware exists right now.
> I think it will be extensible to support that, my thinking was to create this
> struct in net_device struct:
> 
> struct {
> 	enum layer;
> 	u32 id;
> } ts;
> 
> With id saving the phy_index of the PHY X used when the layer PHY is selected.
> This id could also be used to store the timestamp point in case of several
> timestamp in a MAC.

Ok, and I suppose the "u32 id" would be numerically the same as the
ETHTOOL_A_HEADER_PHY_INDEX nlattr that Maxime is proposing?

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?

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?
  

On Mon, 20 Nov 2023 14:06:01 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 12:14:40PM +0100, Köry Maincent wrote:
> > > Does your UAPI proposal make it possible in any way to select
> > > timestamping in phylib PHY A rather than PHY B? Or do you think it is
> > > extensible to support that, somehow?  
> > 
> > It does not support it for now.
> > I didn't want to base my work on his series as it could work without it for
> > now and I didn't want to wait to have his series accepted. It is more a
> > future possible support as I don't have anything to test it and I don't
> > know if such hardware exists right now.
> > I think it will be extensible to support that, my thinking was to create
> > this struct in net_device struct:
> > 
> > struct {
> > 	enum layer;
> > 	u32 id;
> > } ts;
> > 
> > With id saving the phy_index of the PHY X used when the layer PHY is
> > selected. This id could also be used to store the timestamp point in case
> > of several timestamp in a MAC.  
> 
> Ok, and I suppose the "u32 id" would be numerically the same as the
> ETHTOOL_A_HEADER_PHY_INDEX nlattr that Maxime is proposing?

Yes.

> 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.
 
> 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. 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?

Regards,
  

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/

> > 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.
  

On Mon, 20 Nov 2023 16:23:16 +0200
Vladimir Oltean <vladimir.oltean@nxp.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,
  

On Mon, Nov 20, 2023 at 03:53:44PM +0100, Köry Maincent wrote:
> 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?

I would take what Richard said with a grain of salt, and interpret as
"there exists hardware with hwts but w/o PHC, and that may work for
marginal use cases", not that "we should design having that as a
first-class citizen in mind".

If there is any need for me to point out that such kind of driver isn't
a first class citizen, then here's an experiment:

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index e993ed04ab57..5755f54197b9 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -842,13 +842,7 @@ static int enetc_get_ts_info(struct net_device *ndev,
 {
 	int *phc_idx;
 	
-	phc_idx = symbol_get(enetc_phc_index);
-	if (phc_idx) {
-		info->phc_index = *phc_idx;
-		symbol_put(enetc_phc_index);
-	} else {
-		info->phc_index = -1;
-	}
+	info->phc_index = -1;
 
 #ifdef CONFIG_FSL_ENETC_PTP_CLOCK
 	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |

When elected as master, it kinda works, and does synchronize with a
slave, even if ptp4l gets confused about the phc_index being -1.

But when elected as a slave by the BMCA, ptp4l gets confused and thinks
that phc_index == -1 means that it's supposed to use software timestamping.

$ ptp4l -i eno0 -2 -P -m -s
ptp4l[1185.594]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[1185.598]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[1186.887]: port 1: new foreign master 00049f.fffe.05f628-1
ptp4l[1190.889]: selected best master clock 00049f.fffe.05f628
ptp4l[1190.890]: port 1: LISTENING to UNCALIBRATED on RS_SLAVE
ptp4l[1191.891]: master offset 37000003850 s0 freq +100000000 path delay       823
ptp4l[1192.896]: master offset 3[ 1235.693485] systemd-journald[125]: Time jumped backwards, rotating.
7000003848 s1 freq +99999998 path delay       822
ptp4l[1193.603]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1193.892]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1193.893]: master offset 37000003852 s0 freq +99999998 path delay       822
ptp4l[1194.342]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1194.604]: clockcheck: clock jumped forward or running faster than expected!

So, I guess the only thing we need to do to this kind of setup is not do
too much harm to it.

We break nothing if we make the phc_index the central aspect of hwts
layer selection - except for the fact that such a MAC won't be able to
change its timestamping layer to be a PHY.

If we wanted to add such a capability to that MAC driver, the obvious
way to solve the lack of a PHC is to create a PHC that returns
-EOPNOTSUPP for all of its ptp_clock_info operations (gettime, settime
etc). It may possibly be that, in the worst case, ptp4l needs to probe
for each syscall on the NIC's PHC being operational before deciding what
can be done with it. But that's already an improvement over the current
handling to make it more graceful, it's not to keep things on par.

> > 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.

I don't understand. Documentation/networking/timestamping.rst says that
skb_tx_timestamp() is one of the actual _mechanisms_ through which phylib
timestamping works. It's called by the parent MAC driver, and
mii_ts->txtstamp() hooks onto that. So in some situations, the PHY
timestamping core _piggybacks_ on top of software timestamping support
in the MAC.

Where I agree is that the PHY driver has no business in deciding whether
the interface should report the socket option flags for software timestamping
or not. But I still don't see a proof that they can't coexist. What you need
to explain is what makes said software timestamping unusable in the
presence of PHY timestamping - to justify this separate software
timestamping layer in your UAPI proposal.
  

On Mon, 20 Nov 2023 18:10:04 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 03:53:44PM +0100, Köry Maincent wrote:
> > 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?  
> 
> I would take what Richard said with a grain of salt, and interpret as
> "there exists hardware with hwts but w/o PHC, and that may work for
> marginal use cases", not that "we should design having that as a
> first-class citizen in mind".

> When elected as master, it kinda works, and does synchronize with a
> slave, even if ptp4l gets confused about the phc_index being -1.
> 
> But when elected as a slave by the BMCA, ptp4l gets confused and thinks
> that phc_index == -1 means that it's supposed to use software timestamping.
> 
> So, I guess the only thing we need to do to this kind of setup is not do
> too much harm to it.
> 
> We break nothing if we make the phc_index the central aspect of hwts
> layer selection - except for the fact that such a MAC won't be able to
> change its timestamping layer to be a PHY.
> 
> If we wanted to add such a capability to that MAC driver, the obvious
> way to solve the lack of a PHC is to create a PHC that returns
> -EOPNOTSUPP for all of its ptp_clock_info operations (gettime, settime
> etc). It may possibly be that, in the worst case, ptp4l needs to probe
> for each syscall on the NIC's PHC being operational before deciding what
> can be done with it. But that's already an improvement over the current
> handling to make it more graceful, it's not to keep things on par.

Ok, you convinced me.
Thanks for your arguments and spending time explaining it.


Regards,
  

On Mon, 20 Nov 2023 16:23:16 +0200 Vladimir Oltean wrote:
> 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"?

What about my use case of having a NIC which can stamp at a low rate
at the PHY (for PTP) and high rate at the DMA block (for congestion
control)? Both stamp points have the same PHC index.
  

> What about my use case of having a NIC which can stamp at a low rate
> at the PHY (for PTP) and high rate at the DMA block (for congestion
> control)? Both stamp points have the same PHC index.

How theoretical is that? To me, it seems more likely you have two PHC.

The PHY stamping tends to be slow because of the MDIO bus. If the MAC
has fast access to the PHC, it means its not on the MDIO bus. It
probably means you have the PHY integrated into the MAC/SoC, so the
MAC can access it. But if its integrated like this, i don't see why
PHY stamping should be particularly slow. So you can probably use it
for congestion control. And then you don't need DMA stamping.

Do you know of real hardware with a MAC and a PHY sharing a PHC?

   Andrew
  

On Mon, 20 Nov 2023 19:39:35 +0100 Andrew Lunn wrote:
> > What about my use case of having a NIC which can stamp at a low rate
> > at the PHY (for PTP) and high rate at the DMA block (for congestion
> > control)? Both stamp points have the same PHC index.  
> 
> How theoretical is that? To me, it seems more likely you have two PHC.

Very practical. mlx5 does this today, based on guessing and private
ethtool flags.

> The PHY stamping tends to be slow because of the MDIO bus. If the MAC
> has fast access to the PHC, it means its not on the MDIO bus. It
> probably means you have the PHY integrated into the MAC/SoC, so the
> MAC can access it. But if its integrated like this, i don't see why
> PHY stamping should be particularly slow. So you can probably use it
> for congestion control. And then you don't need DMA stamping.

Tx stamps are harder to carry back to the host all the way from the PHY
than from the DMA block when DMA completion is signaled. Rx stamps seem
much easier to carry down the pipeline but apparently some vendors are
incapable of doing that as well.

> 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.
  

On Mon, Nov 20, 2023 at 09:37:23AM -0800, Jakub Kicinski wrote:
> On Mon, 20 Nov 2023 16:23:16 +0200 Vladimir Oltean wrote:
> > 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"?
> 
> What about my use case of having a NIC which can stamp at a low rate
> at the PHY (for PTP) and high rate at the DMA block (for congestion
> control)? Both stamp points have the same PHC index.

Well, first of all, given my understanding of the "laws of physics",
I think something has to give in your use case description. I can't
see how on RX, the NIC can decide in advance whether to provide low
rate MAC timestamps for packets going to a socket and high rate DMA
timestamps for packets going to another socket. It can either provide
MAC timestamps, or DMA timestamps, or an unreliable, unpresentable to
user space, mix.

But maybe I'm wrong and there are NICs which can do that filtering.
If such NIC exists, then I guess a SOF_TIMESTAMPING_RX_DMA flag should
be added to the socket layer, and the NIC driver provides timestamps
according to the skb->sk->sk_tsflags, and that problem is completely out
of scope for Köry's patch set - and implicitly compatible with it, since
as you say, the device-wide timestamping layer - PHC index - does not
really change.

If I'm not wrong and the MAC-or-DMA timestamp selection is NIC-wide
(which diverges from your problem description), then neither Köry's work
nor my "everything is a phc_index" proposal will bring your use case to
fruition without further work. Here I would avoid speculating, because a
lot will depend upon the details which you haven't really given.

One question will be whether, in the case of "NIC-wide DMA timestamps",
DMA timestamps should be presented as hardware timestamps - struct
scm_timestamping[2] from CMSG_DATA() - or as their own thing, that user
space needs explicit support for - by parsing a new cmsg level/type.
If DMA timestamps won't look to user space like hardware timestamps,
then the use case is again out of scope for Köry's work, as far as I see
it.

Another simple question is - if NICs do this today - probably by giving
the "unrepresentable mix" to user space in an implicit, hardcoded and
very fine tuned way such that nobody bats an eye - then what is there
more to support? Are you looking at extra UAPI as a way to legitimize
hacks, or do you feel there is extra control that applications can gain?
  

On Mon, Nov 20, 2023 at 07:39:35PM +0100, Andrew Lunn wrote:
> > What about my use case of having a NIC which can stamp at a low rate
> > at the PHY (for PTP) and high rate at the DMA block (for congestion
> > control)? Both stamp points have the same PHC index.
> 
> How theoretical is that? To me, it seems more likely you have two PHC.
> 
> The PHY stamping tends to be slow because of the MDIO bus. If the MAC
> has fast access to the PHC, it means its not on the MDIO bus. It
> probably means you have the PHY integrated into the MAC/SoC, so the
> MAC can access it. But if its integrated like this, i don't see why
> PHY stamping should be particularly slow. So you can probably use it
> for congestion control. And then you don't need DMA stamping.
> 
> Do you know of real hardware with a MAC and a PHY sharing a PHC?

Not so much a MAC and PHY sharing a PHC, but I notice in USXGMII-M,
there is the ability for the PHY to pass the timestamp back to the MAC
through the USXGMII-M connection - which would eliminate the problems
of accessing the PHY to get that the timestamps.
  

On Mon, 20 Nov 2023 21:00:23 +0200 Vladimir Oltean wrote:
> Well, first of all, given my understanding of the "laws of physics",
> I think something has to give in your use case description. I can't
> see how on RX, the NIC can decide in advance whether to provide low
> rate MAC timestamps for packets going to a socket and high rate DMA
> timestamps for packets going to another socket. It can either provide
> MAC timestamps, or DMA timestamps, or an unreliable, unpresentable to
> user space, mix.

Rx time stamping is configured by filters. Is there a problem with user
specifying that they want "true" timestamps for PTP/NTP packets, and
"dma" timestamps for all the rest?

Maybe we can extend struct scm_timestamping to carry an indication
which stamp ended up in ts[2] but that's less important to me than
the ability to configure the thing. Right now, as I said, mlx5 uses
an ethtool priv flag :(

> But maybe I'm wrong and there are NICs which can do that filtering.
> If such NIC exists, then I guess a SOF_TIMESTAMPING_RX_DMA flag should
> be added to the socket layer, and the NIC driver provides timestamps
> according to the skb->sk->sk_tsflags, and that problem is completely out
> of scope for Köry's patch set - and implicitly compatible with it, since
> as you say, the device-wide timestamping layer - PHC index - does not
> really change.

IDK. Maybe the sniffles I picked up at LPC are clouding my judgment
but to me this patch set is shaped too much by current implementation
and not enough by what it's modeling. It basically exposes to user
space the "mux" for choosing NETDEV vs PHYLIB.

There are multiple time stamping points as the packet moves thru 
the pipeline. Expose them so that SIOC[GS]HWTSTAMP can target each
on individually.

> If I'm not wrong and the MAC-or-DMA timestamp selection is NIC-wide
> (which diverges from your problem description),

Nope.

> then neither Köry's work
> nor my "everything is a phc_index" proposal will bring your use case to
> fruition without further work. Here I would avoid speculating, because a
> lot will depend upon the details which you haven't really given.

What are the details you'd like? PTP gets stamped at the PHY/MAC, 
the rest gets stamped at DMA. mlx5 achieves this by splitting the
PTP traffic to a separate queue pair, and configuring that qp to
capture PHY/MAC stamps, AFAIU.

> One question will be whether, in the case of "NIC-wide DMA timestamps",
> DMA timestamps should be presented as hardware timestamps - struct
> scm_timestamping[2] from CMSG_DATA() - or as their own thing, that user
> space needs explicit support for - by parsing a new cmsg level/type.
> If DMA timestamps won't look to user space like hardware timestamps,
> then the use case is again out of scope for Köry's work, as far as I see
> it.
> 
> Another simple question is - if NICs do this today - probably by giving
> the "unrepresentable mix" to user space in an implicit, hardcoded and
> very fine tuned way such that nobody bats an eye - then what is there
> more to support? Are you looking at extra UAPI as a way to legitimize
> hacks, or do you feel there is extra control that applications can gain?

I don't understand what you're asking me.

DMA timestamping is becoming increasingly important. Ready any
congestion control paper from the last 5 years and chances are
it will be using delay as a signal. If we're extending uAPI
for Hw stamping we should make sure to cater to CC use cases.
  

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?
  

On Mon, Nov 20, 2023 at 11:58:39AM -0800, Jakub Kicinski wrote:
> Rx time stamping is configured by filters. Is there a problem with user
> specifying that they want "true" timestamps for PTP/NTP packets, and
> "dma" timestamps for all the rest?

There is, because enum hwtstamp_rx_filters is NIC-wide, and there is
only one of those - corresponding to the single hwtstamp (ts[2]) provider.
There were never talks in this patch set, AFAIR, about multiple hwtstamp
providers active simultaneously (for different traffic streams) and
configuring them independently, with separate RX filters.

> Maybe we can extend struct scm_timestamping to carry an indication
> which stamp ended up in ts[2] but that's less important to me than
> the ability to configure the thing. Right now, as I said, mlx5 uses
> an ethtool priv flag :(

No, you misunderstood me. I didn't suggest (at least not here) to add an
indication to struct scm_timestamping of "what's the source of ts[2]
(the hwtstamp)".

I was just _asking_ (collecting data) whether it's ultimately desirable
for DMA timestamps to be visible in ts[2] (indistinguishable from a
better hwtstamp, as they currently are, I guess) rather than their
own thing. Like for example, in a congestion control algorithm, where
does TCP really get them from.

If they'd rather be their own thing in a fully developed API, then the
whole discussion is rather off-topic to Köry, because here, we're
beating the dead horse of "where does ts[2] come from" - _still_ a
single source, just selectable, that is.

> > But maybe I'm wrong and there are NICs which can do that filtering.
> > If such NIC exists, then I guess a SOF_TIMESTAMPING_RX_DMA flag should
> > be added to the socket layer, and the NIC driver provides timestamps
> > according to the skb->sk->sk_tsflags, and that problem is completely out
> > of scope for Köry's patch set - and implicitly compatible with it, since
> > as you say, the device-wide timestamping layer - PHC index - does not
> > really change.
> 
> IDK. Maybe the sniffles I picked up at LPC are clouding my judgment
> but to me this patch set is shaped too much by current implementation
> and not enough by what it's modeling. It basically exposes to user
> space the "mux" for choosing NETDEV vs PHYLIB.

The last sentence I agree with.

> There are multiple time stamping points as the packet moves thru 
> the pipeline. Expose them so that SIOC[GS]HWTSTAMP can target each
> on individually.

Ok, that is rather vague and complex.

You will forever have to contend with the fact that struct scm_timestamping
can contain a single hwtstamp per packet: ts[2]. So you need complex
control path logic to ensure that the sum of RX filters for all
timestamping points in the packet data path doesn't actually request
more than one ts[2] for any skb.

I understand how that could scrath your itch, but here, it sounds
off-topic?

This is actually starting to get close, in a way, to my feedback to
Richard to allow multiple hwtstamp sources for an skb, and to just give
an indication in the cmsg of what's their source, leaving user space to
figure out the rest.
https://lore.kernel.org/netdev/20220120164832.xdebp5vykib6h6dp@skbuf/

But his response was "There was a fair amount of discussion, and it
seemed to me that everyone wanted a pony."
https://lore.kernel.org/netdev/Y%2F0Idkhy27TObawi@hoboy.vegasvil.org/

I mean, IDK, maybe it's not off-topic, but it's a round-about way of
achieving what they think they can achieve in a more straightforward way.

Rephrased in my own words, you're saying:

Forget the concept of an active hwtstamp provider, just open up the
knobs of _all_ possible hwtstamp providers for a NIC. Simultaneously!
To make one active and all the others inactive, just use
HWTSTAMP_FILTER_NONE/HWTSTAMP_TX_OFF for all except one, and the desired
enum hwtstamp_rx_filters / enum hwtstamp_tx_types for the active one.
Live with this expanded configuration model for a while, just restricted
for a single active timestamping layer, and then, once user space is
ready for an enhanced struct scm_timestamping which supports potentially
multiple cmsgs with distinct hwtstamps, remove the restriction and let
it all rip! Everybody gets their pony!

Additionally, SIOCSHWTSTAMP is kinda rusty, has a fixed binary format,
and is not extensible to target a specific hwtstamp provider. So a
netlink conversion of that, as a first step, would of course be great.

Is it an accurate summary?

If it is, I'll let Köry comment on the feasibility :)

> > If I'm not wrong and the MAC-or-DMA timestamp selection is NIC-wide
> > (which diverges from your problem description),
> 
> Nope.
> 
> > then neither Köry's work
> > nor my "everything is a phc_index" proposal will bring your use case to
> > fruition without further work. Here I would avoid speculating, because a
> > lot will depend upon the details which you haven't really given.
> 
> What are the details you'd like? PTP gets stamped at the PHY/MAC, 
> the rest gets stamped at DMA. mlx5 achieves this by splitting the
> PTP traffic to a separate queue pair, and configuring that qp to
> capture PHY/MAC stamps, AFAIU.

That's enough, thanks.

> > One question will be whether, in the case of "NIC-wide DMA timestamps",
> > DMA timestamps should be presented as hardware timestamps - struct
> > scm_timestamping[2] from CMSG_DATA() - or as their own thing, that user
> > space needs explicit support for - by parsing a new cmsg level/type.
> > If DMA timestamps won't look to user space like hardware timestamps,
> > then the use case is again out of scope for Köry's work, as far as I see
> > it.
> > 
> > Another simple question is - if NICs do this today - probably by giving
> > the "unrepresentable mix" to user space in an implicit, hardcoded and
> > very fine tuned way such that nobody bats an eye - then what is there
> > more to support? Are you looking at extra UAPI as a way to legitimize
> > hacks, or do you feel there is extra control that applications can gain?
> 
> I don't understand what you're asking me.

You've partially answered above. The mix of timestamps coming from the
PHY/MAC and those coming from the DMA is unrepresentable in today's
UAPI, and is just fine-tuned to work for the existing use case of "PTP
gets PHY/MAC, everything else gets DMA".

Still not 100% clear what would the proper UAPI (separate user-controllable
RX filters for PHY, MAC and DMA) gain, in addition to what exists in mlx5.

> DMA timestamping is becoming increasingly important. Ready any
> congestion control paper from the last 5 years and chances are
> it will be using delay as a signal. If we're extending uAPI
> for Hw stamping we should make sure to cater to CC use cases.

I'll stop commenting here for a while and go read some of those papers and
RFCs, in the hope that I find some info about the way in which TCP expects
hwtstamps from a NIC. It's quite evident to me that I don't have enough
information to help this discussion reach a conclusion.
  

On Mon, 20 Nov 2023 23:17:59 +0200 Vladimir Oltean wrote:
> Forget the concept of an active hwtstamp provider, just open up the
> knobs of _all_ possible hwtstamp providers for a NIC. Simultaneously!
> To make one active and all the others inactive, just use
> HWTSTAMP_FILTER_NONE/HWTSTAMP_TX_OFF for all except one, and the desired
> enum hwtstamp_rx_filters / enum hwtstamp_tx_types for the active one.
> Live with this expanded configuration model for a while, just restricted
> for a single active timestamping layer, and then, once user space is
> ready for an enhanced struct scm_timestamping which supports potentially
> multiple cmsgs with distinct hwtstamps, remove the restriction and let
> it all rip! Everybody gets their pony!
> 
> Additionally, SIOCSHWTSTAMP is kinda rusty, has a fixed binary format,
> and is not extensible to target a specific hwtstamp provider. So a
> netlink conversion of that, as a first step, would of course be great.
> 
> Is it an accurate summary?

Yes.

For now we can impose the requirement that only one can be active 
easily at the kernel level. But the uAPI should allow expressing more.

> You've partially answered above. The mix of timestamps coming from the
> PHY/MAC and those coming from the DMA is unrepresentable in today's
> UAPI, and is just fine-tuned to work for the existing use case of "PTP
> gets PHY/MAC, everything else gets DMA".
> 
> Still not 100% clear what would the proper UAPI (separate user-controllable
> RX filters for PHY, MAC and DMA) gain, in addition to what exists in mlx5.

Too late for mlx5 but I'm anticipating that more vendors will start
needing such configuration in the future. At which point it will be
good to have an API in place.
  

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.
  

On Mon, Nov 20, 2023 at 01:37:37PM -0800, Jakub Kicinski wrote:
> > Is it an accurate summary?
> 
> Yes.
> 
> For now we can impose the requirement that only one can be active 
> easily at the kernel level. But the uAPI should allow expressing more.

I see. That's quite something to think about for Köry. In its defense,
I also agree that this idea seems the most orthogonal to everything else
that we have or may want to add in the future, and is not likely to
become obsoleted by some other mechanism that can achieve the same
thing, but in a more flexible way. It's just that it's quite the task.

I sense it may be time to dust off and submit the rest of my
ndo_hwtstamp_get()/ ndo_hwtstamp_set() conversions before a netlink
conversion of SIOCGHWTSTAMP/SIOCSHWTSTAMP could even take place...
https://github.com/vladimiroltean/linux/commits/ndo-hwtstamp-v9
  

On Tue, 21 Nov 2023 00:05:49 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 01:37:37PM -0800, Jakub Kicinski wrote:
> > > Is it an accurate summary?  
> > 
> > Yes.
> > 
> > For now we can impose the requirement that only one can be active 
> > easily at the kernel level. But the uAPI should allow expressing more.  
> 
> I see. That's quite something to think about for Köry. In its defense,
> I also agree that this idea seems the most orthogonal to everything else
> that we have or may want to add in the future, and is not likely to
> become obsoleted by some other mechanism that can achieve the same
> thing, but in a more flexible way. It's just that it's quite the task.
> 
> I sense it may be time to dust off and submit the rest of my
> ndo_hwtstamp_get()/ ndo_hwtstamp_set() conversions before a netlink
> conversion of SIOCGHWTSTAMP/SIOCSHWTSTAMP could even take place...
> https://github.com/vladimiroltean/linux/commits/ndo-hwtstamp-v9

Ok I kind of got an idea of what is your prerequisites.

If I summarize, a solution could be this:

- 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?

- In netlink part, send one netlink tsinfo skb for each phc_index.

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.

Do you think this is feasible?
I might miss some core stuff.

Regards,
  

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).

> - 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.

> 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.
> 
> Do you think this is feasible?
  

On Tue, Nov 21, 2023 at 06:31:14PM +0100, Köry Maincent wrote:
> If I summarize, a solution could be this:
> 
> - 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?

You can't "expand" hwtstamp_config because it is an established ABI.

(You could introduce SIOC[GS]HWTSTAMP_2 with an expanded layout)

Thanks,
Richard
  

On Tue, 21 Nov 2023 09:43:54 -0800
Jakub Kicinski <kuba@kernel.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,
  

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@kernel.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.
  

On Wed, Nov 22, 2023 at 04:08:50PM +0200, Vladimir Oltean wrote:
> 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.

One clarification: the extended timestamping filters are PER NETDEV
(in addition to being per one of the hwtstamp providers listed for that
netdev). This was understated from the fact that the netlink interface
itself targets a netdev, but I didn't say it explicitly.
  

On Wed, Nov 22, 2023 at 04:08:50PM +0200, Vladimir Oltean wrote:
> 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.

@Jakub, for your long-term "MAC timestamps for PTP, DMA for everything else".
How do you see this? I guess we need some sort of priority function in
the UAPI between hwtstamp providers.

And even with that, I think the enums that we currently have for filters
are not specific enough. The most we could expose is:

                      MAC provider                      DMA provider

hwtstamp_rx_filters   HWTSTAMP_FILTER_PTP_V2_EVENT      HWTSTAMP_FILTER_ALL
tx_type               HWTSTAMP_TX_ON                    HWTSTAMP_TX_ON

but it isn't clear: for PTP, does the DMA provider give you an RX
timestamp too? What about a TX timestamp?
  

On Wed, 22 Nov 2023 16:08:50 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> 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@kernel.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?

> 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.

I appreciate the clarification, it now makes more sense to me.

Regards,
  

On Wed, 22 Nov 2023 16:08:50 +0200 Vladimir Oltean wrote:
> 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.

Well, ethtool has been the catch all for a lot of random things
for the longest time. The question is whether we want to extend
ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
(i.e. getting capabilities)?

My vote is that keeping it in ethtool is less bad than 3rd API.

> 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.

"Active provider" is the one that has TX_ON, rx != FILTER_NONE, right?

> 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.

Right, definitely.

> 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.
  

On Wed, 22 Nov 2023 16:36:18 +0200 Vladimir Oltean wrote:
> @Jakub, for your long-term "MAC timestamps for PTP, DMA for everything else".
> How do you see this? I guess we need some sort of priority function in
> the UAPI between hwtstamp providers.
> 
> And even with that, I think the enums that we currently have for filters
> are not specific enough. The most we could expose is:
> 
>                       MAC provider                      DMA provider
> 
> hwtstamp_rx_filters   HWTSTAMP_FILTER_PTP_V2_EVENT      HWTSTAMP_FILTER_ALL
> tx_type               HWTSTAMP_TX_ON                    HWTSTAMP_TX_ON
> 
> but it isn't clear: for PTP, does the DMA provider give you an RX
> timestamp too?

If we phrase it as "precise / approximate" rather than "MAC / DMA" - it
seems fairly intuitive to give the best timestamp available for a given
packet, no?

> What about a TX timestamp?

I was thinking - socket flag to make packets for a given socket request
precise timestamps.
  

On Wed, Nov 22, 2023 at 08:50:00AM -0800, Jakub Kicinski wrote:
> On Wed, 22 Nov 2023 16:08:50 +0200 Vladimir Oltean wrote:
> > 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.
> 
> Well, ethtool has been the catch all for a lot of random things
> for the longest time. The question is whether we want to extend
> ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
> do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
> (i.e. getting capabilities)?
> 
> My vote is that keeping it in ethtool is less bad than 3rd API.

With SIOCSHWTSTAMP also implemented by CAN (and presumably also by
wireless in the future), I do wonder whether ethtool is the right place
for the netlink conversion.

I wouldn't suggest duplicating ETHTOOL_GET_TS_INFO towards the netdev
netlink family.

> > 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.
> 
> "Active provider" is the one that has TX_ON, rx != FILTER_NONE, right?

In the "implicit" definition of an "active hwtstamp provider", yes.
  

On Wed, Nov 22, 2023 at 08:54:59AM -0800, Jakub Kicinski wrote:
> On Wed, 22 Nov 2023 16:36:18 +0200 Vladimir Oltean wrote:
> > @Jakub, for your long-term "MAC timestamps for PTP, DMA for everything else".
> > How do you see this? I guess we need some sort of priority function in
> > the UAPI between hwtstamp providers.
> > 
> > And even with that, I think the enums that we currently have for filters
> > are not specific enough. The most we could expose is:
> > 
> >                       MAC provider                      DMA provider
> > 
> > hwtstamp_rx_filters   HWTSTAMP_FILTER_PTP_V2_EVENT      HWTSTAMP_FILTER_ALL
> > tx_type               HWTSTAMP_TX_ON                    HWTSTAMP_TX_ON
> > 
> > but it isn't clear: for PTP, does the DMA provider give you an RX
> > timestamp too?
> 
> If we phrase it as "precise / approximate" rather than "MAC / DMA" - it
> seems fairly intuitive to give the best timestamp available for a given
> packet, no?

I wouldn't be so sure. The alternative interpretation "for PTP, give me
timestamps from both sources" also sounds reasonable for the distant
future where that will be possible (with proper cmsg identification).
But I don't see how to distinguish the two - the filters, expressed in
these terms, would be the same.

> > What about a TX timestamp?
> 
> I was thinking - socket flag to make packets for a given socket request
> precise timestamps.

So the ptp4l source code would have to be modified to still work with
the same precision as before? I'm not seeing this through.
  

On Wed, 22 Nov 2023 18:59:55 +0200 Vladimir Oltean wrote:
> I wouldn't be so sure. The alternative interpretation "for PTP, give me
> timestamps from both sources" also sounds reasonable for the distant
> future where that will be possible (with proper cmsg identification).
> But I don't see how to distinguish the two - the filters, expressed in
> these terms, would be the same.

We can add an attribute that explicitly says that the configuration
is only requesting one stamp. But feels like jumping the gun at this
stage, given we have no other option to express there.

> So the ptp4l source code would have to be modified to still work with
> the same precision as before? I'm not seeing this through.

We can do the opposite and add a socket flag which says "DMA is okay".
  

On Wed, 22 Nov 2023 18:55:17 +0200 Vladimir Oltean wrote:
> > Well, ethtool has been the catch all for a lot of random things
> > for the longest time. The question is whether we want to extend
> > ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
> > do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
> > (i.e. getting capabilities)?
> > 
> > My vote is that keeping it in ethtool is less bad than 3rd API.  
> 
> With SIOCSHWTSTAMP also implemented by CAN (and presumably also by
> wireless in the future), I do wonder whether ethtool is the right place
> for the netlink conversion.

ethtool currently provides the only way we have to configure ring
length, ring count, RSS, UDP tunnels etc.

It's a matter of taste, IMO ethtool is a bit of a lost cause already
and keeping things together (ethtool already has TS_INFO) is cleaner
than spreading them around.

> I wouldn't suggest duplicating ETHTOOL_GET_TS_INFO towards the netdev
> netlink family.

FTR so far the netdev family is all about SW configuration. We should
probably keep it that way, so it doesn't become ginormous. It's easy
enough to create a new family, if needed.
  

On Wed, Nov 22, 2023 at 12:55 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 22 Nov 2023 18:59:55 +0200 Vladimir Oltean wrote:
> > I wouldn't be so sure. The alternative interpretation "for PTP, give me
> > timestamps from both sources" also sounds reasonable for the distant
> > future where that will be possible (with proper cmsg identification).
> > But I don't see how to distinguish the two - the filters, expressed in
> > these terms, would be the same.
>
> We can add an attribute that explicitly says that the configuration
> is only requesting one stamp. But feels like jumping the gun at this
> stage, given we have no other option to express there.
>
> > So the ptp4l source code would have to be modified to still work with
> > the same precision as before? I'm not seeing this through.
>
> We can do the opposite and add a socket flag which says "DMA is okay".

There already is a disconnect between configuring hardware timestamp
generation. Through the ioctl, which is a global admin-only interface.
And requesting timestamps with SO_TIMESTAMPING.

Today the user of ptp4l already has to know that the admin has
configured the right RX and TX filters. That is no different if
multiple filters can be installed? (PHY for PTP, DMA for everything
else).

If attribution becomes important, we could add another cmsg alongside
the timestamp. On TX this already happens with
IP_RECVERR/IPV6_RECVERR/PACKET_TX_TIMESTAMP. Maybe the
sock_extended_err struct even still has a field that can be (ab)used
for this purpose.

Being able to pass multiple timestamps up to userspace eventually will
be interesting. A large blocker is where to store these values in the
sk_buff on the path between the driver and the socket (skb_ext?). At
Google we already have this scenario, where the local TCP stack and
userspace both want converted hardware timestamps -- but converted
from raw to different timebases.
  

On Wed, 22 Nov 2023 10:01:42 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 22 Nov 2023 18:55:17 +0200 Vladimir Oltean wrote:
> > > Well, ethtool has been the catch all for a lot of random things
> > > for the longest time. The question is whether we want to extend
> > > ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
> > > do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
> > > (i.e. getting capabilities)?
> > > 
> > > My vote is that keeping it in ethtool is less bad than 3rd API.    
> > 
> > With SIOCSHWTSTAMP also implemented by CAN (and presumably also by
> > wireless in the future), I do wonder whether ethtool is the right place
> > for the netlink conversion.  
> 
> ethtool currently provides the only way we have to configure ring
> length, ring count, RSS, UDP tunnels etc.
> 
> It's a matter of taste, IMO ethtool is a bit of a lost cause already
> and keeping things together (ethtool already has TS_INFO) is cleaner
> than spreading them around.
> 
> > I wouldn't suggest duplicating ETHTOOL_GET_TS_INFO towards the netdev
> > netlink family.  
> 
> FTR so far the netdev family is all about SW configuration. We should
> probably keep it that way, so it doesn't become ginormous. It's easy
> enough to create a new family, if needed.

So, do we have a consensus? Vlad, do you agree on putting all under ethtool?

ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
ETHTOOL_A_TSINFO_QUALIFIER to the request.

ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
qualifier}") through the dumpit callback. I will add a filter to be able to
list only the hwtstamp provider of one netdev.

ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
implementation.

Is that ok for you?

Regards,
  

On Thu, 23 Nov 2023 16:00:56 +0100 Köry Maincent wrote:
> > FTR so far the netdev family is all about SW configuration. We should
> > probably keep it that way, so it doesn't become ginormous. It's easy
> > enough to create a new family, if needed.  
> 
> So, do we have a consensus? Vlad, do you agree on putting all under ethtool?

If not we can do a vote/poll? Maybe others don't find the configuration
of timestamping as confusing as me.
  

On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:
> > So, do we have a consensus? Vlad, do you agree on putting all under ethtool?
> > 
> > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > 
> > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > qualifier}") through the dumpit callback. I will add a filter to be able to
> > list only the hwtstamp provider of one netdev.
> > 
> > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > implementation.
> 
> If not we can do a vote/poll? Maybe others don't find the configuration
> of timestamping as confusing as me.

If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
is an ioctl), you're saying that you want to move the entire contents of
SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
addition to the existing __ethtool_get_ts_info()?

Yeah, I don't know, I don't have a real objection, I guess it's fine.

What will be a bit of an "?!" moment for users is when ethtool gains
support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
for the original ioctls. So hwstamp_ctl will be able to change timestamping
configuration, but ethtool wouldn't - all on the same system. Unless
ethtool gains an ioctl fallback for a ioctl that was never down its alley.

But by all means, still hold a poll if you want to. I would vote for
ethtool netlink, not because it's great, just because I don't have a
better alternative to propose.
  

Hi Willem,

On Wed, Nov 22, 2023 at 01:11:02PM -0500, Willem de Bruijn wrote:
> There already is a disconnect between configuring hardware timestamp
> generation. Through the ioctl, which is a global admin-only interface.
> And requesting timestamps with SO_TIMESTAMPING.
> 
> Today the user of ptp4l already has to know that the admin has
> configured the right RX and TX filters. That is no different if
> multiple filters can be installed? (PHY for PTP, DMA for everything
> else).

Are you saying that ptp4l doesn't configure the RX and TX filters by
itself, just the admin had to do that? Because it does.
https://github.com/richardcochran/linuxptp/blob/master/sk.c#L59

I'm not seeing the disconnect. SO_TIMESTAMPING is for the socket,
SIOCSHWTSTAMP is for the configuration at the device level.

It _is_ different if multiple filters can be installed, because either
we let things be (and ptp4l issues the same ioctl which affects the
default hwtstamp provider, which may or may not coincide with what we
intend), or we teach ptp4l to deal with the multitude of providers that
a port may have.
  

On Fri, 24 Nov 2023 17:43:43 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> > On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:  
> > > So, do we have a consensus? Vlad, do you agree on putting all under
> > > ethtool?
> > > 
> > > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > > 
> > > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > > qualifier}") through the dumpit callback. I will add a filter to be able
> > > to list only the hwtstamp provider of one netdev.
> > > 
> > > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > > implementation.  
> > 
> > If not we can do a vote/poll? Maybe others don't find the configuration
> > of timestamping as confusing as me.  
> 
> If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
> is an ioctl), you're saying that you want to move the entire contents of
> SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
> addition to the existing __ethtool_get_ts_info()?

Yes.
 
> Yeah, I don't know, I don't have a real objection, I guess it's fine.
> 
> What will be a bit of an "?!" moment for users is when ethtool gains
> support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
> for the original ioctls. So hwstamp_ctl will be able to change timestamping
> configuration, but ethtool wouldn't - all on the same system. Unless
> ethtool gains an ioctl fallback for a ioctl that was never down its alley.

Yes indeed. Would it break things if both ioctls and netlink can get and set
the hwtstamps configuration? It is only configuration. Both happen under
rtnl_lock it should be alright.

The question is which hwtstamp provider will the original ioctls be able to
change? Maybe the default one (MAC with phy whitelist) and only this one.

> But by all means, still hold a poll if you want to. I would vote for
> ethtool netlink, not because it's great, just because I don't have a
> better alternative to propose.

If you agree on that choice, let's go. Jakub and your are the most proactive
reviewers in this patch series. Willem you are the timestamping maintainer do
you also agree on this? 
If anyone have another proposition let them speak now, or forever remain
silent! ;)

Regards,
  

On Fri, Nov 24, 2023 at 12:28 PM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> Hi Willem,
>
> On Wed, Nov 22, 2023 at 01:11:02PM -0500, Willem de Bruijn wrote:
> > There already is a disconnect between configuring hardware timestamp
> > generation. Through the ioctl, which is a global admin-only interface.
> > And requesting timestamps with SO_TIMESTAMPING.
> >
> > Today the user of ptp4l already has to know that the admin has
> > configured the right RX and TX filters. That is no different if
> > multiple filters can be installed? (PHY for PTP, DMA for everything
> > else).
>
> Are you saying that ptp4l doesn't configure the RX and TX filters by
> itself, just the admin had to do that? Because it does.
> https://github.com/richardcochran/linuxptp/blob/master/sk.c#L59
>
> I'm not seeing the disconnect. SO_TIMESTAMPING is for the socket,
> SIOCSHWTSTAMP is for the configuration at the device level.
>
> It _is_ different if multiple filters can be installed, because either
> we let things be (and ptp4l issues the same ioctl which affects the
> default hwtstamp provider, which may or may not coincide with what we
> intend), or we teach ptp4l to deal with the multitude of providers that
> a port may have.

I see. By disconnect, I meant that the socket option is unprivileged and
can be set by many processes, while the ioctl is a global privileged
setting, so must be under control of a single admin.

But I did not know that ptp4l can take on both those roles for PTP.

Perhaps multiple SIOCSHWTSTAMP rules can coexist, up to
one per level:

HWTSTAMP_FILTER_PTP_V2_EVENT, level=PHY
HWTSTAMP_FILTER_ALL, level, level=DMA

Then ptp4l can manage all levels except the DMA level. And DMA
timestamps can be configured independently by another admin.

If only one timestamp can be communicated to the host, the earliest
match must takes precedence. Jakub pointed out how one device
handles this by having a separate queue for PHY  timestamped
packets.

This does not address the issue that packets with different
precision skb_shinfo(skb)->hwtstamps->hwtstamp may now exist
in the system. All packets reaching ptp4l sockets must have a high
resolution source, but there is no explicit annotation to ensure or
check this. This is fully based on trusting the HWSTAMP_FILTER.
Expanding the skb infra and cmsg might be follow-on work.
  

On Fri, Nov 24, 2023 at 06:34:31PM +0100, Köry Maincent wrote:
> Would it break things if both ioctls and netlink can get and set the
> hwtstamps configuration?

Uhm, obviously? It would break things if ioctl and netlink were _not_
freely interchangeable, and you couldn't see in a ioctl GET what got set
through a netlink SET.

> It is only configuration. Both happen under rtnl_lock it should be
> alright.

Yeah, but you always need to keep the API interchangeability in mind
during the implementation.

> The question is which hwtstamp provider will the original ioctls be able to
> change? Maybe the default one (MAC with phy whitelist) and only this one.

TL;DR: yeah.

Remember one single rule and go from there: new development should not
change established setups. So SIOCSHWSTAMPs should continue to behave
"as before".

This is also the exact reason why I asked for the phy whitelist. The
introduction of CONFIG_NETWORK_PHY_TIMESTAMPING introduced exactly that:
a breaking change in the mode in which deployed setups operate.

> > But by all means, still hold a poll if you want to. I would vote for
> > ethtool netlink, not because it's great, just because I don't have a
> > better alternative to propose.
> 
> If you agree on that choice, let's go. Jakub and your are the most proactive
> reviewers in this patch series. Willem you are the timestamping maintainer do
> you also agree on this? 
> If anyone have another proposition let them speak now, or forever remain
> silent! ;)

Hmm, proactive means doing stuff in anticipation of being requested to
do it. I'd use the work "active" at most...
  

On Fri, Nov 24, 2023 at 12:34 PM Köry Maincent
<kory.maincent@bootlin.com> wrote:
>
> On Fri, 24 Nov 2023 17:43:43 +0200
> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> > On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> > > On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:
> > > > So, do we have a consensus? Vlad, do you agree on putting all under
> > > > ethtool?
> > > >
> > > > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > > > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > > > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > > >
> > > > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > > > qualifier}") through the dumpit callback. I will add a filter to be able
> > > > to list only the hwtstamp provider of one netdev.
> > > >
> > > > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > > > implementation.
> > >
> > > If not we can do a vote/poll? Maybe others don't find the configuration
> > > of timestamping as confusing as me.
> >
> > If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
> > is an ioctl), you're saying that you want to move the entire contents of
> > SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
> > addition to the existing __ethtool_get_ts_info()?
>
> Yes.
>
> > Yeah, I don't know, I don't have a real objection, I guess it's fine.
> >
> > What will be a bit of an "?!" moment for users is when ethtool gains
> > support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
> > for the original ioctls. So hwstamp_ctl will be able to change timestamping
> > configuration, but ethtool wouldn't - all on the same system. Unless
> > ethtool gains an ioctl fallback for a ioctl that was never down its alley.
>
> Yes indeed. Would it break things if both ioctls and netlink can get and set
> the hwtstamps configuration? It is only configuration. Both happen under
> rtnl_lock it should be alright.
>
> The question is which hwtstamp provider will the original ioctls be able to
> change? Maybe the default one (MAC with phy whitelist) and only this one.
>
> > But by all means, still hold a poll if you want to. I would vote for
> > ethtool netlink, not because it's great, just because I don't have a
> > better alternative to propose.
>
> If you agree on that choice, let's go. Jakub and your are the most proactive
> reviewers in this patch series. Willem you are the timestamping maintainer do
> you also agree on this?

I don't have a strong opinion. Ethtool netlink SGTM.

For new network configuration we are moving away from ioctl towards
netlink in general.

Ethtool itself made this move, where the old ioctl way of things
continues to work, but will no longer be extended.

Since one of the APIs we use already uses ethtool, converting the
other two there makes sense to me.

I'm not familiar enough with configuring CAN or wireless to know
whether it would pose a problem for these mentioned cases.

> If anyone have another proposition let them speak now, or forever remain
> silent! ;)
  

On Fri, Nov 24, 2023 at 02:45:46PM -0500, Willem de Bruijn wrote:

> Expanding the skb infra and cmsg might be follow-on work.

Yes, and would I suggest limiting the scope of the present series just
to allow changing the global, device-level time stamping layer
administratively.

Trying to support multiple time stamps from different layers is a much
larger development project, and it will require new kernel/user
interfaces.

(Of course it would be grand if the series is forward looking to the
day when time stamp reporting expands beyond the current hw/sw cmsg.)

Thanks,
Richard
  

On Mon, 20 Nov 2023 13:45:51 -0800
Jakub Kicinski <kuba@kernel.org> 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?

Regards,
  

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@kernel.org> 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.
  

On Wed, 29 Nov 2023 22:37:00 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> 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@kernel.org> wrote:

> > > 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?

Yes I will, I was just saying that all the PHC would be set as precise for now.
Approximate timestamp quality won't be used because IIUC there are no NIC driver
supporting it yet.

> 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.

Alright, this seems relevant.

Regards,
  

On Wed, 29 Nov 2023 23:00:34 +0100 Köry Maincent wrote:
> > 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?  
> 
> Yes I will, I was just saying that all the PHC would be set as precise for now.
> Approximate timestamp quality won't be used because IIUC there are no NIC driver
> supporting it yet.

Agreed that we should add the attr from the start.

Maybe we can ask/work with Rahul <rrameshbabu@nvidia.com>
to implement the right thing in mlx5?

Failing that we can mark mlx5 as imprecise, until its sorted out.
So that we have both types in the tree.
  

On Wed, 29 Nov, 2023 15:56:13 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 29 Nov 2023 23:00:34 +0100 Köry Maincent wrote:
>> > 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?  
>> 
>> Yes I will, I was just saying that all the PHC would be set as precise for now.
>> Approximate timestamp quality won't be used because IIUC there are no NIC driver
>> supporting it yet.
>
> Agreed that we should add the attr from the start.
>
> Maybe we can ask/work with Rahul <rrameshbabu@nvidia.com>
> to implement the right thing in mlx5?

Thanks for looping me in. We were already looking at this patch series
out of interest. I saw your suggestion to rephrase "MAC / DMA" as
"precise / approximate", which we really like for mlx5 devices because
our "approximate" timestamping logic is not exactly a "MAC" timestamp
but its not a port timestamp that has the greater precision we use. I
have a task already for implementing support for this ethtool attribute.
If folks here are open to it, I can add mlx5 support for both modes in
this patch series for the next revision that will entail the discussed
changes.

>
> Failing that we can mark mlx5 as imprecise, until its sorted out.
> So that we have both types in the tree.

--
Thanks,

Rahul Rameshbabu