[PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Richard Cochran]

Commit a6111d3c "vlan: Pass SIOC[SG]HWTSTAMP ioctls to real device"
intended to enable hardware time stamping on VLAN interfaces, but
passing SIOCSHWTSTAMP is only half of the story. This patch adds
the second half, by letting user space find out the time stamping
capabilities of the device backing a VLAN interface.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 net/8021q/vlan_dev.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 0d441ec..960aeb8 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
+#include <linux/net_tstamp.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <net/arp.h>
@@ -669,6 +670,23 @@ static void vlan_ethtool_get_drvinfo(struct net_device *dev,
 	strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
 }
 
+static int vlan_ethtool_get_ts_info(struct net_device *dev,
+				    struct ethtool_ts_info *info)
+{
+	const struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
+	const struct ethtool_ops *ops = vlan->real_dev->ethtool_ops;
+
+	if (ops->get_ts_info) {
+		return ops->get_ts_info(vlan->real_dev, info);
+	} else {
+		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
+			SOF_TIMESTAMPING_SOFTWARE;
+		info->phc_index = -1;
+	}
+
+	return 0;
+}
+
 static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 {
 	struct vlan_pcpu_stats *p;
@@ -752,6 +770,7 @@ static const struct ethtool_ops vlan_ethtool_ops = {
 	.get_settings	        = vlan_ethtool_get_settings,
 	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
+	.get_ts_info		= vlan_ethtool_get_ts_info,
 };
 
 static const struct net_device_ops vlan_netdev_ops = {
-- 
1.7.10.4

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Fri, 21 Nov 2014 14:16:20 +0100

> Commit a6111d3c "vlan: Pass SIOC[SG]HWTSTAMP ioctls to real device"
> intended to enable hardware time stamping on VLAN interfaces, but
> passing SIOCSHWTSTAMP is only half of the story. This patch adds
> the second half, by letting user space find out the time stamping
> capabilities of the device backing a VLAN interface.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>

Applied, thanks Richard.

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Ben Hutchings]

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

On Fri, 2014-11-21 at 14:16 +0100, Richard Cochran wrote:
> Commit a6111d3c "vlan: Pass SIOC[SG]HWTSTAMP ioctls to real device"
> intended to enable hardware time stamping on VLAN interfaces, but
> passing SIOCSHWTSTAMP is only half of the story. This patch adds
> the second half, by letting user space find out the time stamping
> capabilities of the device backing a VLAN interface.
[...]

This assumes that the same PTP capabilities apply to VLAN-tagged frames.
I don't think it's at all safe to assume that RX filter modes other than
HWTSTAMP_FILTER_ALL will include VLAN-tagged frames.  I think it is
necessary to define additional modes that explicitly include VLAN-tagged
frames.

I also disagree in general that reconfiguring a VLAN device should make
changes to the underlying device that affect more than just that VLAN,
i.e. SIOCSHWTSTAMP should not be passed through.  SIOCGHWTSTAMP could be
passed through, but rx_filter would need adjustment (VLAN-tagged modes
on the underlying devices become untagged modes on the VLAN device).

Ben.

-- 
Ben Hutchings
Never put off till tomorrow what you can avoid all together.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Richard Cochran]

On Sun, Nov 23, 2014 at 03:15:12AM +0000, Ben Hutchings wrote:
> This assumes that the same PTP capabilities apply to VLAN-tagged frames.
> I don't think it's at all safe to assume that RX filter modes other than
> HWTSTAMP_FILTER_ALL will include VLAN-tagged frames.  I think it is
> necessary to define additional modes that explicitly include VLAN-tagged
> frames.

Unsafe? How?

Do you mean that some HW cannot identify and time stamp PTP frames
when VLAN tagged? That is surely disappointing for people who shell
out money for such cards, but it is hardly unsafe.

> I also disagree in general that reconfiguring a VLAN device should make
> changes to the underlying device that affect more than just that VLAN,
> i.e. SIOCSHWTSTAMP should not be passed through.  SIOCGHWTSTAMP could be
> passed through, but rx_filter would need adjustment (VLAN-tagged modes
> on the underlying devices become untagged modes on the VLAN device).

The whole filter list with every last combination (at least, the ones
at the time) came directly from a early, limited HW design.  Sane,
modern PTP hardware provides time stamps regardless of whether a frame
is VLAN tagged or not. I don't see any reason not to make our stack
even more ugly just to cater to broken hardware.

I have nothing against adding VLAN to the SIOCGHWTSTAMP list, because
the hardware people *really* use all have:

	HWTSTAMP_TX_ON, and
	HWTSTAMP_FILTER_ALL, or 
	HWTSTAMP_FILTER_PTP_V2_EVENT,

So adding more won't hurt. (But it won't help either. If your HW
cannot time stamp Layer2 and you transmit Layer2, you simply never get
a time stamp.)

But please don't hold up progress just for this sort of thing.

Thanks,
Richard

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Ben Hutchings]

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

On Sun, 2014-11-23 at 17:05 +0100, Richard Cochran wrote:
> On Sun, Nov 23, 2014 at 03:15:12AM +0000, Ben Hutchings wrote:
> > This assumes that the same PTP capabilities apply to VLAN-tagged frames.
> > I don't think it's at all safe to assume that RX filter modes other than
> > HWTSTAMP_FILTER_ALL will include VLAN-tagged frames.  I think it is
> > necessary to define additional modes that explicitly include VLAN-tagged
> > frames.
> 
> Unsafe? How?

I mean that the assumption is wrong in general.  So these changes will
result in silent failure to enable RX timestamps, on some devices.

> Do you mean that some HW cannot identify and time stamp PTP frames
> when VLAN tagged? That is surely disappointing for people who shell
> out money for such cards, but it is hardly unsafe.
>
> > I also disagree in general that reconfiguring a VLAN device should make
> > changes to the underlying device that affect more than just that VLAN,
> > i.e. SIOCSHWTSTAMP should not be passed through.  SIOCGHWTSTAMP could be
> > passed through, but rx_filter would need adjustment (VLAN-tagged modes
> > on the underlying devices become untagged modes on the VLAN device).
> 
> The whole filter list with every last combination (at least, the ones
> at the time) came directly from a early, limited HW design.  Sane,
> modern PTP hardware provides time stamps regardless of whether a frame
> is VLAN tagged or not.

I would hope so.  However, this is non-standard - IEEE802.1AS
*explicitly forbids* the use of VLAN tags.  Linux's software PTP
classifier also doesn't yet handle 802.1ad VLAN tags.

> I don't see any reason not to make our stack even more ugly just to cater
> to broken hardware.

So failure to implement a non-standard extension is now 'broken'?

> I have nothing against adding VLAN to the SIOCGHWTSTAMP list, because
> the hardware people *really* use all have:
> 
> 	HWTSTAMP_TX_ON, and
> 	HWTSTAMP_FILTER_ALL, or 
> 	HWTSTAMP_FILTER_PTP_V2_EVENT,
> 
> So adding more won't hurt. (But it won't help either. If your HW
> cannot time stamp Layer2 and you transmit Layer2, you simply never get
> a time stamp.)
>
> But please don't hold up progress just for this sort of thing.

This is progress for some devices, false advertising for others.

Ben.

-- 
Ben Hutchings
Never put off till tomorrow what you can avoid all together.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Richard Cochran]

On Sun, Nov 23, 2014 at 05:19:59PM +0000, Ben Hutchings wrote:
> > Unsafe? How?
> 
> I mean that the assumption is wrong in general.  So these changes will
> result in silent failure to enable RX timestamps, on some devices.

Silent failure is dissapointing, yes, but not unsafe. Nothing breaks.
 
> > The whole filter list with every last combination (at least, the ones
> > at the time) came directly from a early, limited HW design.  Sane,
> > modern PTP hardware provides time stamps regardless of whether a frame
> > is VLAN tagged or not.
> 
> I would hope so.  However, this is non-standard - IEEE802.1AS
> *explicitly forbids* the use of VLAN tags.

Yes, but still people are using them. People fail to understand that
VLAN tags are not meant for end stations. The power industry has
created IEEE 61850 which actually requires end stations to send and
receive VLAN tagged frames with VID!=0, in violation of 802.1. Then
they published a PTP profile running on VLAN tagged Layer2.

(For this reason alone, most hardware does handle tagged frames, and
that is also why the linuxptp stack accepts vlan tags when using Layer2
transport.)

Setting up vlan interfaces on an end station is already somewhat
fishy. People who do that can surely take care that their cards do
correctly handle vlan tagged frames.

Concerning this patch, I needed get_ts_info to work on a VLAN interface
in order to implement a grand master clock on a managed switch. I think
that is valid use case, and implementing a Linux based Transparent
Clock will also need this.

>  Linux's software PTP
> classifier also doesn't yet handle 802.1ad VLAN tags.

As soon as someone wants this, I don't see why it cannot be added.

> > I don't see any reason not to make our stack even more ugly just to cater
> > to broken hardware.
> 
> So failure to implement a non-standard extension is now 'broken'?

If your PTP traffic flows through a managed, transparent switch with
VLANs enabled, then the device will have to provide time stamps. This
is really a standard use case. Also the power profile mandates vlan.
 
> This is progress for some devices, false advertising for others.

And where do we draw the line with describing the limitations of poorly
made hardware classifiers? What if a card can time stamp IPv4 packets,
but not in the presence of IPv4 options?

Again, I don't mind if someone wants to add a bunch more combinations
to the rx_filters. But they are useless in the end. In practice, user
space software will ignore them anyhow, unless the software somehow
caters to hardware limitations. Really, look at the list in rx_filters,
and think about this. What is the stack supposed to do if SIOCGHWTSTAMP
returns HWTSTAMP_FILTER_PTP_V2_DELAY_REQ? It is madness, really.

Thanks,
Richard

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Sun, 23 Nov 2014 17:05:35 +0100

> On Sun, Nov 23, 2014 at 03:15:12AM +0000, Ben Hutchings wrote:
>> This assumes that the same PTP capabilities apply to VLAN-tagged frames.
>> I don't think it's at all safe to assume that RX filter modes other than
>> HWTSTAMP_FILTER_ALL will include VLAN-tagged frames.  I think it is
>> necessary to define additional modes that explicitly include VLAN-tagged
>> frames.
> 
> Unsafe? How?
> 
> Do you mean that some HW cannot identify and time stamp PTP frames
> when VLAN tagged? That is surely disappointing for people who shell
> out money for such cards, but it is hardly unsafe.

Ben, you will have to provide a concrete example of a chip that will
do the wrong thing with Richard's change.

Hypotheticals won't cut it.

Thanks.

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Ben Hutchings]

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

On Sun, 2014-11-23 at 13:48 -0500, David Miller wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> Date: Sun, 23 Nov 2014 17:05:35 +0100
> 
> > On Sun, Nov 23, 2014 at 03:15:12AM +0000, Ben Hutchings wrote:
> >> This assumes that the same PTP capabilities apply to VLAN-tagged frames.
> >> I don't think it's at all safe to assume that RX filter modes other than
> >> HWTSTAMP_FILTER_ALL will include VLAN-tagged frames.  I think it is
> >> necessary to define additional modes that explicitly include VLAN-tagged
> >> frames.
> > 
> > Unsafe? How?
> > 
> > Do you mean that some HW cannot identify and time stamp PTP frames
> > when VLAN tagged? That is surely disappointing for people who shell
> > out money for such cards, but it is hardly unsafe.
> 
> Ben, you will have to provide a concrete example of a chip that will
> do the wrong thing with Richard's change.
> 
> Hypotheticals won't cut it.

The two datasheets I've looked at so far (EG20T for pch_gbe driver,
BF518 for bfin_mac driver) mention support for 802.1q but not 802.1ad.
I already mentioned ptp_classify.c doesn't support 802.1ad, and I can't
see how it would ever make sense to run PTP directly over 802.1ad
anyway.

I'll read some more datasheets to see whether 802.1q is generally
supported, but for 802.1ad VLAN devices I think we should pass-through
only the NONE and ALL filter capabilities.

Ben.

-- 
Ben Hutchings
Never put off till tomorrow what you can avoid all together.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net-next] vlan: Pass ethtool get_ts_info queries to real device.

[Richard Cochran]

On Sun, Nov 23, 2014 at 07:04:57PM +0000, Ben Hutchings wrote:
> The two datasheets I've looked at so far (EG20T for pch_gbe driver,
> BF518 for bfin_mac driver) mention support for 802.1q but not 802.1ad.
> I already mentioned ptp_classify.c doesn't support 802.1ad, and I can't
> see how it would ever make sense to run PTP directly over 802.1ad
> anyway.

Me, neither.
 
> I'll read some more datasheets to see whether 802.1q is generally
> supported, but for 802.1ad VLAN devices I think we should pass-through
> only the NONE and ALL filter capabilities.

Fine with me.

Thanks,
Richard