[PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

Make the sysfs knob writable, and add checks in the ioctl and time
stamping paths to respect the currently selected time stamping layer.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 .../ABI/testing/sysfs-class-net-timestamping  |  5 +-
 net/core/dev_ioctl.c                          | 44 ++++++++++++++--
 net/core/net-sysfs.c                          | 50 +++++++++++++++++--
 net/core/timestamping.c                       |  6 +++
 net/ethtool/common.c                          | 18 +++++--
 5 files changed, 111 insertions(+), 12 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
index 529c3a6eb607..6dfd59740cad 100644
--- a/Documentation/ABI/testing/sysfs-class-net-timestamping
+++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
@@ -11,7 +11,10 @@ What:		/sys/class/net/<iface>/current_timestamping_provider
 Date:		January 2022
 Contact:	Richard Cochran <richardcochran@gmail.com>
 Description:
-		Show the current SO_TIMESTAMPING provider.
+		Shows or sets the current SO_TIMESTAMPING provider.
+		When changing the value, some packets in the kernel
+		networking stack may still be delivered with time
+		stamps from the previous provider.
 		The possible values are:
 		- "mac"  The MAC provides time stamping.
 		- "phy"  The PHY or MII device provides time stamping.
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 1b807d119da5..269068ce3a51 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -260,6 +260,43 @@ static int dev_eth_ioctl(struct net_device *dev,
 	return err;
 }
 
+static int dev_hwtstamp_ioctl(struct net_device *dev,
+			      struct ifreq *ifr, unsigned int cmd)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	int err;
+
+	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
+	if (err == 0 || err != -EOPNOTSUPP)
+		return err;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	switch (dev->selected_timestamping_layer) {
+
+	case MAC_TIMESTAMPING:
+		if (ops->ndo_do_ioctl == phy_do_ioctl) {
+			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
+			err = -EOPNOTSUPP;
+		} else {
+			err = ops->ndo_eth_ioctl(dev, ifr, cmd);
+		}
+		break;
+
+	case PHY_TIMESTAMPING:
+		if (phy_has_hwtstamp(dev->phydev)) {
+			err = phy_mii_ioctl(dev->phydev, ifr, cmd);
+		} else {
+			err = -ENODEV;
+			WARN_ON(1);
+		}
+		break;
+	}
+
+	return err;
+}
+
 static int dev_siocbond(struct net_device *dev,
 			struct ifreq *ifr, unsigned int cmd)
 {
@@ -395,6 +432,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 			return err;
 		fallthrough;
 
+	case SIOCGHWTSTAMP:
+		return dev_hwtstamp_ioctl(dev, ifr, cmd);
+
 	/*
 	 *	Unknown or private ioctl
 	 */
@@ -405,9 +445,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 
 		if (cmd == SIOCGMIIPHY ||
 		    cmd == SIOCGMIIREG ||
-		    cmd == SIOCSMIIREG ||
-		    cmd == SIOCSHWTSTAMP ||
-		    cmd == SIOCGHWTSTAMP) {
+		    cmd == SIOCSMIIREG) {
 			err = dev_eth_ioctl(dev, ifr, cmd);
 		} else if (cmd == SIOCBONDENSLAVE ||
 		    cmd == SIOCBONDRELEASE ||
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4ff7ef417c38..c27f01a1a285 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -664,17 +664,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev,
 	if (!rtnl_trylock())
 		return restart_syscall();
 
-	if (phy_has_tsinfo(phydev)) {
-		ret = sprintf(buf, "%s\n", "phy");
-	} else {
+	switch (netdev->selected_timestamping_layer) {
+	case MAC_TIMESTAMPING:
 		ret = sprintf(buf, "%s\n", "mac");
+		break;
+	case PHY_TIMESTAMPING:
+		ret = sprintf(buf, "%s\n", "phy");
+		break;
 	}
 
 	rtnl_unlock();
 
 	return ret;
 }
-static DEVICE_ATTR_RO(current_timestamping_provider);
+
+static ssize_t current_timestamping_provider_store(struct device *dev,
+						   struct device_attribute *attr,
+						   const char *buf, size_t len)
+{
+	struct net_device *netdev = to_net_dev(dev);
+	struct net *net = dev_net(netdev);
+	enum timestamping_layer flavor;
+
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (!strcmp(buf, "mac\n"))
+		flavor = MAC_TIMESTAMPING;
+	else if (!strcmp(buf, "phy\n"))
+		flavor = PHY_TIMESTAMPING;
+	else
+		return -EINVAL;
+
+	if (!rtnl_trylock())
+		return restart_syscall();
+
+	if (!dev_isalive(netdev))
+		goto out;
+
+	if (netdev->selected_timestamping_layer != flavor) {
+		const struct net_device_ops *ops = netdev->netdev_ops;
+		struct ifreq ifr = {0};
+
+		/* Disable time stamping in the current layer. */
+		if (netif_device_present(netdev) && ops->ndo_eth_ioctl)
+			ops->ndo_eth_ioctl(netdev, &ifr, SIOCSHWTSTAMP);
+
+		netdev->selected_timestamping_layer = flavor;
+	}
+out:
+	rtnl_unlock();
+	return len;
+}
+static DEVICE_ATTR_RW(current_timestamping_provider);
 
 static struct attribute *net_class_attrs[] __ro_after_init = {
 	&dev_attr_netdev_group.attr,
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 04840697fe79..31c3142787b7 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -28,6 +28,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 	if (!skb->sk)
 		return;
 
+	if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING)
+		return;
+
 	type = classify(skb);
 	if (type == PTP_CLASS_NONE)
 		return;
@@ -50,6 +53,9 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
 	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
 		return false;
 
+	if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING)
+		return false;
+
 	if (skb_headroom(skb) < ETH_HLEN)
 		return false;
 
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 651d18eef589..7b50820c1d1d 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -545,10 +545,20 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 	memset(info, 0, sizeof(*info));
 	info->cmd = ETHTOOL_GET_TS_INFO;
 
-	if (phy_has_tsinfo(phydev))
-		return phy_ts_info(phydev, info);
-	if (ops->get_ts_info)
-		return ops->get_ts_info(dev, info);
+	switch (dev->selected_timestamping_layer) {
+
+	case MAC_TIMESTAMPING:
+		if (ops->get_ts_info)
+			return ops->get_ts_info(dev, info);
+		break;
+
+	case PHY_TIMESTAMPING:
+		if (phy_has_tsinfo(phydev)) {
+			return phy_ts_info(phydev, info);
+		}
+		WARN_ON(1);
+		return -ENODEV;
+	}
 
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
 				SOF_TIMESTAMPING_SOFTWARE;
-- 
2.20.1

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Russell King (Oracle)]

On Mon, Jan 03, 2022 at 03:25:54PM -0800, Richard Cochran wrote:
> Make the sysfs knob writable, and add checks in the ioctl and time
> stamping paths to respect the currently selected time stamping layer.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>

As I stated for patch 2, this patch will break mvpp2 PTP support,
since as soon as we bind a PHY, whether or not it supports PTP, you
will switch "selected_timestamping_layer" to be PHY mode PTP, and
direct all PTP calls to the PHY layer whether or not the PHY has
PTP support, away from the MAC layer.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Mon, Jan 03, 2022 at 11:53:57PM +0000, Russell King (Oracle) wrote:
> On Mon, Jan 03, 2022 at 03:25:54PM -0800, Richard Cochran wrote:
> > Make the sysfs knob writable, and add checks in the ioctl and time
> > stamping paths to respect the currently selected time stamping layer.
> > 
> > Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> 
> As I stated for patch 2, this patch will break mvpp2 PTP support,
> since as soon as we bind a PHY, whether or not it supports PTP, you
> will switch "selected_timestamping_layer" to be PHY mode PTP, and
> direct all PTP calls to the PHY layer whether or not the PHY has
> PTP support, away from the MAC layer.

Oh, that was a brain fart of mine.

I'll amend that to switch selected_timestamping_layer only if the PHY
does support time stamping.

IMO, the default should be PHY because up until now the PHY layer was
prefered.

Or would you say the MAC layer should take default priority?

(that may well break some existing systems)

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Michael Walle]

Sorry for digging out this older thread, but it seems to be discussed
in [1].

> IMO, the default should be PHY because up until now the PHY layer was
> prefered.
> 
> Or would you say the MAC layer should take default priority?
> 
> (that may well break some existing systems)

Correct me if I'm wrong, but for systems with multiple interfaces,
in particular switches, you'd need external circuits to synchronize
the PHCs within in the PHYs. (And if you use a time aware scheduler
you'd need to synchronize the MAC, too). Whereas for switches there
is usually just one PHC in the MAC which just works.

On these systems, pushing the timestamping to the PHY would mean
that this external circuitry must exist and have to be in use/
supported. MAC timestamping will work in all cases without any
external dependencies.

I'm working on a board with the LAN9668 switch which has one LAN8814
PHY and two GPY215 PHYs and two internal PHYs. The LAN9668 driver
will forward all timestamping ioctls to the PHY if it supports
timestamping (unconditionally). As soon as the patches to add ptp
support to the LAN8814 will be accepted, I guess it will break the
PTP/TAS support because there is no synchronization between all the
PHCs on that board. Thus, IMHO MAC timestamping should be the default.

-michael

[1] https://lore.kernel.org/netdev/20220308145405.GD29063@hoboy.vegasvil.org/

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Andrew Lunn]

On Mon, Apr 04, 2022 at 05:05:08PM +0200, Michael Walle wrote:
> Sorry for digging out this older thread, but it seems to be discussed
> in [1].
> 
> > IMO, the default should be PHY because up until now the PHY layer was
> > prefered.
> > 
> > Or would you say the MAC layer should take default priority?
> > 
> > (that may well break some existing systems)
> 
> Correct me if I'm wrong, but for systems with multiple interfaces,
> in particular switches, you'd need external circuits to synchronize
> the PHCs within in the PHYs.

If the PHYs are external. There are switches with internal PHYs, so
they might already have the needed synchronisation.

> (And if you use a time aware scheduler
> you'd need to synchronize the MAC, too). Whereas for switches there
> is usually just one PHC in the MAC which just works.

And there could be switches with the MACs being totally
independent. In theory.

> On these systems, pushing the timestamping to the PHY would mean
> that this external circuitry must exist and have to be in use/
> supported. MAC timestamping will work in all cases without any
> external dependencies.

And if the MAC are independent, you need the external dependency.

> I'm working on a board with the LAN9668 switch which has one LAN8814
> PHY and two GPY215 PHYs and two internal PHYs. The LAN9668 driver
> will forward all timestamping ioctls to the PHY if it supports
> timestamping (unconditionally). As soon as the patches to add ptp
> support to the LAN8814 will be accepted, I guess it will break the
> PTP/TAS support because there is no synchronization between all the
> PHCs on that board. Thus, IMHO MAC timestamping should be the default.

There are arguments for MAC being the defaults. But we must have the
option to do different, see above. But the real problem here is
history. It is very hard to change a default without breaking systems
which depend on that default. I believe Russell has a system which
will break if the default is changed.

So i suspect the default cannot be changed, but maybe we need a
mechanism where an interface or a board can express a preference it
would prefer be used when there are multiple choices, in addition to
the user space API to make the selection.

   Andrew

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Michael Walle]

Am 2022-04-04 17:20, schrieb Andrew Lunn:
> On Mon, Apr 04, 2022 at 05:05:08PM +0200, Michael Walle wrote:
>> Sorry for digging out this older thread, but it seems to be discussed
>> in [1].
>> 
>> > IMO, the default should be PHY because up until now the PHY layer was
>> > prefered.
>> >
>> > Or would you say the MAC layer should take default priority?
>> >
>> > (that may well break some existing systems)
>> 
>> Correct me if I'm wrong, but for systems with multiple interfaces,
>> in particular switches, you'd need external circuits to synchronize
>> the PHCs within in the PHYs.
> 
> If the PHYs are external. There are switches with internal PHYs, so
> they might already have the needed synchronisation.
> 
>> (And if you use a time aware scheduler
>> you'd need to synchronize the MAC, too). Whereas for switches there
>> is usually just one PHC in the MAC which just works.
> 
> And there could be switches with the MACs being totally
> independent. In theory.
> 
>> On these systems, pushing the timestamping to the PHY would mean
>> that this external circuitry must exist and have to be in use/
>> supported. MAC timestamping will work in all cases without any
>> external dependencies.
> 
> And if the MAC are independent, you need the external dependency.
> 
>> I'm working on a board with the LAN9668 switch which has one LAN8814
>> PHY and two GPY215 PHYs and two internal PHYs. The LAN9668 driver
>> will forward all timestamping ioctls to the PHY if it supports
>> timestamping (unconditionally). As soon as the patches to add ptp
>> support to the LAN8814 will be accepted, I guess it will break the
>> PTP/TAS support because there is no synchronization between all the
>> PHCs on that board. Thus, IMHO MAC timestamping should be the default.
> 
> There are arguments for MAC being the defaults. But we must have the
> option to do different, see above. But the real problem here is
> history. It is very hard to change a default without breaking systems
> which depend on that default. I believe Russell has a system which
> will break if the default is changed.
> 
> So i suspect the default cannot be changed, but maybe we need a
> mechanism where an interface or a board can express a preference it
> would prefer be used when there are multiple choices, in addition to
> the user space API to make the selection.

That would make sense. I guess what bothers me with the current
mechanism is that a feature addition to the PHY in the *future* (the
timestamping support) might break a board - or at least changes the
behavior by suddenly using PHY timestamping.

-michael

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Mon, Apr 04, 2022 at 07:12:11PM +0200, Michael Walle wrote:

> That would make sense. I guess what bothers me with the current
> mechanism is that a feature addition to the PHY in the *future* (the
> timestamping support) might break a board - or at least changes the
> behavior by suddenly using PHY timestamping.

That is a good point, but then something will break in any case.

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Michael Walle]

Am 2022-04-05 07:59, schrieb Richard Cochran:
> On Mon, Apr 04, 2022 at 07:12:11PM +0200, Michael Walle wrote:
> 
>> That would make sense. I guess what bothers me with the current
>> mechanism is that a feature addition to the PHY in the *future* (the
>> timestamping support) might break a board - or at least changes the
>> behavior by suddenly using PHY timestamping.
> 
> That is a good point, but then something will break in any case.

Can the ethernet driver select the default one? So any current
driver which has "if phy_has_hwtstamp() forward_ioctl;" can set
it to PHY while newer drivers can (or should actually) leave it as
MAC.

This doesn't really fix the problem per se. But at least new
drivers won't be affected. For my problem at hand, I'd need to
convince Microchip to default to MAC although the driver is
already in the tree (but there is no user of it in mainline
right now).

-michael

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Tue, Apr 05, 2022 at 10:48:21AM +0200, Michael Walle wrote:
> Can the ethernet driver select the default one? So any current
> driver which has "if phy_has_hwtstamp() forward_ioctl;" can set
> it to PHY while newer drivers can (or should actually) leave it as
> MAC.

I want to remove all of the phy_has_ stuff from MAC drivers.
MAC/PHY drivers should just implement the in-kernel interfaces.
IMO that is the clean way forward.

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Mon Apr 04 2022, Michael Walle wrote:
> That would make sense. I guess what bothers me with the current
> mechanism is that a feature addition to the PHY in the *future* (the
> timestamping support) might break a board - or at least changes the
> behavior by suddenly using PHY timestamping.

Currently PHY timestamping is hidden behind a configuration option
(NETWORK_PHY_TIMESTAMPING). By disabling this option the default
behavior should stay at MAC timestamping even if additional features
are added on top of the PHY drivers at later stages. Or not?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Michael Walle]

Am 2022-04-05 11:01, schrieb Kurt Kanzenbach:
> On Mon Apr 04 2022, Michael Walle wrote:
>> That would make sense. I guess what bothers me with the current
>> mechanism is that a feature addition to the PHY in the *future* (the
>> timestamping support) might break a board - or at least changes the
>> behavior by suddenly using PHY timestamping.
> 
> Currently PHY timestamping is hidden behind a configuration option
> (NETWORK_PHY_TIMESTAMPING). By disabling this option the default
> behavior should stay at MAC timestamping even if additional features
> are added on top of the PHY drivers at later stages. Or not?

That is correct. But a Kconfig option has several drawbacks:
(1) Doesn't work with boards where I might want PHY timestamping
     on *some* ports, thus I need to enable it and then stumple
     across the same problem.
(2) Doesn't work with generic distro support, which is what is
     ARM pushing right now with their SystemReady stuff (among other
     things also for embeddem system). Despite that, I have two boards
     which are already ready for booting debian out of the box for
     example. While I might convince Debian to enable that option
     (as I see it, that option is there to disable the additional
     overhead) it certainly won't be on a per board basis.
     Actually for keeping the MAC timestamping as is, you'd need to
     convince a distribution to never enable the PHY timestamping
     kconfig option.

So yes, I agree it will work when you have control over your
kconfig options, after all (1) might be more academic. But I'm
really concerned about (2).

-michael

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

On Tue Apr 05 2022, Michael Walle wrote:
> Am 2022-04-05 11:01, schrieb Kurt Kanzenbach:
>> On Mon Apr 04 2022, Michael Walle wrote:
>>> That would make sense. I guess what bothers me with the current
>>> mechanism is that a feature addition to the PHY in the *future* (the
>>> timestamping support) might break a board - or at least changes the
>>> behavior by suddenly using PHY timestamping.
>> 
>> Currently PHY timestamping is hidden behind a configuration option
>> (NETWORK_PHY_TIMESTAMPING). By disabling this option the default
>> behavior should stay at MAC timestamping even if additional features
>> are added on top of the PHY drivers at later stages. Or not?
>
> That is correct. But a Kconfig option has several drawbacks:
> (1) Doesn't work with boards where I might want PHY timestamping
>      on *some* ports, thus I need to enable it and then stumple
>      across the same problem.
> (2) Doesn't work with generic distro support, which is what is
>      ARM pushing right now with their SystemReady stuff (among other
>      things also for embeddem system). Despite that, I have two boards
>      which are already ready for booting debian out of the box for
>      example. While I might convince Debian to enable that option
>      (as I see it, that option is there to disable the additional
>      overhead) it certainly won't be on a per board basis.
>      Actually for keeping the MAC timestamping as is, you'd need to
>      convince a distribution to never enable the PHY timestamping
>      kconfig option.
>
> So yes, I agree it will work when you have control over your
> kconfig options, after all (1) might be more academic. But I'm
> really concerned about (2).

Yes, the limitations described above are exactly one of the reasons to
make the timestamping layer configurable at run time as done by these
patches.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Grygorii Strashko]

On 05/04/2022 14:19, Kurt Kanzenbach wrote:
> On Tue Apr 05 2022, Michael Walle wrote:
>> Am 2022-04-05 11:01, schrieb Kurt Kanzenbach:
>>> On Mon Apr 04 2022, Michael Walle wrote:
>>>> That would make sense. I guess what bothers me with the current
>>>> mechanism is that a feature addition to the PHY in the *future* (the
>>>> timestamping support) might break a board - or at least changes the
>>>> behavior by suddenly using PHY timestamping.
>>>
>>> Currently PHY timestamping is hidden behind a configuration option
>>> (NETWORK_PHY_TIMESTAMPING). By disabling this option the default
>>> behavior should stay at MAC timestamping even if additional features
>>> are added on top of the PHY drivers at later stages. Or not?
>>
>> That is correct. But a Kconfig option has several drawbacks:
>> (1) Doesn't work with boards where I might want PHY timestamping
>>       on *some* ports, thus I need to enable it and then stumple
>>       across the same problem.
>> (2) Doesn't work with generic distro support, which is what is
>>       ARM pushing right now with their SystemReady stuff (among other
>>       things also for embeddem system). Despite that, I have two boards
>>       which are already ready for booting debian out of the box for
>>       example. While I might convince Debian to enable that option
>>       (as I see it, that option is there to disable the additional
>>       overhead) it certainly won't be on a per board basis.
>>       Actually for keeping the MAC timestamping as is, you'd need to
>>       convince a distribution to never enable the PHY timestamping
>>       kconfig option.
>>
>> So yes, I agree it will work when you have control over your
>> kconfig options, after all (1) might be more academic. But I'm
>> really concerned about (2).
> 
> Yes, the limitations described above are exactly one of the reasons to
> make the timestamping layer configurable at run time as done by these
> patches.

Seems like PHY TS support belongs to HW description category, so could it be device tree material,
like generic property defining which layer should do timestamping?

-- 
Best regards,
Grygorii, Ukraine

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Andrew Lunn]

> > Yes, the limitations described above are exactly one of the reasons to
> > make the timestamping layer configurable at run time as done by these
> > patches.
> 
> Seems like PHY TS support belongs to HW description category, so could it be device tree material,
> like generic property defining which layer should do timestamping?

Maybe. Device tree is supposed to describe the hardware, not how you
configure the hardware. Which PTP you using is a configuration choice,
so i expect some people will argue it should not be in DT.

   Andrew

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Tue, Apr 05, 2022 at 03:29:05PM +0200, Andrew Lunn wrote:

> Maybe. Device tree is supposed to describe the hardware, not how you
> configure the hardware. Which PTP you using is a configuration choice,
> so i expect some people will argue it should not be in DT.

+1

Pure DT means no configuration choices.

(but you find many examples that break the rules!)

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Grygorii Strashko]

On 05/04/2022 18:48, Richard Cochran wrote:
> On Tue, Apr 05, 2022 at 03:29:05PM +0200, Andrew Lunn wrote:
> 
>> Maybe. Device tree is supposed to describe the hardware, not how you
>> configure the hardware. Which PTP you using is a configuration choice,
>> so i expect some people will argue it should not be in DT.
> 
> +1
> 
> Pure DT means no configuration choices.
> 
> (but you find many examples that break the rules!)
> 

My point was related to one of issues described by Michael Walle in this thread:
- supporting TS by the PHY may require also additional board support;
- phy_has_hwtstamp() defined statically by PHY drivers without taking into account board design;
- Kconfig option Doesn't really work with generic distro support and not allowed per-port cfg.

So adding smth like "hwtstamp-en" will clear identify that this particular PHY on this particular board
supports time stamping.
(or hwtstamp-full/hwtstamp-rx/hwtstamp-tx).

Of course, it will not help with default or dynamic selection of time stamping layer :(,
but it will be one problem less.

-- 
Best regards,
Grygorii, Ukraine

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

Hi Richard,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Richard-Cochran/Make-MAC-PHY-time-stamping-selectable/20220104-072717
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3d694552fd8fe502e7ffd22ffa0e085bfd73b19a
config: nds32-defconfig (https://download.01.org/0day-ci/archive/20220104/202201041453.ofyk2zEq-lkp(a)intel.com/config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/33070417b78b3844a2b2f55f94313072859da977
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Richard-Cochran/Make-MAC-PHY-time-stamping-selectable/20220104-072717
        git checkout 33070417b78b3844a2b2f55f94313072859da977
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nds32 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   nds32le-linux-ld: net/core/dev_ioctl.o: in function `dev_ifsioc':
>> dev_ioctl.c:(.text+0x4e0): undefined reference to `phy_do_ioctl'
>> nds32le-linux-ld: dev_ioctl.c:(.text+0x4e4): undefined reference to `phy_do_ioctl'
>> nds32le-linux-ld: dev_ioctl.c:(.text+0x576): undefined reference to `phy_mii_ioctl'
   nds32le-linux-ld: dev_ioctl.c:(.text+0x57a): undefined reference to `phy_mii_ioctl'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

Hi Richard,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Richard-Cochran/Make-MAC-PHY-time-stamping-selectable/20220104-072717
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3d694552fd8fe502e7ffd22ffa0e085bfd73b19a
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20220104/202201041412.WPk0XCA0-lkp(a)intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/33070417b78b3844a2b2f55f94313072859da977
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Richard-Cochran/Make-MAC-PHY-time-stamping-selectable/20220104-072717
        git checkout 33070417b78b3844a2b2f55f94313072859da977
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   hppa-linux-ld: net/core/dev_ioctl.o: in function `dev_ifsioc':
>> (.text+0x1b0): undefined reference to `phy_mii_ioctl'
   hppa-linux-ld: net/core/dev_ioctl.o: in function `.LC2':
>> (.rodata.cst4+0x0): undefined reference to `phy_do_ioctl'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

Hi Richard,

On Mon, Jan 03, 2022 at 03:25:54PM -0800, Richard Cochran wrote:
> Make the sysfs knob writable, and add checks in the ioctl and time
> stamping paths to respect the currently selected time stamping layer.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---

Could we think of a more flexible solution? Your proposal would not
allow a packet to have multiple hwtstamps, and maybe that would be
interesting for some use cases (hardware testing, mostly).
The problem with that, really, is that user space doesn't know which PHC
is a hardware timestamp coming from - this info isn't present in the
struct scm_timestamping presented in the SOL_SOCKET/SCM_TIMESTAMPING
cmsg.

This is also the reason why DSA denies PTP timestamping on the master
interface, although there isn't any physical reason to do that. For the
same reason mentioned earlier, it would be nice to see hwtstamps for a
packet as it traverses DSA master -> DSA switch port -> PHY attached to
DSA switch.

With a new SO_TIMESTAMPING API (say, SOF_TIMESTAMPING_SELECT_PHC |
SOF_TIMESTAMPING_PHY, plus a new SCM_TIMESTAMP_PHC enum that would also
contain the PHC index), we could make this a per-socket option. Kernel
keeps track of whether any socket requests PHY timestamping, and
enables/disables PHY timestamping as needed. Your patch replays a call
to ops->ndo_eth_ioctl() from current_timestamping_provider_store()
anyway (I mean creates a call from outside its intended calling context),
we'd need similar logic to call that function from sock_set_timestamping()
or thereabouts.

Do you consider this a valid use case? Different approaches for
different needs, I suppose. I guess what you want to achieve is for
ptp4l to not really care who is the timestamp provider.

>  .../ABI/testing/sysfs-class-net-timestamping  |  5 +-
>  net/core/dev_ioctl.c                          | 44 ++++++++++++++--
>  net/core/net-sysfs.c                          | 50 +++++++++++++++++--
>  net/core/timestamping.c                       |  6 +++
>  net/ethtool/common.c                          | 18 +++++--
>  5 files changed, 111 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
> index 529c3a6eb607..6dfd59740cad 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-timestamping
> +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
> @@ -11,7 +11,10 @@ What:		/sys/class/net/<iface>/current_timestamping_provider
>  Date:		January 2022
>  Contact:	Richard Cochran <richardcochran@gmail.com>
>  Description:
> -		Show the current SO_TIMESTAMPING provider.
> +		Shows or sets the current SO_TIMESTAMPING provider.
> +		When changing the value, some packets in the kernel
> +		networking stack may still be delivered with time
> +		stamps from the previous provider.
>  		The possible values are:
>  		- "mac"  The MAC provides time stamping.
>  		- "phy"  The PHY or MII device provides time stamping.
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 1b807d119da5..269068ce3a51 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -260,6 +260,43 @@ static int dev_eth_ioctl(struct net_device *dev,
>  	return err;
>  }
>  
> +static int dev_hwtstamp_ioctl(struct net_device *dev,
> +			      struct ifreq *ifr, unsigned int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err;
> +
> +	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
> +	if (err == 0 || err != -EOPNOTSUPP)
> +		return err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	switch (dev->selected_timestamping_layer) {
> +
> +	case MAC_TIMESTAMPING:
> +		if (ops->ndo_do_ioctl == phy_do_ioctl) {
> +			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
> +			err = -EOPNOTSUPP;
> +		} else {
> +			err = ops->ndo_eth_ioctl(dev, ifr, cmd);
> +		}
> +		break;
> +
> +	case PHY_TIMESTAMPING:
> +		if (phy_has_hwtstamp(dev->phydev)) {
> +			err = phy_mii_ioctl(dev->phydev, ifr, cmd);
> +		} else {
> +			err = -ENODEV;
> +			WARN_ON(1);
> +		}
> +		break;
> +	}
> +
> +	return err;
> +}
> +
>  static int dev_siocbond(struct net_device *dev,
>  			struct ifreq *ifr, unsigned int cmd)
>  {
> @@ -395,6 +432,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  			return err;
>  		fallthrough;
>  
> +	case SIOCGHWTSTAMP:
> +		return dev_hwtstamp_ioctl(dev, ifr, cmd);
> +
>  	/*
>  	 *	Unknown or private ioctl
>  	 */
> @@ -405,9 +445,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  
>  		if (cmd == SIOCGMIIPHY ||
>  		    cmd == SIOCGMIIREG ||
> -		    cmd == SIOCSMIIREG ||
> -		    cmd == SIOCSHWTSTAMP ||
> -		    cmd == SIOCGHWTSTAMP) {
> +		    cmd == SIOCSMIIREG) {
>  			err = dev_eth_ioctl(dev, ifr, cmd);
>  		} else if (cmd == SIOCBONDENSLAVE ||
>  		    cmd == SIOCBONDRELEASE ||
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 4ff7ef417c38..c27f01a1a285 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -664,17 +664,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev,
>  	if (!rtnl_trylock())
>  		return restart_syscall();
>  
> -	if (phy_has_tsinfo(phydev)) {
> -		ret = sprintf(buf, "%s\n", "phy");
> -	} else {
> +	switch (netdev->selected_timestamping_layer) {
> +	case MAC_TIMESTAMPING:
>  		ret = sprintf(buf, "%s\n", "mac");
> +		break;
> +	case PHY_TIMESTAMPING:
> +		ret = sprintf(buf, "%s\n", "phy");
> +		break;
>  	}
>  
>  	rtnl_unlock();
>  
>  	return ret;
>  }
> -static DEVICE_ATTR_RO(current_timestamping_provider);
> +
> +static ssize_t current_timestamping_provider_store(struct device *dev,
> +						   struct device_attribute *attr,
> +						   const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(dev);
> +	struct net *net = dev_net(netdev);
> +	enum timestamping_layer flavor;
> +
> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	if (!strcmp(buf, "mac\n"))
> +		flavor = MAC_TIMESTAMPING;
> +	else if (!strcmp(buf, "phy\n"))
> +		flavor = PHY_TIMESTAMPING;

Shouldn't we use sysfs_streq()?

> +	else
> +		return -EINVAL;
> +
> +	if (!rtnl_trylock())
> +		return restart_syscall();
> +
> +	if (!dev_isalive(netdev))
> +		goto out;
> +
> +	if (netdev->selected_timestamping_layer != flavor) {
> +		const struct net_device_ops *ops = netdev->netdev_ops;
> +		struct ifreq ifr = {0};
> +
> +		/* Disable time stamping in the current layer. */
> +		if (netif_device_present(netdev) && ops->ndo_eth_ioctl)
> +			ops->ndo_eth_ioctl(netdev, &ifr, SIOCSHWTSTAMP);
> +
> +		netdev->selected_timestamping_layer = flavor;

I'm unclear about this. If MAC timestamping was previously enabled on
the interface, ptp4l is running, and the admin surprise-changes it to
PHY, this will not enable PHY timestamping, will it? So the ptp4l
application must be restarted.

If I'm correct about this, then it would be cleaner to use the
setsockopt interface, since the kernel would have a better way of not
changing stuff from under existing processes' feet.

> +	}
> +out:
> +	rtnl_unlock();
> +	return len;
> +}
> +static DEVICE_ATTR_RW(current_timestamping_provider);
>  
>  static struct attribute *net_class_attrs[] __ro_after_init = {
>  	&dev_attr_netdev_group.attr,
> diff --git a/net/core/timestamping.c b/net/core/timestamping.c
> index 04840697fe79..31c3142787b7 100644
> --- a/net/core/timestamping.c
> +++ b/net/core/timestamping.c
> @@ -28,6 +28,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
>  	if (!skb->sk)
>  		return;
>  
> +	if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING)
> +		return;
> +
>  	type = classify(skb);
>  	if (type == PTP_CLASS_NONE)
>  		return;
> @@ -50,6 +53,9 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
>  	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
>  		return false;
>  
> +	if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING)
> +		return false;
> +
>  	if (skb_headroom(skb) < ETH_HLEN)
>  		return false;
>  
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 651d18eef589..7b50820c1d1d 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -545,10 +545,20 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
>  	memset(info, 0, sizeof(*info));
>  	info->cmd = ETHTOOL_GET_TS_INFO;
>  
> -	if (phy_has_tsinfo(phydev))
> -		return phy_ts_info(phydev, info);
> -	if (ops->get_ts_info)
> -		return ops->get_ts_info(dev, info);
> +	switch (dev->selected_timestamping_layer) {
> +
> +	case MAC_TIMESTAMPING:
> +		if (ops->get_ts_info)
> +			return ops->get_ts_info(dev, info);
> +		break;
> +
> +	case PHY_TIMESTAMPING:
> +		if (phy_has_tsinfo(phydev)) {
> +			return phy_ts_info(phydev, info);
> +		}
> +		WARN_ON(1);
> +		return -ENODEV;
> +	}
>  
>  	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
>  				SOF_TIMESTAMPING_SOFTWARE;
> -- 
> 2.20.1
>

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Andrew Lunn]

> This is also the reason why DSA denies PTP timestamping on the master
> interface, although there isn't any physical reason to do that. For the
> same reason mentioned earlier, it would be nice to see hwtstamps for a
> packet as it traverses DSA master -> DSA switch port -> PHY attached to
> DSA switch.

Don't forget there could be back to back PHYs between the master and
the DSA switch port. In theory they could also be doing time stamping.

Also consider the case of a switch port connected to a PHY which does
media conversion to SFP. And the SFP has a copper module, so contains
another PHY. So you could have the MAC and both PHYs doing time
stamping?

So in the extreme case, you have 7 time stamps, 3 from MACs and 4 from
PHYs!

I doubt we want to support this, is there a valid use case for it?

  Andrew

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Fri, Jan 21, 2022 at 04:38:09AM +0100, Andrew Lunn wrote:

> So in the extreme case, you have 7 time stamps, 3 from MACs and 4 from
> PHYs!

:^)
 
> I doubt we want to support this, is there a valid use case for it?

Someday, someone will surely say it is important, but with any luck
I'll be dead by then...

Cheers,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Thu, Jan 20, 2022 at 08:05:08PM -0800, Richard Cochran wrote:
> On Fri, Jan 21, 2022 at 04:38:09AM +0100, Andrew Lunn wrote:
> 
> > So in the extreme case, you have 7 time stamps, 3 from MACs and 4 from
> > PHYs!
> 
> :^)
>  
> > I doubt we want to support this, is there a valid use case for it?
> 
> Someday, someone will surely say it is important, but with any luck
> I'll be dead by then...

So as I mentioned earlier, the use case would be hardware performance
testing and diagnosing. You may consider that as not that important, but
this is basically what I had to do for several months, and even wrote
a program for that, that collects packet timestamps at all possible points.
And to your point, the more complex a system is, the more important it
becomes to be able to diagnose it precisely.

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Fri, Jan 21, 2022 at 02:50:36PM +0000, Vladimir Oltean wrote:
> So as I mentioned earlier, the use case would be hardware performance
> testing and diagnosing. You may consider that as not that important, but
> this is basically what I had to do for several months, and even wrote
> a program for that, that collects packet timestamps at all possible points.

This is not possible without making a brand new CMSG to accommodate
time stamps from all the various layers.

That is completely out of scope for this series.

The only practical use case of this series is to switch from PHY back to MAC.

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Fri, Jan 21, 2022 at 07:28:20AM -0800, Richard Cochran wrote:
> On Fri, Jan 21, 2022 at 02:50:36PM +0000, Vladimir Oltean wrote:
> > So as I mentioned earlier, the use case would be hardware performance
> > testing and diagnosing. You may consider that as not that important, but
> > this is basically what I had to do for several months, and even wrote
> > a program for that, that collects packet timestamps at all possible points.
> 
> This is not possible without making a brand new CMSG to accommodate
> time stamps from all the various layers.
> 
> That is completely out of scope for this series.
> 
> The only practical use case of this series is to switch from PHY back to MAC.

I don't think my proposal is out of scope. It deals with the same thing
as what you propose: the kernel makes a selection by default, user space
can change it. The need for PHC identification in the cmsg arises as a
direct consequence of the fact that there are multiple PHCs in the path.
It isn't a requirement that I introduced artificially. Your solution has
a need to deal with that problem too, it's just that it omits to do so:

|               When changing the value, some packets in the kernel
|               networking stack may still be delivered with time
|               stamps from the previous provider.

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Fri, Jan 21, 2022 at 04:23:27PM +0000, Vladimir Oltean wrote:
> On Fri, Jan 21, 2022 at 07:28:20AM -0800, Richard Cochran wrote:
> > On Fri, Jan 21, 2022 at 02:50:36PM +0000, Vladimir Oltean wrote:
> > > So as I mentioned earlier, the use case would be hardware performance
> > > testing and diagnosing. You may consider that as not that important, but
> > > this is basically what I had to do for several months, and even wrote
> > > a program for that, that collects packet timestamps at all possible points.
> > 
> > This is not possible without making a brand new CMSG to accommodate
> > time stamps from all the various layers.
> > 
> > That is completely out of scope for this series.
> > 
> > The only practical use case of this series is to switch from PHY back to MAC.
> 
> I don't think my proposal is out of scope.

I don't see a way to implement your proposal at all.

If you want to implement your proposal and submit a patch series, I
will gladly drop mine in favor of yours.  Otherwise, my series solves
a real world problem and improves the stack at least a little bit.

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Miroslav Lichvar]

On Fri, Jan 21, 2022 at 07:28:20AM -0800, Richard Cochran wrote:
> On Fri, Jan 21, 2022 at 02:50:36PM +0000, Vladimir Oltean wrote:
> > So as I mentioned earlier, the use case would be hardware performance
> > testing and diagnosing. You may consider that as not that important, but
> > this is basically what I had to do for several months, and even wrote
> > a program for that, that collects packet timestamps at all possible points.
> 
> This is not possible without making a brand new CMSG to accommodate
> time stamps from all the various layers.

FWIW, scm_timestamping has three fields and the middle one no longer
seems to be used. If a new socket/timestamping option enabled all
three (SW, MAC, PHY) timestamps in the cmsg, I think that would be a
nice feature.

There are applications that receive both SW and HW timestamps in order
to fall back to SW when a HW timestamp glitched or is missing. This
could be extended to three levels with MAC and PHY timestamps.

> That is completely out of scope for this series.
> 
> The only practical use case of this series is to switch from PHY back to MAC.

From an admin point of view, it makes sense to me to have an option to
disable PHY timestamps for the whole device if there are issues with
it. For debugging and applications, it would be nice to have an option
to get all of them at the same time.

-- 
Miroslav Lichvar

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Mon, Jan 24, 2022 at 10:28:23AM +0100, Miroslav Lichvar wrote:

> FWIW, scm_timestamping has three fields and the middle one no longer
> seems to be used. If a new socket/timestamping option enabled all
> three (SW, MAC, PHY) timestamps in the cmsg, I think that would be a
> nice feature.

This won't work because:

- There would need to be seven^W eight, not three slots.

- Even with just three, the CMSG would have to have a bit that clearly
  identifies the new format.
 
> From an admin point of view, it makes sense to me to have an option to
> disable PHY timestamps for the whole device if there are issues with
> it. For debugging and applications, it would be nice to have an option
> to get all of them at the same time.

Right.  Those are two different use cases.  The present series
addresses the first one.  The second one entails making a new flavor
of time stamping API.

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Mon, Jan 24, 2022 at 07:37:16AM -0800, Richard Cochran wrote:
> On Mon, Jan 24, 2022 at 10:28:23AM +0100, Miroslav Lichvar wrote:
> 
> > FWIW, scm_timestamping has three fields and the middle one no longer
> > seems to be used. If a new socket/timestamping option enabled all
> > three (SW, MAC, PHY) timestamps in the cmsg, I think that would be a
> > nice feature.
> 
> This won't work because:
> 
> - There would need to be seven^W eight, not three slots.
> 
> - Even with just three, the CMSG would have to have a bit that clearly
>   identifies the new format.
>  
> > From an admin point of view, it makes sense to me to have an option to
> > disable PHY timestamps for the whole device if there are issues with
> > it. For debugging and applications, it would be nice to have an option
> > to get all of them at the same time.
> 
> Right.  Those are two different use cases.  The present series
> addresses the first one.  The second one entails making a new flavor
> of time stamping API.

I agree that they are different use cases, but with the runtime PHC
change being now possible, there isn't really a clear-cut moment when an
application can now say "from this moment on, I'm only getting
timestamps from the new PHC". Especially when you switch over from the
PHY to the MAC, a few tens of ms may pass until you get an RX timestamp
from the PHY, and the MAC PHC may well be up and operational in the
meantime, and ptp4l synchronizing based on its timestamps.

I don't have a clear idea how to put it in practice, either. I'll think
about it, but the problem I see is that the phc_index reported by
"ethtool -T" becomes a fuzzy concept in the presence of multiple PHCs.
Adding the ability to retrieve timestamps from all of them doesn't make
it any easier to select which one gets reported to ethtool.
Maybe there's room for both the sysfs and the socket option after all.

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Thu Jan 20 2022, Vladimir Oltean wrote:
> Hi Richard,
>
> On Mon, Jan 03, 2022 at 03:25:54PM -0800, Richard Cochran wrote:
>> Make the sysfs knob writable, and add checks in the ioctl and time
>> stamping paths to respect the currently selected time stamping layer.
>> 
>> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
>> ---
>
> Could we think of a more flexible solution? Your proposal would not
> allow a packet to have multiple hwtstamps, and maybe that would be
> interesting for some use cases (hardware testing, mostly).

One use case i can think of for having multiple hwtimestamps per packet
is crosstimestamping. Some devices such as hellcreek have multiple PHCs
and allow generation of such crosstimestamps.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[Richard Cochran]

On Fri, Jan 21, 2022 at 12:05:22PM +0100, Kurt Kanzenbach wrote:
> On Thu Jan 20 2022, Vladimir Oltean wrote:
> > Hi Richard,
> >
> > On Mon, Jan 03, 2022 at 03:25:54PM -0800, Richard Cochran wrote:
> >> Make the sysfs knob writable, and add checks in the ioctl and time
> >> stamping paths to respect the currently selected time stamping layer.
> >> 
> >> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> >> ---
> >
> > Could we think of a more flexible solution? Your proposal would not
> > allow a packet to have multiple hwtstamps, and maybe that would be
> > interesting for some use cases (hardware testing, mostly).
> 
> One use case i can think of for having multiple hwtimestamps per packet
> is crosstimestamping. Some devices such as hellcreek have multiple PHCs
> and allow generation of such crosstimestamps.

That may well be nice to have, but that is not in scope for this series.

Thanks,
Richard

Re: [PATCH RFC V1 net-next 3/4] net: Let the active time stamping layer be selectable.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 18183 bytes --]

CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220103232555.19791-4-richardcochran@gmail.com>
References: <20220103232555.19791-4-richardcochran@gmail.com>
TO: Richard Cochran <richardcochran@gmail.com>

Hi Richard,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Richard-Cochran/Make-MAC-PHY-time-stamping-selectable/20220104-072717
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3d694552fd8fe502e7ffd22ffa0e085bfd73b19a
:::::: branch date: 7 days ago
:::::: commit date: 7 days ago
config: x86_64-randconfig-c007-20220105 (https://download.01.org/0day-ci/archive/20220110/202201102352.6hOYMc7y-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d5b6e30ed3acad794dd0aec400e617daffc6cc3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/33070417b78b3844a2b2f55f94313072859da977
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Richard-Cochran/Make-MAC-PHY-time-stamping-selectable/20220104-072717
        git checkout 33070417b78b3844a2b2f55f94313072859da977
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
                              ^
   include/linux/compiler_types.h:302:3: note: expanded from macro '__native_word'
           (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
            ^
   drivers/infiniband/core/addr.c:508:28: note: Left side of '||' is false
           struct net_device *ndev = READ_ONCE(dst->dev);
                                     ^
   include/asm-generic/rwonce.h:49:2: note: expanded from macro 'READ_ONCE'
           compiletime_assert_rwonce_type(x);                              \
           ^
   include/asm-generic/rwonce.h:36:21: note: expanded from macro 'compiletime_assert_rwonce_type'
           compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
                              ^
   include/linux/compiler_types.h:302:3: note: expanded from macro '__native_word'
           (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
            ^
   drivers/infiniband/core/addr.c:508:28: note: Left side of '||' is false
           struct net_device *ndev = READ_ONCE(dst->dev);
                                     ^
   include/asm-generic/rwonce.h:49:2: note: expanded from macro 'READ_ONCE'
           compiletime_assert_rwonce_type(x);                              \
           ^
   include/asm-generic/rwonce.h:36:21: note: expanded from macro 'compiletime_assert_rwonce_type'
           compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
                              ^
   include/linux/compiler_types.h:302:3: note: expanded from macro '__native_word'
           (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
            ^
   drivers/infiniband/core/addr.c:508:28: note: Left side of '||' is true
           struct net_device *ndev = READ_ONCE(dst->dev);
                                     ^
   include/asm-generic/rwonce.h:49:2: note: expanded from macro 'READ_ONCE'
           compiletime_assert_rwonce_type(x);                              \
           ^
   include/asm-generic/rwonce.h:36:38: note: expanded from macro 'compiletime_assert_rwonce_type'
           compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
                                               ^
   drivers/infiniband/core/addr.c:508:28: note: Taking false branch
           struct net_device *ndev = READ_ONCE(dst->dev);
                                     ^
   include/asm-generic/rwonce.h:49:2: note: expanded from macro 'READ_ONCE'
           compiletime_assert_rwonce_type(x);                              \
           ^
   include/asm-generic/rwonce.h:36:2: note: expanded from macro 'compiletime_assert_rwonce_type'
           compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
           ^
   include/linux/compiler_types.h:335:2: note: expanded from macro 'compiletime_assert'
           _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
           ^
   include/linux/compiler_types.h:323:2: note: expanded from macro '_compiletime_assert'
           __compiletime_assert(condition, msg, prefix, suffix)
           ^
   include/linux/compiler_types.h:315:3: note: expanded from macro '__compiletime_assert'
                   if (!(condition))                                       \
                   ^
   include/linux/compiler.h:56:23: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                         ^
   drivers/infiniband/core/addr.c:508:28: note: Loop condition is false.  Exiting loop
           struct net_device *ndev = READ_ONCE(dst->dev);
                                     ^
   include/asm-generic/rwonce.h:49:2: note: expanded from macro 'READ_ONCE'
           compiletime_assert_rwonce_type(x);                              \
           ^
   include/asm-generic/rwonce.h:36:2: note: expanded from macro 'compiletime_assert_rwonce_type'
           compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
           ^
   include/linux/compiler_types.h:335:2: note: expanded from macro 'compiletime_assert'
           _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
           ^
   include/linux/compiler_types.h:323:2: note: expanded from macro '_compiletime_assert'
           __compiletime_assert(condition, msg, prefix, suffix)
           ^
   include/linux/compiler_types.h:307:2: note: expanded from macro '__compiletime_assert'
           do {                                                            \
           ^
   drivers/infiniband/core/addr.c:508:28: note: Dereference of null pointer
           struct net_device *ndev = READ_ONCE(dst->dev);
                                     ^
   include/asm-generic/rwonce.h:50:2: note: expanded from macro 'READ_ONCE'
           __READ_ONCE(x);                                                 \
           ^~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:44:24: note: expanded from macro '__READ_ONCE'
   #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 8 warnings (8 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   16 warnings generated.
   net/core/net-sysfs.c:632:2: warning: Value stored to 'ops' is never read [clang-analyzer-deadcode.DeadStores]
           ops = netdev->ethtool_ops;
           ^     ~~~~~~~~~~~~~~~~~~~
   net/core/net-sysfs.c:632:2: note: Value stored to 'ops' is never read
           ops = netdev->ethtool_ops;
           ^     ~~~~~~~~~~~~~~~~~~~
   net/core/net-sysfs.c:642:3: warning: Value stored to 'buf' is never read [clang-analyzer-deadcode.DeadStores]
                   buf += 4;
                   ^      ~
   net/core/net-sysfs.c:642:3: note: Value stored to 'buf' is never read
                   buf += 4;
                   ^      ~
>> net/core/net-sysfs.c:661:2: warning: Value stored to 'phydev' is never read [clang-analyzer-deadcode.DeadStores]
           phydev = netdev->phydev;
           ^        ~~~~~~~~~~~~~~
   net/core/net-sysfs.c:661:2: note: Value stored to 'phydev' is never read
           phydev = netdev->phydev;
           ^        ~~~~~~~~~~~~~~
   net/core/net-sysfs.c:662:2: warning: Value stored to 'ops' is never read [clang-analyzer-deadcode.DeadStores]
           ops = netdev->ethtool_ops;
           ^     ~~~~~~~~~~~~~~~~~~~
   net/core/net-sysfs.c:662:2: note: Value stored to 'ops' is never read
           ops = netdev->ethtool_ops;
           ^     ~~~~~~~~~~~~~~~~~~~
   net/core/net-sysfs.c:1871:2: warning: Value stored to 'txq' is never read [clang-analyzer-deadcode.DeadStores]
           txq = real_tx;
           ^     ~~~~~~~
   net/core/net-sysfs.c:1871:2: note: Value stored to 'txq' is never read
           txq = real_tx;
           ^     ~~~~~~~
   Suppressed 11 warnings (8 in non-user code, 3 with check filters).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   8 warnings generated.
   Suppressed 8 warnings (8 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   8 warnings generated.
   Suppressed 8 warnings (8 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   57 warnings generated.
   Suppressed 57 warnings (57 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   9 warnings generated.
   net/core/drop_monitor.c:870:2: warning: 1st function call argument is an uninitialized value [clang-analyzer-core.CallAndMessage]
           kfree(hw_metadata->fa_cookie);
           ^
   net/core/drop_monitor.c:941:6: note: Assuming field 'trap_type' is not equal to DEVLINK_TRAP_TYPE_CONTROL
           if (metadata->trap_type == DEVLINK_TRAP_TYPE_CONTROL)
               ^
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                      ^~~~
   net/core/drop_monitor.c:941:2: note: '?' condition is false
           if (metadata->trap_type == DEVLINK_TRAP_TYPE_CONTROL)
           ^
   include/linux/compiler.h:56:28: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                              ^
   include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                 ^
   net/core/drop_monitor.c:941:16: note: Field 'trap_type' is not equal to DEVLINK_TRAP_TYPE_CONTROL
           if (metadata->trap_type == DEVLINK_TRAP_TYPE_CONTROL)
                         ^
   net/core/drop_monitor.c:941:2: note: '?' condition is false
           if (metadata->trap_type == DEVLINK_TRAP_TYPE_CONTROL)
           ^
   include/linux/compiler.h:56:28: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                              ^
   include/linux/compiler.h:58:69: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                       ^
   include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
           (cond) ?                                        \
           ^
   net/core/drop_monitor.c:941:2: note: Taking false branch
           if (metadata->trap_type == DEVLINK_TRAP_TYPE_CONTROL)
           ^
   include/linux/compiler.h:56:23: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                         ^
   net/core/drop_monitor.c:944:2: note: '?' condition is false
           if (!skb_mac_header_was_set(skb))
           ^
   include/linux/compiler.h:56:28: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                              ^
   include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                 ^
   net/core/drop_monitor.c:944:2: note: '?' condition is false
           if (!skb_mac_header_was_set(skb))
           ^
   include/linux/compiler.h:56:28: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                              ^
   include/linux/compiler.h:58:69: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                       ^
   include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
           (cond) ?                                        \
           ^
   net/core/drop_monitor.c:944:2: note: Taking false branch
           if (!skb_mac_header_was_set(skb))
           ^
   include/linux/compiler.h:56:23: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                         ^
   net/core/drop_monitor.c:948:6: note: Assuming 'nskb' is non-null
           if (!nskb)

vim +/phydev +661 net/core/net-sysfs.c

5fdd2f0e5c6484 Wei Wang        2021-02-08  620  
c7ffd872b92c08 Richard Cochran 2022-01-03  621  static ssize_t available_timestamping_providers_show(struct device *dev,
c7ffd872b92c08 Richard Cochran 2022-01-03  622  						     struct device_attribute *attr,
c7ffd872b92c08 Richard Cochran 2022-01-03  623  						     char *buf)
c7ffd872b92c08 Richard Cochran 2022-01-03  624  {
c7ffd872b92c08 Richard Cochran 2022-01-03  625  	const struct ethtool_ops *ops;
c7ffd872b92c08 Richard Cochran 2022-01-03  626  	struct net_device *netdev;
c7ffd872b92c08 Richard Cochran 2022-01-03  627  	struct phy_device *phydev;
c7ffd872b92c08 Richard Cochran 2022-01-03  628  	int ret = 0;
c7ffd872b92c08 Richard Cochran 2022-01-03  629  
c7ffd872b92c08 Richard Cochran 2022-01-03  630  	netdev = to_net_dev(dev);
c7ffd872b92c08 Richard Cochran 2022-01-03  631  	phydev = netdev->phydev;
c7ffd872b92c08 Richard Cochran 2022-01-03 @632  	ops = netdev->ethtool_ops;
c7ffd872b92c08 Richard Cochran 2022-01-03  633  
c7ffd872b92c08 Richard Cochran 2022-01-03  634  	if (!rtnl_trylock())
c7ffd872b92c08 Richard Cochran 2022-01-03  635  		return restart_syscall();
c7ffd872b92c08 Richard Cochran 2022-01-03  636  
c7ffd872b92c08 Richard Cochran 2022-01-03  637  	ret += sprintf(buf, "%s\n", "mac");
c7ffd872b92c08 Richard Cochran 2022-01-03  638  	buf += 4;
c7ffd872b92c08 Richard Cochran 2022-01-03  639  
c7ffd872b92c08 Richard Cochran 2022-01-03  640  	if (phy_has_tsinfo(phydev)) {
c7ffd872b92c08 Richard Cochran 2022-01-03  641  		ret += sprintf(buf, "%s\n", "phy");
c7ffd872b92c08 Richard Cochran 2022-01-03  642  		buf += 4;
c7ffd872b92c08 Richard Cochran 2022-01-03  643  	}
c7ffd872b92c08 Richard Cochran 2022-01-03  644  
c7ffd872b92c08 Richard Cochran 2022-01-03  645  	rtnl_unlock();
c7ffd872b92c08 Richard Cochran 2022-01-03  646  
c7ffd872b92c08 Richard Cochran 2022-01-03  647  	return ret;
c7ffd872b92c08 Richard Cochran 2022-01-03  648  }
c7ffd872b92c08 Richard Cochran 2022-01-03  649  static DEVICE_ATTR_RO(available_timestamping_providers);
c7ffd872b92c08 Richard Cochran 2022-01-03  650  
c7ffd872b92c08 Richard Cochran 2022-01-03  651  static ssize_t current_timestamping_provider_show(struct device *dev,
c7ffd872b92c08 Richard Cochran 2022-01-03  652  						  struct device_attribute *attr,
c7ffd872b92c08 Richard Cochran 2022-01-03  653  						  char *buf)
c7ffd872b92c08 Richard Cochran 2022-01-03  654  {
c7ffd872b92c08 Richard Cochran 2022-01-03  655  	const struct ethtool_ops *ops;
c7ffd872b92c08 Richard Cochran 2022-01-03  656  	struct net_device *netdev;
c7ffd872b92c08 Richard Cochran 2022-01-03  657  	struct phy_device *phydev;
c7ffd872b92c08 Richard Cochran 2022-01-03  658  	int ret;
c7ffd872b92c08 Richard Cochran 2022-01-03  659  
c7ffd872b92c08 Richard Cochran 2022-01-03  660  	netdev = to_net_dev(dev);
c7ffd872b92c08 Richard Cochran 2022-01-03 @661  	phydev = netdev->phydev;
c7ffd872b92c08 Richard Cochran 2022-01-03  662  	ops = netdev->ethtool_ops;
c7ffd872b92c08 Richard Cochran 2022-01-03  663  
c7ffd872b92c08 Richard Cochran 2022-01-03  664  	if (!rtnl_trylock())
c7ffd872b92c08 Richard Cochran 2022-01-03  665  		return restart_syscall();
c7ffd872b92c08 Richard Cochran 2022-01-03  666  
33070417b78b38 Richard Cochran 2022-01-03  667  	switch (netdev->selected_timestamping_layer) {
33070417b78b38 Richard Cochran 2022-01-03  668  	case MAC_TIMESTAMPING:
c7ffd872b92c08 Richard Cochran 2022-01-03  669  		ret = sprintf(buf, "%s\n", "mac");
33070417b78b38 Richard Cochran 2022-01-03  670  		break;
33070417b78b38 Richard Cochran 2022-01-03  671  	case PHY_TIMESTAMPING:
33070417b78b38 Richard Cochran 2022-01-03  672  		ret = sprintf(buf, "%s\n", "phy");
33070417b78b38 Richard Cochran 2022-01-03  673  		break;
c7ffd872b92c08 Richard Cochran 2022-01-03  674  	}
c7ffd872b92c08 Richard Cochran 2022-01-03  675  
c7ffd872b92c08 Richard Cochran 2022-01-03  676  	rtnl_unlock();
c7ffd872b92c08 Richard Cochran 2022-01-03  677  
c7ffd872b92c08 Richard Cochran 2022-01-03  678  	return ret;
c7ffd872b92c08 Richard Cochran 2022-01-03  679  }
33070417b78b38 Richard Cochran 2022-01-03  680  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org