[PATCH net-next 0/4] Fix broken PTP over IP on Ocelot switches

Po Liu reported recently that timestamping PTP over IPv4 is broken using
the felix driver on NXP LS1028A. This has been known for a while, of
course, since it has always been broken. The reason is because IP PTP
packets are currently treated as unknown IP multicast, which is not
flooded to the CPU port in the ocelot driver design, so packets don't
reach the ptp4l program.

The series solves the problem by installing packet traps per port when
the timestamping ioctl is called, depending on the RX filter selected
(L2, L4 or both).

Vladimir Oltean (4):
  net: mscc: ocelot: don't downgrade timestamping RX filters in
    SIOCSHWTSTAMP
  net: mscc: ocelot: create a function that replaces an existing VCAP
    filter
  net: ptp: add a definition for the UDP port for IEEE 1588 general
    messages
  net: mscc: ocelot: set up traps for PTP packets

 drivers/net/ethernet/mscc/ocelot.c      | 247 +++++++++++++++++++++++-
 drivers/net/ethernet/mscc/ocelot_vcap.c |  16 ++
 include/linux/ptp_classify.h            |   1 +
 include/soc/mscc/ocelot_vcap.h          |   2 +
 4 files changed, 259 insertions(+), 7 deletions(-)

-- 
2.25.1

The ocelot driver, when asked to timestamp all receiving packets, 1588
v1 or NTP, says "nah, here's 1588 v2 for you".

According to this discussion:
https://patchwork.kernel.org/project/netdevbpf/patch/20211104133204.19757-8-martin.kaistra@linutronix.de/#24577647
drivers that downgrade from a wider request to a narrower response (or
even a response where the intersection with the request is empty) are
buggy, and should return -ERANGE instead. This patch fixes that.

Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Suggested-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 95920668feb0..f5490533c884 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c

@@ -1389,12 +1389,6 @@ int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr)
 	switch (cfg.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		break;
-	case HWTSTAMP_FILTER_ALL:
-	case HWTSTAMP_FILTER_SOME:
-	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
-	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
-	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
-	case HWTSTAMP_FILTER_NTP_ALL:
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:

-- 
2.25.1

VCAP (Versatile Content Aware Processor) is the TCAM-based engine behind
tc flower offload on ocelot, among other things. The ingress port mask
on which VCAP rules match is present as a bit field in the actual key of
the rule. This means that it is possible for a rule to be shared among
multiple source ports. When the rule is added one by one on each desired
port, that the ingress port mask of the key must be edited and rewritten
to hardware.

But the API in ocelot_vcap.c does not allow for this. For one thing,
ocelot_vcap_filter_add() and ocelot_vcap_filter_del() are not symmetric,
because ocelot_vcap_filter_add() works with a preallocated and
prepopulated filter and programs it to hardware, and
ocelot_vcap_filter_del() does both the job of removing the specified
filter from hardware, as well as kfreeing it. That is to say, the only
option of editing a filter in place, which is to delete it, modify the
structure and add it back, does not work because it results in
use-after-free.

This patch introduces ocelot_vcap_filter_replace, which trivially
reprograms a VCAP entry to hardware, at the exact same index at which it
existed before, without modifying any list or allocating any memory.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_vcap.c | 16 ++++++++++++++++
 include/soc/mscc/ocelot_vcap.h          |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_vcap.c b/drivers/net/ethernet/mscc/ocelot_vcap.c
index 18ab0fd303c8..d3544413a8a4 100644
--- a/drivers/net/ethernet/mscc/ocelot_vcap.c
+++ b/drivers/net/ethernet/mscc/ocelot_vcap.c

@@ -1246,6 +1246,22 @@ int ocelot_vcap_filter_del(struct ocelot *ocelot,
 }
 EXPORT_SYMBOL(ocelot_vcap_filter_del);
 
+int ocelot_vcap_filter_replace(struct ocelot *ocelot,
+			       struct ocelot_vcap_filter *filter)
+{
+	struct ocelot_vcap_block *block = &ocelot->block[filter->block_id];
+	int index;
+
+	index = ocelot_vcap_block_get_filter_index(block, filter);
+	if (index < 0)
+		return index;
+
+	vcap_entry_set(ocelot, index, filter);
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_vcap_filter_replace);
+
 int ocelot_vcap_filter_stats_update(struct ocelot *ocelot,
 				    struct ocelot_vcap_filter *filter)
 {

diff --git a/include/soc/mscc/ocelot_vcap.h b/include/soc/mscc/ocelot_vcap.h
index 9cca2f8e61a2..709cbc198fd2 100644
--- a/include/soc/mscc/ocelot_vcap.h
+++ b/include/soc/mscc/ocelot_vcap.h

@@ -704,6 +704,8 @@ int ocelot_vcap_filter_add(struct ocelot *ocelot,
 			   struct netlink_ext_ack *extack);
 int ocelot_vcap_filter_del(struct ocelot *ocelot,
 			   struct ocelot_vcap_filter *rule);
+int ocelot_vcap_filter_replace(struct ocelot *ocelot,
+			       struct ocelot_vcap_filter *filter);
 struct ocelot_vcap_filter *
 ocelot_vcap_block_find_filter_by_id(struct ocelot_vcap_block *block,
 				    unsigned long cookie, bool tc_offload);

-- 
2.25.1

As opposed to event messages (Sync, PdelayReq etc) which require
timestamping, general messages (Announce, FollowUp etc) do not.
In PTP they are part of different streams of data.

IEEE 1588-2008 Annex D.2 "UDP port numbers" states that the UDP
destination port assigned by IANA is 319 for event messages, and 320 for
general messages. Yet the kernel seems to be missing the definition for
general messages. This patch adds it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/ptp_classify.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index ae04968a3a47..9afd34a2d36c 100644
--- a/include/linux/ptp_classify.h
+++ b/include/linux/ptp_classify.h

@@ -37,6 +37,7 @@
 #define PTP_MSGTYPE_PDELAY_RESP 0x3
 
 #define PTP_EV_PORT 319
+#define PTP_GEN_PORT 320
 #define PTP_GEN_BIT 0x08 /* indicates general message, if set in message type */
 
 #define OFF_PTP_SOURCE_UUID	22 /* PTPv1 only */

-- 
2.25.1

IEEE 1588 support was declared too soon for the Ocelot switch. Out of
reset, this switch does not apply any special treatment for PTP packets,
i.e. when an event message is received, the natural tendency is to
forward it by MAC DA/VLAN ID. This poses a problem when the ingress port
is under a bridge, since user space application stacks (written
primarily for endpoint ports, not switches) like ptp4l expect that PTP
messages are always received on AF_PACKET / AF_INET sockets (depending
on the PTP transport being used), and never being autonomously
forwarded. Any forwarding, if necessary (for example in Transparent
Clock mode) is handled in software by ptp4l. Having the hardware forward
these packets too will cause duplicates which will confuse endpoints
connected to these switches.

So PTP over L2 barely works, in the sense that PTP packets reach the CPU
port, but they reach it via flooding, and therefore reach lots of other
unwanted destinations too. But PTP over IPv4/IPv6 does not work at all.
This is because the Ocelot switch have a separate destination port mask
for unknown IP multicast (which PTP over IP is) flooding compared to
unknown non-IP multicast (which PTP over L2 is) flooding. Specifically,
the driver allows the CPU port to be in the PGID_MC port group, but not
in PGID_MCIPV4 and PGID_MCIPV6. There are several presentations from
Allan Nielsen which explain that the embedded MIPS CPU on Ocelot
switches is not very powerful at all, so every penny they could save by
not allowing flooding to the CPU port module matters. Unknown IP
multicast did not make it.

The de facto consensus is that when a switch is PTP-aware and an
application stack for PTP is running, switches should have some sort of
trapping mechanism for PTP packets, to extract them from the hardware
data path. This avoids both problems:
(a) PTP packets are no longer flooded to unwanted destinations
(b) PTP over IP packets are no longer denied from reaching the CPU since
    they arrive there via a trap and not via flooding

It is not the first time when this change is attempted. Last time, the
feedback from Allan Nielsen and Andrew Lunn was that the traps should
not be installed by default, and that PTP-unaware switching may be
desired for some use cases:
https://patchwork.ozlabs.org/project/netdev/patch/20190813025214.18601-5-yangbo.lu@nxp.com/

To address that feedback, the present patch adds the necessary packet
traps according to the RX filter configuration transmitted by user space
through the SIOCSHWTSTAMP ioctl. Trapping is done via VCAP IS2, where we
keep 5 filters, which are amended each time RX timestamping is enabled
or disabled on a port:
- 1 for PTP over L2
- 2 for PTP over IPv4 (UDP ports 319 and 320)
- 2 for PTP over IPv6 (UDP ports 319 and 320)

The cookie by which these filters (invisible to tc) are identified is
strategically chosen such that it does not collide with the filters used
for the ocelot-8021q tagging protocol by the Felix driver, or with the
MRP traps set up by the Ocelot library.

Other alternatives were considered, like patching user space to do
something, but there are so many ways in which PTP packets could be made
to reach the CPU, generically speaking, that "do what?" is a very valid
question. The ptp4l program from the linuxptp stack already attempts to
do something: it calls setsockopt(IP_ADD_MEMBERSHIP) (and
PACKET_ADD_MEMBERSHIP, respectively) which translates in both cases into
a dev_mc_add() on the interface, in the kernel:
https://github.com/richardcochran/linuxptp/blob/v3.1.1/udp.c#L73
https://github.com/richardcochran/linuxptp/blob/v3.1.1/raw.c

Reality shows that this is not sufficient in case the interface belongs
to a switchdev driver, as dev_mc_add() does not show the intention to
trap a packet to the CPU, but rather the intention to not drop it (it is
strictly for RX filtering, same as promiscuous does not mean to send all
traffic to the CPU, but to not drop traffic with unknown MAC DA). This
topic is a can of worms in itself, and it would be great if user space
could just stay out of it.

On the other hand, setting up PTP traps privately within the driver is
not new by any stretch of the imagination:
https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c#L833
https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/net/dsa/hirschmann/hellcreek.c#L1050
https://elixir.bootlin.com/linux/v5.16-rc2/source/include/linux/dsa/sja1105.h#L21

So this is the approach taken here as well. The difference here being
that we prepare and destroy the traps per port, dynamically at runtime,
as opposed to driver init time, because apparently, PTP-unaware
forwarding is a use case.

Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Reported-by: Po Liu <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 241 ++++++++++++++++++++++++++++-
 1 file changed, 240 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index f5490533c884..456aeba2b245 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c

@@ -1347,6 +1347,225 @@ int ocelot_fdb_dump(struct ocelot *ocelot, int port,
 }
 EXPORT_SYMBOL(ocelot_fdb_dump);
 
+static void ocelot_populate_l2_ptp_trap_key(struct ocelot_vcap_filter *trap)
+{
+	trap->key_type = OCELOT_VCAP_KEY_ETYPE;
+	*(__be16 *)trap->key.etype.etype.value = htons(ETH_P_1588);
+	*(__be16 *)trap->key.etype.etype.mask = htons(0xffff);
+}
+
+static void
+ocelot_populate_ipv4_ptp_event_trap_key(struct ocelot_vcap_filter *trap)
+{
+	trap->key_type = OCELOT_VCAP_KEY_IPV4;
+	trap->key.ipv4.dport.value = PTP_EV_PORT;
+	trap->key.ipv4.dport.mask = 0xffff;
+}
+
+static void
+ocelot_populate_ipv6_ptp_event_trap_key(struct ocelot_vcap_filter *trap)
+{
+	trap->key_type = OCELOT_VCAP_KEY_IPV6;
+	trap->key.ipv6.dport.value = PTP_EV_PORT;
+	trap->key.ipv6.dport.mask = 0xffff;
+}
+
+static void
+ocelot_populate_ipv4_ptp_general_trap_key(struct ocelot_vcap_filter *trap)
+{
+	trap->key_type = OCELOT_VCAP_KEY_IPV4;
+	trap->key.ipv4.dport.value = PTP_GEN_PORT;
+	trap->key.ipv4.dport.mask = 0xffff;
+}
+
+static void
+ocelot_populate_ipv6_ptp_general_trap_key(struct ocelot_vcap_filter *trap)
+{
+	trap->key_type = OCELOT_VCAP_KEY_IPV6;
+	trap->key.ipv6.dport.value = PTP_GEN_PORT;
+	trap->key.ipv6.dport.mask = 0xffff;
+}
+
+static int ocelot_trap_add(struct ocelot *ocelot, int port,
+			   unsigned long cookie,
+			   void (*populate)(struct ocelot_vcap_filter *f))
+{
+	struct ocelot_vcap_block *block_vcap_is2;
+	struct ocelot_vcap_filter *trap;
+	bool new = false;
+	int err;
+
+	block_vcap_is2 = &ocelot->block[VCAP_IS2];
+
+	trap = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, cookie,
+						   false);
+	if (!trap) {
+		trap = kzalloc(sizeof(*trap), GFP_KERNEL);
+		if (!trap)
+			return -ENOMEM;
+
+		populate(trap);
+		trap->prio = 1;
+		trap->id.cookie = cookie;
+		trap->id.tc_offload = false;
+		trap->block_id = VCAP_IS2;
+		trap->type = OCELOT_VCAP_FILTER_OFFLOAD;
+		trap->lookup = 0;
+		trap->action.cpu_copy_ena = true;
+		trap->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY;
+		trap->action.port_mask = 0;
+		new = true;
+	}
+
+	trap->ingress_port_mask |= BIT(port);
+
+	if (new)
+		err = ocelot_vcap_filter_add(ocelot, trap, NULL);
+	else
+		err = ocelot_vcap_filter_replace(ocelot, trap);
+	if (err) {
+		trap->ingress_port_mask &= ~BIT(port);
+		if (!trap->ingress_port_mask)
+			kfree(trap);
+		return err;
+	}
+
+	return 0;
+}
+
+static int ocelot_trap_del(struct ocelot *ocelot, int port,
+			   unsigned long cookie)
+{
+	struct ocelot_vcap_block *block_vcap_is2;
+	struct ocelot_vcap_filter *trap;
+
+	block_vcap_is2 = &ocelot->block[VCAP_IS2];
+
+	trap = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, cookie,
+						   false);
+	if (!trap)
+		return 0;
+
+	trap->ingress_port_mask &= ~BIT(port);
+	if (!trap->ingress_port_mask)
+		return ocelot_vcap_filter_del(ocelot, trap);
+
+	return ocelot_vcap_filter_replace(ocelot, trap);
+}
+
+static int ocelot_l2_ptp_trap_add(struct ocelot *ocelot, int port)
+{
+	unsigned long l2_cookie = ocelot->num_phys_ports + 1;
+
+	return ocelot_trap_add(ocelot, port, l2_cookie,
+			       ocelot_populate_l2_ptp_trap_key);
+}
+
+static int ocelot_l2_ptp_trap_del(struct ocelot *ocelot, int port)
+{
+	unsigned long l2_cookie = ocelot->num_phys_ports + 1;
+
+	return ocelot_trap_del(ocelot, port, l2_cookie);
+}
+
+static int ocelot_ipv4_ptp_trap_add(struct ocelot *ocelot, int port)
+{
+	unsigned long ipv4_gen_cookie = ocelot->num_phys_ports + 2;
+	unsigned long ipv4_ev_cookie = ocelot->num_phys_ports + 3;
+	int err;
+
+	err = ocelot_trap_add(ocelot, port, ipv4_ev_cookie,
+			      ocelot_populate_ipv4_ptp_event_trap_key);
+	if (err)
+		return err;
+
+	err = ocelot_trap_add(ocelot, port, ipv4_gen_cookie,
+			      ocelot_populate_ipv4_ptp_general_trap_key);
+	if (err)
+		ocelot_trap_del(ocelot, port, ipv4_ev_cookie);
+
+	return err;
+}
+
+static int ocelot_ipv4_ptp_trap_del(struct ocelot *ocelot, int port)
+{
+	unsigned long ipv4_gen_cookie = ocelot->num_phys_ports + 2;
+	unsigned long ipv4_ev_cookie = ocelot->num_phys_ports + 3;
+	int err;
+
+	err = ocelot_trap_del(ocelot, port, ipv4_ev_cookie);
+	err |= ocelot_trap_del(ocelot, port, ipv4_gen_cookie);
+	return err;
+}
+
+static int ocelot_ipv6_ptp_trap_add(struct ocelot *ocelot, int port)
+{
+	unsigned long ipv6_gen_cookie = ocelot->num_phys_ports + 4;
+	unsigned long ipv6_ev_cookie = ocelot->num_phys_ports + 5;
+	int err;
+
+	err = ocelot_trap_add(ocelot, port, ipv6_ev_cookie,
+			      ocelot_populate_ipv6_ptp_event_trap_key);
+	if (err)
+		return err;
+
+	err = ocelot_trap_add(ocelot, port, ipv6_gen_cookie,
+			      ocelot_populate_ipv6_ptp_general_trap_key);
+	if (err)
+		ocelot_trap_del(ocelot, port, ipv6_ev_cookie);
+
+	return err;
+}
+
+static int ocelot_ipv6_ptp_trap_del(struct ocelot *ocelot, int port)
+{
+	unsigned long ipv6_gen_cookie = ocelot->num_phys_ports + 4;
+	unsigned long ipv6_ev_cookie = ocelot->num_phys_ports + 5;
+	int err;
+
+	err = ocelot_trap_del(ocelot, port, ipv6_ev_cookie);
+	err |= ocelot_trap_del(ocelot, port, ipv6_gen_cookie);
+	return err;
+}
+
+static int ocelot_setup_ptp_traps(struct ocelot *ocelot, int port,
+				  bool l2, bool l4)
+{
+	int err;
+
+	if (l2)
+		err = ocelot_l2_ptp_trap_add(ocelot, port);
+	else
+		err = ocelot_l2_ptp_trap_del(ocelot, port);
+	if (err)
+		return err;
+
+	if (l4) {
+		err = ocelot_ipv4_ptp_trap_add(ocelot, port);
+		if (err)
+			goto err_ipv4;
+
+		err = ocelot_ipv6_ptp_trap_add(ocelot, port);
+		if (err)
+			goto err_ipv6;
+	} else {
+		err = ocelot_ipv4_ptp_trap_del(ocelot, port);
+
+		err |= ocelot_ipv6_ptp_trap_del(ocelot, port);
+	}
+	if (err)
+		return err;
+
+	return 0;
+
+err_ipv6:
+	ocelot_ipv4_ptp_trap_del(ocelot, port);
+err_ipv4:
+	if (l2)
+		ocelot_l2_ptp_trap_del(ocelot, port);
+	return err;
+}
+
 int ocelot_hwstamp_get(struct ocelot *ocelot, int port, struct ifreq *ifr)
 {
 	return copy_to_user(ifr->ifr_data, &ocelot->hwtstamp_config,

@@ -1357,7 +1576,9 @@ EXPORT_SYMBOL(ocelot_hwstamp_get);
 int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	bool l2 = false, l4 = false;
 	struct hwtstamp_config cfg;
+	int err;
 
 	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
 		return -EFAULT;

@@ -1392,19 +1613,37 @@ int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		l4 = true;
+		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		l2 = true;
+		break;
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		l2 = true;
+		l4 = true;
 		break;
 	default:
 		mutex_unlock(&ocelot->ptp_lock);
 		return -ERANGE;
 	}
 
+	err = ocelot_setup_ptp_traps(ocelot, port, l2, l4);
+	if (err)
+		return err;
+
+	if (l2 && l4)
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+	else if (l2)
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+	else if (l4)
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+	else
+		cfg.rx_filter = HWTSTAMP_FILTER_NONE;
+
 	/* Commit back the result & save it */
 	memcpy(&ocelot->hwtstamp_config, &cfg, sizeof(cfg));
 	mutex_unlock(&ocelot->ptp_lock);

-- 
2.25.1

On Fri, Nov 26, 2021 at 01:21:14AM +0200, Vladimir Oltean wrote:

Po Liu reported recently that timestamping PTP over IPv4 is broken using
the felix driver on NXP LS1028A. This has been known for a while, of
course, since it has always been broken. The reason is because IP PTP
packets are currently treated as unknown IP multicast, which is not
flooded to the CPU port in the ocelot driver design, so packets don't
reach the ptp4l program.

The series solves the problem by installing packet traps per port when
the timestamping ioctl is called, depending on the RX filter selected
(L2, L4 or both).

Vladimir Oltean (4):
  net: mscc: ocelot: don't downgrade timestamping RX filters in
    SIOCSHWTSTAMP
  net: mscc: ocelot: create a function that replaces an existing VCAP
    filter
  net: ptp: add a definition for the UDP port for IEEE 1588 general
    messages
  net: mscc: ocelot: set up traps for PTP packets

 drivers/net/ethernet/mscc/ocelot.c      | 247 +++++++++++++++++++++++-
 drivers/net/ethernet/mscc/ocelot_vcap.c |  16 ++
 include/linux/ptp_classify.h            |   1 +
 include/soc/mscc/ocelot_vcap.h          |   2 +
 4 files changed, 259 insertions(+), 7 deletions(-)

-- 
2.25.1

I don't know why I targeted these patches to "net-next". Habit I guess.
Nonetheless, they apply equally well to "net", can they be considered
for merging there without me resending?

On Thu, 25 Nov 2021 23:45:21 +0000 Vladimir Oltean wrote:

On Fri, Nov 26, 2021 at 01:21:14AM +0200, Vladimir Oltean wrote:

quoted

Po Liu reported recently that timestamping PTP over IPv4 is broken using
the felix driver on NXP LS1028A. This has been known for a while, of
course, since it has always been broken. The reason is because IP PTP
packets are currently treated as unknown IP multicast, which is not
flooded to the CPU port in the ocelot driver design, so packets don't
reach the ptp4l program.

The series solves the problem by installing packet traps per port when
the timestamping ioctl is called, depending on the RX filter selected
(L2, L4 or both).

I don't know why I targeted these patches to "net-next". Habit I guess.
Nonetheless, they apply equally well to "net", can they be considered
for merging there without me resending?

Only patch 1 looks like a fix, tho? Patch 4 seems to fall into 
the "this never worked and doesn't cause a crash" category.

I'm hoping to send a PR tomorrow, so if you resend quickly it 
will be in net-next soon.

On Thu, Nov 25, 2021 at 07:01:01PM -0800, Jakub Kicinski wrote:

On Thu, 25 Nov 2021 23:45:21 +0000 Vladimir Oltean wrote:

quoted

On Fri, Nov 26, 2021 at 01:21:14AM +0200, Vladimir Oltean wrote:

quoted

Po Liu reported recently that timestamping PTP over IPv4 is broken using
the felix driver on NXP LS1028A. This has been known for a while, of
course, since it has always been broken. The reason is because IP PTP
packets are currently treated as unknown IP multicast, which is not
flooded to the CPU port in the ocelot driver design, so packets don't
reach the ptp4l program.

The series solves the problem by installing packet traps per port when
the timestamping ioctl is called, depending on the RX filter selected
(L2, L4 or both).

I don't know why I targeted these patches to "net-next". Habit I guess.
Nonetheless, they apply equally well to "net", can they be considered
for merging there without me resending?

Only patch 1 looks like a fix, tho? Patch 4 seems to fall into
the "this never worked and doesn't cause a crash" category.

I'm hoping to send a PR tomorrow, so if you resend quickly it
will be in net-next soon.

It's true that a lot of work went into ocelot_vcap.c in order to make it
safely usable for traps outside of the tc-flower offload, and I
understand that you need to draw the line somewhere. But on the other
hand, this is fixing very real problems that are bothering real users.
Patch 1, not so much, it popped up as a result of discussions and
looking at code. None of the bugs fixed here cause a crash, it's just
that things don't work as expected. Technically, a user could still set
up the appropriate traps via tc-flower and PTP would work, but they'd
have to know that they need to, in the first place. So I would still be
very appreciative if all 4 patches would be considered for inclusion
into "net". I'm not expecting them to be backported very far, of course,
but as long as they reach at least v5.15 I'm happy.

On Fri, Nov 26, 2021 at 08:58:47AM -0800, Richard Cochran wrote:

On Fri, Nov 26, 2021 at 01:21:18AM +0200, Vladimir Oltean wrote:

quoted

So PTP over L2 barely works, in the sense that PTP packets reach the CPU
port, but they reach it via flooding, and therefore reach lots of other
unwanted destinations too. But PTP over IPv4/IPv6 does not work at all.
This is because the Ocelot switch have ...

Not that the details are same, but I'd like to report that the Marvell
switches (or driver or kernel) also have issues with PTP over UDP/IPv4.

When configured with separate interfaces, not in a bridge, you can run
ptp4l as a UDP/IPv4 Boundary Clock, and it works fine.

When configured as a bridge, and running ptp4l as a UDP/IPv4
Transparent Clock, it doesn't work.  It has been a while, and I don't
have the HW any more, but I don't recall the exact behavior.  I think
the switch did not treat the Event frames as switch management frames.

(BTW, running ptp4l TC over Layer-2 worked just fine with the switch
configured as a bridge.)

Just saying, in case somebody with such a switch would like to try and
fix the driver by adding special forwarding rules for the IPv4/6
multicast addresses.

This, to me, sounds more like the bridge trapping the packets on br0
instead of letting them flow on the port netdevices, which is solved by
some netfilter rules? Or is it really a driver/hardware issue?

https://lore.kernel.org/netdev/20211116102138.26vkpeh23el6akya@skbuf/

On Fri, Nov 26, 2021 at 01:21:14AM +0200, Vladimir Oltean wrote:

Po Liu reported recently that timestamping PTP over IPv4 is broken using
the felix driver on NXP LS1028A. This has been known for a while, of
course, since it has always been broken. The reason is because IP PTP
packets are currently treated as unknown IP multicast, which is not
flooded to the CPU port in the ocelot driver design, so packets don't
reach the ptp4l program.

The series solves the problem by installing packet traps per port when
the timestamping ioctl is called, depending on the RX filter selected
(L2, L4 or both).

Vladimir Oltean (4):
  net: mscc: ocelot: don't downgrade timestamping RX filters in
    SIOCSHWTSTAMP
  net: mscc: ocelot: create a function that replaces an existing VCAP
    filter
  net: ptp: add a definition for the UDP port for IEEE 1588 general
    messages
  net: mscc: ocelot: set up traps for PTP packets

For the series:

Acked-by: Richard Cochran <richardcochran@gmail.com>

On Fri, Nov 26, 2021 at 09:08:01AM -0800, Richard Cochran wrote:

On Fri, Nov 26, 2021 at 05:01:13PM +0000, Vladimir Oltean wrote:

quoted

This, to me, sounds more like the bridge trapping the packets on br0
instead of letting them flow on the port netdevices, which is solved by
some netfilter rules? Or is it really a driver/hardware issue?

https://lore.kernel.org/netdev/20211116102138.26vkpeh23el6akya@skbuf/

Yeah, thanks for the link.  I had seen it, but alas it came too late
for me to try on actual working HW.  Maybe it fixes the issue.

If someone out there has a Marvell switch, please try it and let us
know...

On the NXP LS1028A, PTP over IP, under a bridge, works with netfilter rules.
I have a Marvell 6390 switch which supposedly supports timestamping, but
I never got that to work, not even L2, and never had the time to sit
down with it and figure out what's wrong. I think I saw a discussion
initiated by George McCollister saying the same thing.

On Fri, Nov 26, 2021 at 01:21:18AM +0200, Vladimir Oltean wrote:

So PTP over L2 barely works, in the sense that PTP packets reach the CPU
port, but they reach it via flooding, and therefore reach lots of other
unwanted destinations too. But PTP over IPv4/IPv6 does not work at all.
This is because the Ocelot switch have ...

Not that the details are same, but I'd like to report that the Marvell
switches (or driver or kernel) also have issues with PTP over UDP/IPv4.

When configured with separate interfaces, not in a bridge, you can run
ptp4l as a UDP/IPv4 Boundary Clock, and it works fine.

When configured as a bridge, and running ptp4l as a UDP/IPv4
Transparent Clock, it doesn't work.  It has been a while, and I don't
have the HW any more, but I don't recall the exact behavior.  I think
the switch did not treat the Event frames as switch management frames.

(BTW, running ptp4l TC over Layer-2 worked just fine with the switch
configured as a bridge.)

Just saying, in case somebody with such a switch would like to try and
fix the driver by adding special forwarding rules for the IPv4/6
multicast addresses.

Thanks,
Richard

On Fri, Nov 26, 2021 at 05:01:13PM +0000, Vladimir Oltean wrote:

This, to me, sounds more like the bridge trapping the packets on br0
instead of letting them flow on the port netdevices, which is solved by
some netfilter rules? Or is it really a driver/hardware issue?

https://lore.kernel.org/netdev/20211116102138.26vkpeh23el6akya@skbuf/

Yeah, thanks for the link.  I had seen it, but alas it came too late
for me to try on actual working HW.  Maybe it fixes the issue.

If someone out there has a Marvell switch, please try it and let us
know...

Thanks,
Richard

On Fri, 26 Nov 2021 09:55:00 +0000 Vladimir Oltean wrote:

On Thu, Nov 25, 2021 at 07:01:01PM -0800, Jakub Kicinski wrote:

quoted

On Thu, 25 Nov 2021 23:45:21 +0000 Vladimir Oltean wrote:  

quoted

I don't know why I targeted these patches to "net-next". Habit I guess.
Nonetheless, they apply equally well to "net", can they be considered
for merging there without me resending?  

Only patch 1 looks like a fix, tho? Patch 4 seems to fall into
the "this never worked and doesn't cause a crash" category.

I'm hoping to send a PR tomorrow, so if you resend quickly it
will be in net-next soon.  

It's true that a lot of work went into ocelot_vcap.c in order to make it
safely usable for traps outside of the tc-flower offload, and I
understand that you need to draw the line somewhere. But on the other
hand, this is fixing very real problems that are bothering real users.
Patch 1, not so much, it popped up as a result of discussions and
looking at code. None of the bugs fixed here cause a crash, it's just
that things don't work as expected. Technically, a user could still set
up the appropriate traps via tc-flower and PTP would work, but they'd
have to know that they need to, in the first place. So I would still be
very appreciative if all 4 patches would be considered for inclusion
into "net". I'm not expecting them to be backported very far, of course,
but as long as they reach at least v5.15 I'm happy.

Alright, but please expect more push back going forward. Linus was
pretty clear on what constitutes -rc material in the past, and we're
sending quite a lot of code in each week..

On Fri, Nov 26, 2021 at 10:35:07AM -0800, Jakub Kicinski wrote:

On Fri, 26 Nov 2021 09:55:00 +0000 Vladimir Oltean wrote:

quoted

On Thu, Nov 25, 2021 at 07:01:01PM -0800, Jakub Kicinski wrote:

quoted

On Thu, 25 Nov 2021 23:45:21 +0000 Vladimir Oltean wrote:

quoted

I don't know why I targeted these patches to "net-next". Habit I guess.
Nonetheless, they apply equally well to "net", can they be considered
for merging there without me resending?

Only patch 1 looks like a fix, tho? Patch 4 seems to fall into
the "this never worked and doesn't cause a crash" category.

I'm hoping to send a PR tomorrow, so if you resend quickly it
will be in net-next soon.

It's true that a lot of work went into ocelot_vcap.c in order to make it
safely usable for traps outside of the tc-flower offload, and I
understand that you need to draw the line somewhere. But on the other
hand, this is fixing very real problems that are bothering real users.
Patch 1, not so much, it popped up as a result of discussions and
looking at code. None of the bugs fixed here cause a crash, it's just
that things don't work as expected. Technically, a user could still set
up the appropriate traps via tc-flower and PTP would work, but they'd
have to know that they need to, in the first place. So I would still be
very appreciative if all 4 patches would be considered for inclusion
into "net". I'm not expecting them to be backported very far, of course,
but as long as they reach at least v5.15 I'm happy.

Alright, but please expect more push back going forward. Linus was
pretty clear on what constitutes -rc material in the past, and we're
sending quite a lot of code in each week..

Thanks, and please don't hesitate to push back.

If for any reason you're not comfortable including these in the "net"
pull request, I'm okay with that, but at least allow me to keep the
"Fixes:" tags on the patches (because they do address incomplete
functionality), and consider applying them to net-next. Then maybe the
AUTOSEL people will notice and pick them up :)

Anyway I've noticed that the linux-stable maintainers are much more
generous these days when it comes to backporting. For example, I shouted
a few months ago that a relatively large quantity of DSA refactoring
patches was brought into "stable" because of some other patch that
wouldn't apply 100% cleanly:
https://lore.kernel.org/lkml/20210316162236.vmvulf3wlmtowdvf@skbuf/
But in the meantime I got used to it and I'm a bit more relaxed about it now.

On Fri, 26 Nov 2021 19:38:07 +0000 Vladimir Oltean wrote:

On Fri, Nov 26, 2021 at 10:35:07AM -0800, Jakub Kicinski wrote:

quoted

Alright, but please expect more push back going forward. Linus was
pretty clear on what constitutes -rc material in the past, and we're
sending quite a lot of code in each week..  

Thanks, and please don't hesitate to push back.

If for any reason you're not comfortable including these in the "net"
pull request, I'm okay with that, but at least allow me to keep the
"Fixes:" tags on the patches (because they do address incomplete
functionality), and consider applying them to net-next. Then maybe the
AUTOSEL people will notice and pick them up :)

Yeah, but then we'll get the opposite complaint of "why is this in -next
if it's a fix".

Anyway I've noticed that the linux-stable maintainers are much more
generous these days when it comes to backporting. For example, I shouted
a few months ago that a relatively large quantity of DSA refactoring
patches was brought into "stable" because of some other patch that
wouldn't apply 100% cleanly:
https://lore.kernel.org/lkml/20210316162236.vmvulf3wlmtowdvf@skbuf/
But in the meantime I got used to it and I'm a bit more relaxed about it now.

Indeed, I'll admit, it feels like the practice of stable is at odds with
the expectations of -rc releases.

If you're confident the changes are good, that's fine by me. It's still
relatively early -rc days.

Hi Richard,

On Fri Nov 26 2021, Richard Cochran wrote:

On Fri, Nov 26, 2021 at 05:01:13PM +0000, Vladimir Oltean wrote:

quoted

This, to me, sounds more like the bridge trapping the packets on br0
instead of letting them flow on the port netdevices, which is solved by
some netfilter rules? Or is it really a driver/hardware issue?

https://lore.kernel.org/netdev/20211116102138.26vkpeh23el6akya@skbuf/

Yeah, thanks for the link.  I had seen it, but alas it came too late
for me to try on actual working HW.  Maybe it fixes the issue.

If someone out there has a Marvell switch, please try it and let us
know...

I did some testing on a Marvell Topaz switch (mv88e6341). The Boundary
Clock over UDPv4/UDPv6 using Vladimir's netfilter rules doesn't
work. Even though I've successfully tested these rules on TI CPSW
(switchdev variant) and hellcreek.

However, BC over Layer 2 transport is also not really working well, as
the switch forwards PTP packets (instead of trapping?). Furthermore,
ptp4l reports loosing of timestamps when running on multiple switch
ports.

Tested with Linux v5.16.

Thanks,
Kurt