[PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Kurt Kanzenbach]

A time aware switch should trap PTP traffic to the management CPU. User space
daemons such as ptp4l will process these messages to implement Boundary (or
Transparent) Clocks.

At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages
which leads to confusion when multiple end devices are connected to the
switch. Therefore, setup PTP traps. Leverage the already implemented policy
mechanism based on destination addresses. Configure the traps only if
timestamping is enabled so that non time aware use case is still functioning.

Introduce an additional PTP operation in case other devices need special
handling in regards to trapping as well.

Tested on Marvell Topaz (mv88e6341) switch with multiple end devices connected
like this:

|# DSA setup
|$ ip link set eth0 up
|$ ip link set lan0 up
|$ ip link set lan1 up
|$ ip link set lan2 up
|$ ip link add name br0 type bridge
|$ ip link set dev lan0 master br0
|$ ip link set dev lan1 master br0
|$ ip link set dev lan2 master br0
|$ ip link set lan0 up
|$ ip link set lan1 up
|$ ip link set lan2 up
|$ ip link set br0 up
|$ dhclient br0
|# Configure bridge routing
|$ ebtables --table broute --append BROUTING --protocol 0x88F7 --jump DROP
|# Start linuxptp
|$ ptp4l -H -2 -i lan0 -i lan1 -i lan2 --tx_timestamp_timeout=40 -m

Verified added policies with mv88e6xxx_dump.

Signed-off-by: Kurt Kanzenbach <kurt@kmk-computers.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c     | 12 +++---
 drivers/net/dsa/mv88e6xxx/chip.h     |  5 +++
 drivers/net/dsa/mv88e6xxx/hwtstamp.c |  7 ++++
 drivers/net/dsa/mv88e6xxx/ptp.c      | 59 ++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/ptp.h      |  2 +
 5 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7fadbf987b23..ab50ebd85f1f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1816,8 +1816,8 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 	return mv88e6xxx_g1_atu_loadpurge(chip, fid, &entry);
 }
 
-static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
-				  const struct mv88e6xxx_policy *policy)
+int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
+			   const struct mv88e6xxx_policy *policy)
 {
 	enum mv88e6xxx_policy_mapping mapping = policy->mapping;
 	enum mv88e6xxx_policy_action action = policy->action;
@@ -1835,10 +1835,12 @@ static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
 	case MV88E6XXX_POLICY_MAPPING_SA:
 		if (action == MV88E6XXX_POLICY_ACTION_NORMAL)
 			state = 0; /* Dissociate the port and address */
-		else if (action == MV88E6XXX_POLICY_ACTION_DISCARD &&
+		else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD ||
+			  action == MV88E6XXX_POLICY_ACTION_TRAP) &&
 			 is_multicast_ether_addr(addr))
 			state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_POLICY;
-		else if (action == MV88E6XXX_POLICY_ACTION_DISCARD &&
+		else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD ||
+			  action == MV88E6XXX_POLICY_ACTION_TRAP) &&
 			 is_unicast_ether_addr(addr))
 			state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC_POLICY;
 		else
@@ -4589,7 +4591,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.serdes_irq_status = mv88e6390_serdes_irq_status,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6390_avb_ops,
-	.ptp_ops = &mv88e6352_ptp_ops,
+	.ptp_ops = &mv88e6341_ptp_ops,
 	.serdes_get_sset_count = mv88e6390_serdes_get_sset_count,
 	.serdes_get_strings = mv88e6390_serdes_get_strings,
 	.serdes_get_stats = mv88e6390_serdes_get_stats,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 8271b8aa7b71..795ae5a56834 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -673,6 +673,8 @@ struct mv88e6xxx_ptp_ops {
 	int (*port_disable)(struct mv88e6xxx_chip *chip, int port);
 	int (*global_enable)(struct mv88e6xxx_chip *chip);
 	int (*global_disable)(struct mv88e6xxx_chip *chip);
+	int (*setup_ptp_traps)(struct mv88e6xxx_chip *chip, int port,
+			       bool enable);
 	int n_ext_ts;
 	int arr0_sts_reg;
 	int arr1_sts_reg;
@@ -760,4 +762,7 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
 
 int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
 
+int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
+			   const struct mv88e6xxx_policy *policy);
+
 #endif /* _MV88E6XXX_CHIP_H */
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 8f74ffc7a279..617aeb6cbaac 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -94,6 +94,7 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
 	const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
 	bool tstamp_enable = false;
+	int ret;
 
 	/* Prevent the TX/RX paths from trying to interact with the
 	 * timestamp hardware while we reconfigure it.
@@ -161,6 +162,12 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
 		if (chip->enable_count == 0 && ptp_ops->global_disable)
 			ptp_ops->global_disable(chip);
 	}
+
+	if (ptp_ops->setup_ptp_traps) {
+		ret = ptp_ops->setup_ptp_traps(chip, port, tstamp_enable);
+		if (tstamp_enable && ret)
+			dev_warn(chip->dev, "Failed to setup PTP traps. PTP might not work as desired!\n");
+	}
 	mv88e6xxx_reg_unlock(chip);
 
 	/* Once hardware has been configured, enable timestamp checks
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index d838c174dc0d..8d6ff03d37c8 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -345,6 +345,37 @@ static int mv88e6352_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
 	return 0;
 }
 
+static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port,
+				     bool enable)
+{
+	static const u8 ptp_destinations[][ETH_ALEN] = {
+		{ 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */
+		{ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */
+		{ 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */
+		{ 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */
+		{ 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */
+		{ 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */
+	};
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) {
+		struct mv88e6xxx_policy policy = { };
+
+		policy.mapping	= MV88E6XXX_POLICY_MAPPING_DA;
+		policy.action	= enable ? MV88E6XXX_POLICY_ACTION_TRAP :
+			MV88E6XXX_POLICY_ACTION_NORMAL;
+		policy.port	= port;
+		policy.vid	= 0;
+		ether_addr_copy(policy.addr, ptp_destinations[i]);
+
+		ret = mv88e6xxx_policy_apply(chip, port, &policy);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {
 	.clock_read = mv88e6165_ptp_clock_read,
 	.global_enable = mv88e6165_global_enable,
@@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {
 	.cc_mult_dem = MV88E6XXX_CC_MULT_DEM,
 };
 
+const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {
+	.clock_read = mv88e6352_ptp_clock_read,
+	.ptp_enable = mv88e6352_ptp_enable,
+	.ptp_verify = mv88e6352_ptp_verify,
+	.event_work = mv88e6352_tai_event_work,
+	.port_enable = mv88e6352_hwtstamp_port_enable,
+	.port_disable = mv88e6352_hwtstamp_port_disable,
+	.setup_ptp_traps = mv88e6341_setup_ptp_traps,
+	.n_ext_ts = 1,
+	.arr0_sts_reg = MV88E6XXX_PORT_PTP_ARR0_STS,
+	.arr1_sts_reg = MV88E6XXX_PORT_PTP_ARR1_STS,
+	.dep_sts_reg = MV88E6XXX_PORT_PTP_DEP_STS,
+	.rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ),
+	.cc_shift = MV88E6XXX_CC_SHIFT,
+	.cc_mult = MV88E6XXX_CC_MULT,
+	.cc_mult_num = MV88E6XXX_CC_MULT_NUM,
+	.cc_mult_dem = MV88E6XXX_CC_MULT_DEM,
+};
+
 static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc)
 {
 	struct mv88e6xxx_chip *chip = cc_to_chip(cc);
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index 269d5d16a466..badcb72d10a6 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -151,6 +151,7 @@ void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip);
 extern const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops;
 extern const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops;
 extern const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops;
+extern const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops;
 
 #else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */
 
@@ -171,6 +172,7 @@ static inline void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip)
 static const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {};
 static const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = {};
 static const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {};
+static const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {};
 
 #endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */
 
-- 
2.34.1

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Tobias Waldekranz]

On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@kmk-computers.de> wrote:
> A time aware switch should trap PTP traffic to the management CPU. User space
> daemons such as ptp4l will process these messages to implement Boundary (or
> Transparent) Clocks.
>
> At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages
> which leads to confusion when multiple end devices are connected to the
> switch. Therefore, setup PTP traps. Leverage the already implemented policy
> mechanism based on destination addresses. Configure the traps only if
> timestamping is enabled so that non time aware use case is still functioning.

Do we know how PTP is supposed to work in relation to things like STP?
I.e should you be able to run PTP over a link that is currently in
blocking? It seems like being able to sync your clock before a topology
change occurs would be nice. In that case, these addresses should be
added to the ATU as MGMT instead of policy entries.

> Introduce an additional PTP operation in case other devices need special
> handling in regards to trapping as well.
>
> Tested on Marvell Topaz (mv88e6341) switch with multiple end devices connected
> like this:
>
> |# DSA setup
> |$ ip link set eth0 up
> |$ ip link set lan0 up
> |$ ip link set lan1 up
> |$ ip link set lan2 up
> |$ ip link add name br0 type bridge
> |$ ip link set dev lan0 master br0
> |$ ip link set dev lan1 master br0
> |$ ip link set dev lan2 master br0
> |$ ip link set lan0 up
> |$ ip link set lan1 up
> |$ ip link set lan2 up
> |$ ip link set br0 up
> |$ dhclient br0
> |# Configure bridge routing
> |$ ebtables --table broute --append BROUTING --protocol 0x88F7 --jump DROP
> |# Start linuxptp
> |$ ptp4l -H -2 -i lan0 -i lan1 -i lan2 --tx_timestamp_timeout=40 -m
>
> Verified added policies with mv88e6xxx_dump.
>
> Signed-off-by: Kurt Kanzenbach <kurt@kmk-computers.de>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c     | 12 +++---
>  drivers/net/dsa/mv88e6xxx/chip.h     |  5 +++
>  drivers/net/dsa/mv88e6xxx/hwtstamp.c |  7 ++++
>  drivers/net/dsa/mv88e6xxx/ptp.c      | 59 ++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx/ptp.h      |  2 +
>  5 files changed, 80 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 7fadbf987b23..ab50ebd85f1f 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1816,8 +1816,8 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
>  	return mv88e6xxx_g1_atu_loadpurge(chip, fid, &entry);
>  }
>  
> -static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
> -				  const struct mv88e6xxx_policy *policy)
> +int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
> +			   const struct mv88e6xxx_policy *policy)
>  {
>  	enum mv88e6xxx_policy_mapping mapping = policy->mapping;
>  	enum mv88e6xxx_policy_action action = policy->action;
> @@ -1835,10 +1835,12 @@ static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
>  	case MV88E6XXX_POLICY_MAPPING_SA:
>  		if (action == MV88E6XXX_POLICY_ACTION_NORMAL)
>  			state = 0; /* Dissociate the port and address */
> -		else if (action == MV88E6XXX_POLICY_ACTION_DISCARD &&
> +		else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD ||
> +			  action == MV88E6XXX_POLICY_ACTION_TRAP) &&
>  			 is_multicast_ether_addr(addr))
>  			state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_POLICY;
> -		else if (action == MV88E6XXX_POLICY_ACTION_DISCARD &&
> +		else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD ||
> +			  action == MV88E6XXX_POLICY_ACTION_TRAP) &&
>  			 is_unicast_ether_addr(addr))
>  			state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC_POLICY;
>  		else
> @@ -4589,7 +4591,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
>  	.serdes_irq_status = mv88e6390_serdes_irq_status,
>  	.gpio_ops = &mv88e6352_gpio_ops,
>  	.avb_ops = &mv88e6390_avb_ops,
> -	.ptp_ops = &mv88e6352_ptp_ops,
> +	.ptp_ops = &mv88e6341_ptp_ops,
>  	.serdes_get_sset_count = mv88e6390_serdes_get_sset_count,
>  	.serdes_get_strings = mv88e6390_serdes_get_strings,
>  	.serdes_get_stats = mv88e6390_serdes_get_stats,
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 8271b8aa7b71..795ae5a56834 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -673,6 +673,8 @@ struct mv88e6xxx_ptp_ops {
>  	int (*port_disable)(struct mv88e6xxx_chip *chip, int port);
>  	int (*global_enable)(struct mv88e6xxx_chip *chip);
>  	int (*global_disable)(struct mv88e6xxx_chip *chip);
> +	int (*setup_ptp_traps)(struct mv88e6xxx_chip *chip, int port,
> +			       bool enable);
>  	int n_ext_ts;
>  	int arr0_sts_reg;
>  	int arr1_sts_reg;
> @@ -760,4 +762,7 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
>  
>  int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
>  
> +int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
> +			   const struct mv88e6xxx_policy *policy);
> +
>  #endif /* _MV88E6XXX_CHIP_H */
> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> index 8f74ffc7a279..617aeb6cbaac 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> @@ -94,6 +94,7 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
>  	const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
>  	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
>  	bool tstamp_enable = false;
> +	int ret;
>  
>  	/* Prevent the TX/RX paths from trying to interact with the
>  	 * timestamp hardware while we reconfigure it.
> @@ -161,6 +162,12 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
>  		if (chip->enable_count == 0 && ptp_ops->global_disable)
>  			ptp_ops->global_disable(chip);
>  	}
> +
> +	if (ptp_ops->setup_ptp_traps) {
> +		ret = ptp_ops->setup_ptp_traps(chip, port, tstamp_enable);
> +		if (tstamp_enable && ret)
> +			dev_warn(chip->dev, "Failed to setup PTP traps. PTP might not work as desired!\n");
> +	}
>  	mv88e6xxx_reg_unlock(chip);
>  
>  	/* Once hardware has been configured, enable timestamp checks
> diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
> index d838c174dc0d..8d6ff03d37c8 100644
> --- a/drivers/net/dsa/mv88e6xxx/ptp.c
> +++ b/drivers/net/dsa/mv88e6xxx/ptp.c
> @@ -345,6 +345,37 @@ static int mv88e6352_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
>  	return 0;
>  }
>  
> +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port,
> +				     bool enable)
> +{
> +	static const u8 ptp_destinations[][ETH_ALEN] = {
> +		{ 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */
> +		{ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */
> +		{ 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */
> +		{ 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */
> +		{ 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */
> +		{ 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */

How does the L3 entries above play together with IGMP/MLD? I.e. what
happens if, after launching ptp4l, an IGMP report comes in on lanX,
requesting that same group? Would the policy entry not be overwritten by
mv88e6xxx_port_mdb_add?

Eventually I think we will have many interfaces to configure static
entries in the ATU:

1. MDB entries from a bridge (already in place)
2. User-configured entries through ethtool's rxnfc (already in place)
3. Driver-internal consumers (this patch, MRP, etc.)
4. User-configured entries through TC.

Seems to me like we need to start tracking the owners for these to stop
them from stomping on one another.

> +	};
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) {
> +		struct mv88e6xxx_policy policy = { };
> +
> +		policy.mapping	= MV88E6XXX_POLICY_MAPPING_DA;
> +		policy.action	= enable ? MV88E6XXX_POLICY_ACTION_TRAP :
> +			MV88E6XXX_POLICY_ACTION_NORMAL;
> +		policy.port	= port;
> +		policy.vid	= 0;
> +		ether_addr_copy(policy.addr, ptp_destinations[i]);
> +
> +		ret = mv88e6xxx_policy_apply(chip, port, &policy);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {
>  	.clock_read = mv88e6165_ptp_clock_read,
>  	.global_enable = mv88e6165_global_enable,
> @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {
>  	.cc_mult_dem = MV88E6XXX_CC_MULT_DEM,
>  };
>  
> +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {
> +	.clock_read = mv88e6352_ptp_clock_read,
> +	.ptp_enable = mv88e6352_ptp_enable,
> +	.ptp_verify = mv88e6352_ptp_verify,
> +	.event_work = mv88e6352_tai_event_work,
> +	.port_enable = mv88e6352_hwtstamp_port_enable,
> +	.port_disable = mv88e6352_hwtstamp_port_disable,
> +	.setup_ptp_traps = mv88e6341_setup_ptp_traps,

Is there any reason why this could not be added to the existing ops for
6352 instead? Their ATU's are compatible, IIRC.

> +	.n_ext_ts = 1,
> +	.arr0_sts_reg = MV88E6XXX_PORT_PTP_ARR0_STS,
> +	.arr1_sts_reg = MV88E6XXX_PORT_PTP_ARR1_STS,
> +	.dep_sts_reg = MV88E6XXX_PORT_PTP_DEP_STS,
> +	.rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
> +		(1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ),
> +	.cc_shift = MV88E6XXX_CC_SHIFT,
> +	.cc_mult = MV88E6XXX_CC_MULT,
> +	.cc_mult_num = MV88E6XXX_CC_MULT_NUM,
> +	.cc_mult_dem = MV88E6XXX_CC_MULT_DEM,
> +};
> +
>  static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc)
>  {
>  	struct mv88e6xxx_chip *chip = cc_to_chip(cc);
> diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
> index 269d5d16a466..badcb72d10a6 100644
> --- a/drivers/net/dsa/mv88e6xxx/ptp.h
> +++ b/drivers/net/dsa/mv88e6xxx/ptp.h
> @@ -151,6 +151,7 @@ void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip);
>  extern const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops;
>  extern const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops;
>  extern const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops;
> +extern const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops;
>  
>  #else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */
>  
> @@ -171,6 +172,7 @@ static inline void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip)
>  static const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {};
>  static const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = {};
>  static const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {};
> +static const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {};
>  
>  #endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */
>  
> -- 
> 2.34.1

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Kurt Kanzenbach]

On Fri Dec 10 2021, Tobias Waldekranz wrote:
> On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@kmk-computers.de> wrote:
>> A time aware switch should trap PTP traffic to the management CPU. User space
>> daemons such as ptp4l will process these messages to implement Boundary (or
>> Transparent) Clocks.
>>
>> At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages
>> which leads to confusion when multiple end devices are connected to the
>> switch. Therefore, setup PTP traps. Leverage the already implemented policy
>> mechanism based on destination addresses. Configure the traps only if
>> timestamping is enabled so that non time aware use case is still functioning.
>
> Do we know how PTP is supposed to work in relation to things like STP?
> I.e should you be able to run PTP over a link that is currently in
> blocking? It seems like being able to sync your clock before a topology
> change occurs would be nice. In that case, these addresses should be
> added to the ATU as MGMT instead of policy entries.

Given the fact that the l2 p2p address is already considered as
management traffic (see mv88e6390_g1_mgmt_rsvd2cpu()) maybe all PTP
addresses could be treated as such.

[snip]

>> +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port,
>> +				     bool enable)
>> +{
>> +	static const u8 ptp_destinations[][ETH_ALEN] = {
>> +		{ 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */
>> +		{ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */
>> +		{ 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */
>> +		{ 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */
>> +		{ 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */
>> +		{ 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */
>
> How does the L3 entries above play together with IGMP/MLD? I.e. what
> happens if, after launching ptp4l, an IGMP report comes in on lanX,
> requesting that same group? Would the policy entry not be overwritten by
> mv88e6xxx_port_mdb_add?

Just tested this. Yes it is overwritten without any detection or
errors. Actually I did test UDP as well and didn't notice it. It
obviously depends on the order of events.

>
> Eventually I think we will have many interfaces to configure static
> entries in the ATU:
>
> 1. MDB entries from a bridge (already in place)
> 2. User-configured entries through ethtool's rxnfc (already in place)
> 3. Driver-internal consumers (this patch, MRP, etc.)
> 4. User-configured entries through TC.
>
> Seems to me like we need to start tracking the owners for these to stop
> them from stomping on one another.

Agreed. Some mechanism is required. Any idea how to implement it? In
case of PTP the management/policy entries should take precedence.

>
>> +	};
>> +	int ret, i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) {
>> +		struct mv88e6xxx_policy policy = { };
>> +
>> +		policy.mapping	= MV88E6XXX_POLICY_MAPPING_DA;
>> +		policy.action	= enable ? MV88E6XXX_POLICY_ACTION_TRAP :
>> +			MV88E6XXX_POLICY_ACTION_NORMAL;
>> +		policy.port	= port;
>> +		policy.vid	= 0;
>> +		ether_addr_copy(policy.addr, ptp_destinations[i]);
>> +
>> +		ret = mv88e6xxx_policy_apply(chip, port, &policy);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {
>>  	.clock_read = mv88e6165_ptp_clock_read,
>>  	.global_enable = mv88e6165_global_enable,
>> @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {
>>  	.cc_mult_dem = MV88E6XXX_CC_MULT_DEM,
>>  };
>>  
>> +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {
>> +	.clock_read = mv88e6352_ptp_clock_read,
>> +	.ptp_enable = mv88e6352_ptp_enable,
>> +	.ptp_verify = mv88e6352_ptp_verify,
>> +	.event_work = mv88e6352_tai_event_work,
>> +	.port_enable = mv88e6352_hwtstamp_port_enable,
>> +	.port_disable = mv88e6352_hwtstamp_port_disable,
>> +	.setup_ptp_traps = mv88e6341_setup_ptp_traps,
>
> Is there any reason why this could not be added to the existing ops for
> 6352 instead? Their ATU's are compatible, IIRC.

No particular reason except that I don't have access to a 6352 device to
test it.

Thanks,
Kurt

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Jakub Kicinski]

On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote:
> > At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages
> > which leads to confusion when multiple end devices are connected to the
> > switch. Therefore, setup PTP traps. Leverage the already implemented policy
> > mechanism based on destination addresses. Configure the traps only if
> > timestamping is enabled so that non time aware use case is still functioning.  
> 
> Do we know how PTP is supposed to work in relation to things like STP?
> I.e should you be able to run PTP over a link that is currently in
> blocking?

Not sure if I'm missing the real question but IIRC the standard
calls out that PTP clock distribution tree can be different that
the STP tree, ergo PTP ignores STP forwarding state.

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Richard Cochran]

On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote:
> On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote:
> > Do we know how PTP is supposed to work in relation to things like STP?
> > I.e should you be able to run PTP over a link that is currently in
> > blocking?
> 
> Not sure if I'm missing the real question but IIRC the standard
> calls out that PTP clock distribution tree can be different that
> the STP tree, ergo PTP ignores STP forwarding state.

That is correct.  The PTP will form its own spanning tree, and that
might be different than the STP.  In fact, the Layer2 PTP messages
have special MAC addresses that are supposed to be sent
unconditionally, even over blocked ports.

Thanks,
Richard

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Tobias Waldekranz]

On Fri, Dec 10, 2021 at 18:39, Kurt Kanzenbach <kurt@kmk-computers.de> wrote:
> On Fri Dec 10 2021, Tobias Waldekranz wrote:
>> On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@kmk-computers.de> wrote:
>>> A time aware switch should trap PTP traffic to the management CPU. User space
>>> daemons such as ptp4l will process these messages to implement Boundary (or
>>> Transparent) Clocks.
>>>
>>> At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages
>>> which leads to confusion when multiple end devices are connected to the
>>> switch. Therefore, setup PTP traps. Leverage the already implemented policy
>>> mechanism based on destination addresses. Configure the traps only if
>>> timestamping is enabled so that non time aware use case is still functioning.
>>
>> Do we know how PTP is supposed to work in relation to things like STP?
>> I.e should you be able to run PTP over a link that is currently in
>> blocking? It seems like being able to sync your clock before a topology
>> change occurs would be nice. In that case, these addresses should be
>> added to the ATU as MGMT instead of policy entries.
>
> Given the fact that the l2 p2p address is already considered as
> management traffic (see mv88e6390_g1_mgmt_rsvd2cpu()) maybe all PTP
> addresses could be treated as such.

The feedback from both Jakub and Richard seems to support that.

>
>>> +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port,
>>> +				     bool enable)
>>> +{
>>> +	static const u8 ptp_destinations[][ETH_ALEN] = {
>>> +		{ 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */
>>> +		{ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */
>>> +		{ 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */
>>> +		{ 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */
>>> +		{ 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */
>>> +		{ 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */
>>
>> How does the L3 entries above play together with IGMP/MLD? I.e. what
>> happens if, after launching ptp4l, an IGMP report comes in on lanX,
>> requesting that same group? Would the policy entry not be overwritten by
>> mv88e6xxx_port_mdb_add?
>
> Just tested this. Yes it is overwritten without any detection or
> errors. Actually I did test UDP as well and didn't notice it. It
> obviously depends on the order of events.
>
>>
>> Eventually I think we will have many interfaces to configure static
>> entries in the ATU:
>>
>> 1. MDB entries from a bridge (already in place)
>> 2. User-configured entries through ethtool's rxnfc (already in place)
>> 3. Driver-internal consumers (this patch, MRP, etc.)
>> 4. User-configured entries through TC.
>>
>> Seems to me like we need to start tracking the owners for these to stop
>> them from stomping on one another.
>
> Agreed. Some mechanism is required. Any idea how to implement it? In
> case of PTP the management/policy entries should take precedence.

One approach would be to create a cache containing all static ATU
entries. That way we can easily track the owner of each entry. There are
also major performance advantages of being able to update memberships of
group entries without having to read the entry back from the ATU
first. This is especially important once we start handling router ports
correctly, in which case you have to update all active entries on every
add/remove.

Before going down that route though, I would suggest getting some
initial feedback from Andrew.

A complicating factor, no matter the implementation, is the relationship
between the bridge MDB and all other consumers of ATU entries. As an
example: If the driver first receives an MDB add for one of the L3 PTP
groups, and then a user starts up ptp4l, the driver can't then go back
to the bridge and say "remember that group entry that I said I loaded,
well I have removed it now". So whatever implementation we choose, I
think it needs to keep a shadow entry for the MDB that can be
re-inserted if the corresponding management or policy entry is removed.

You may simply want to allow all consumers to register any given group
with the cache. The cache would then internally elect the "best" entry
and install that to the ATU. Sort of what zebra/quagga/FRR does for
dynamic routing. The priority would probably be something like:

1. Management entry
2. Policy entry
3. MDB entry

This should still result in the proper forwarding of a registered groups
that are shadowed by management or policy entries. The bridge would know
(via skb->offload_fwd_mark) that the packet had not been forwarded in
hardware and would fallback to software forwarding. If the policy entry
was later removed (e.g. PTP was shut down) the MDB entry could be
reinstalled and offloading resumed.

>>
>>> +	};
>>> +	int ret, i;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) {
>>> +		struct mv88e6xxx_policy policy = { };
>>> +
>>> +		policy.mapping	= MV88E6XXX_POLICY_MAPPING_DA;
>>> +		policy.action	= enable ? MV88E6XXX_POLICY_ACTION_TRAP :
>>> +			MV88E6XXX_POLICY_ACTION_NORMAL;
>>> +		policy.port	= port;
>>> +		policy.vid	= 0;
>>> +		ether_addr_copy(policy.addr, ptp_destinations[i]);
>>> +
>>> +		ret = mv88e6xxx_policy_apply(chip, port, &policy);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {
>>>  	.clock_read = mv88e6165_ptp_clock_read,
>>>  	.global_enable = mv88e6165_global_enable,
>>> @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {
>>>  	.cc_mult_dem = MV88E6XXX_CC_MULT_DEM,
>>>  };
>>>  
>>> +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {
>>> +	.clock_read = mv88e6352_ptp_clock_read,
>>> +	.ptp_enable = mv88e6352_ptp_enable,
>>> +	.ptp_verify = mv88e6352_ptp_verify,
>>> +	.event_work = mv88e6352_tai_event_work,
>>> +	.port_enable = mv88e6352_hwtstamp_port_enable,
>>> +	.port_disable = mv88e6352_hwtstamp_port_disable,
>>> +	.setup_ptp_traps = mv88e6341_setup_ptp_traps,
>>
>> Is there any reason why this could not be added to the existing ops for
>> 6352 instead? Their ATU's are compatible, IIRC.
>
> No particular reason except that I don't have access to a 6352 device to
> test it.

Got it. Well I can hopefully be of assistance there. Anyway, I think we
can safely assume that they are compatible with respect to the ATU.

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Kurt Kanzenbach]

On Sat Dec 11 2021, Richard Cochran wrote:
> On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote:
>> On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote:
>> > Do we know how PTP is supposed to work in relation to things like STP?
>> > I.e should you be able to run PTP over a link that is currently in
>> > blocking?
>> 
>> Not sure if I'm missing the real question but IIRC the standard
>> calls out that PTP clock distribution tree can be different that
>> the STP tree, ergo PTP ignores STP forwarding state.
>
> That is correct.  The PTP will form its own spanning tree, and that
> might be different than the STP.  In fact, the Layer2 PTP messages
> have special MAC addresses that are supposed to be sent
> unconditionally, even over blocked ports.

Thanks for clarification. This needs fixing in hellcreek too, as
pass_blocked is currently not set for the ptp fdb entries.

Thanks,
Kurt

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Richard Cochran]

On Sat, Dec 11, 2021 at 07:39:26AM -0800, Richard Cochran wrote:
> On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote:
> > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote:
> > > Do we know how PTP is supposed to work in relation to things like STP?
> > > I.e should you be able to run PTP over a link that is currently in
> > > blocking?
> > 
> > Not sure if I'm missing the real question but IIRC the standard
> > calls out that PTP clock distribution tree can be different that
> > the STP tree, ergo PTP ignores STP forwarding state.
> 
> That is correct.  The PTP will form its own spanning tree, and that
> might be different than the STP.  In fact, the Layer2 PTP messages
> have special MAC addresses that are supposed to be sent
> unconditionally, even over blocked ports.

Let me correct that statement.

I looked at 1588 again, and for E2E TC it states, "All PTP version 2
messages shall be forwarded according to the addressing rules of the
network."  I suppose that includes the STP state.

For P2P TC, there is an exception that the peer delay messages are not
forwarded.  These are generated and consumed by the switch.

The PTP spanning tree still is formed by the Boundary Clocks (BC), and
a Transparent Clock (TC) does not participate in forming the PTP
spanning tree.

What does this mean for Linux DSA switch drivers?

1. All incoming PTP frames should be forwarded to the CPU port, so
   that the software stack may perform its BC or TC functions.

2. When used as a BC, the PTP frames sent from the CPU port should not
   be dropped.

3. When used as a TC, PTP frames sent from the CPU port can be dropped
   on blocked external ports, except for the Peer Delay messages.

Now maybe a particular switch implementation makes it hard to form
rules for #3 that still work for UDP over IPv4/6.

Thanks,
Richard

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Vladimir Oltean]

Hi Richard,

On Mon, Dec 13, 2021 at 04:10:45AM -0800, Richard Cochran wrote:
> On Sat, Dec 11, 2021 at 07:39:26AM -0800, Richard Cochran wrote:
> > On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote:
> > > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote:
> > > > Do we know how PTP is supposed to work in relation to things like STP?
> > > > I.e should you be able to run PTP over a link that is currently in
> > > > blocking?
> > > 
> > > Not sure if I'm missing the real question but IIRC the standard
> > > calls out that PTP clock distribution tree can be different that
> > > the STP tree, ergo PTP ignores STP forwarding state.
> > 
> > That is correct.  The PTP will form its own spanning tree, and that
> > might be different than the STP.  In fact, the Layer2 PTP messages
> > have special MAC addresses that are supposed to be sent
> > unconditionally, even over blocked ports.
> 
> Let me correct that statement.
> 
> I looked at 1588 again, and for E2E TC it states, "All PTP version 2
> messages shall be forwarded according to the addressing rules of the
> network."  I suppose that includes the STP state.
> 
> For P2P TC, there is an exception that the peer delay messages are not
> forwarded.  These are generated and consumed by the switch.
> 
> The PTP spanning tree still is formed by the Boundary Clocks (BC), and
> a Transparent Clock (TC) does not participate in forming the PTP
> spanning tree.
> 
> What does this mean for Linux DSA switch drivers?
> 
> 1. All incoming PTP frames should be forwarded to the CPU port, so
>    that the software stack may perform its BC or TC functions.
> 
> 2. When used as a BC, the PTP frames sent from the CPU port should not
>    be dropped.
> 
> 3. When used as a TC, PTP frames sent from the CPU port can be dropped
>    on blocked external ports, except for the Peer Delay messages.
> 
> Now maybe a particular switch implementation makes it hard to form
> rules for #3 that still work for UDP over IPv4/6.

With other drivers, all packets injected from the CPU port act as if in
"god mode", bypassing any STP state. It then becomes the responsibility
of the software to not send packets on a port that is blocking,
except for packets for control protocols. Would you agree that ptp4l
should consider monitoring whether its ports are under a bridge, and
what STP state that bridge port is in? I think this isn't even specific
to DSA, the same thing would happen with software bridging: packets sent
through sockets opened on the bridge would have egress prevented on
blocking ports by virtue of the bridge driver code, but packets sent
through sockets opened on the bridge port directly, I don't think the
bridge driver has any saying about the STP state. So similarly, it
becomes the application's responsibility.

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Andrew Lunn]

>  I think this isn't even specific to DSA, the same thing would
> happen with software bridging

And pure switchdev switches. I wonder what the Mellanox switch does in
this case?

     Andrew

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Jakub Kicinski]

On Mon, 13 Dec 2021 04:10:45 -0800 Richard Cochran wrote:
> I looked at 1588 again, and for E2E TC it states, "All PTP version 2
> messages shall be forwarded according to the addressing rules of the
> network."  I suppose that includes the STP state.
> 
> For P2P TC, there is an exception that the peer delay messages are not
> forwarded.  These are generated and consumed by the switch.

Indeed, looks like a v1 vs v2 change :S

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Richard Cochran]

On Mon, Dec 13, 2021 at 08:44:25AM -0800, Jakub Kicinski wrote:
> Indeed, looks like a v1 vs v2 change :S

FWIW, the peer delay mechanism was new in 1588v2.

Thanks,
Richard

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Richard Cochran]

On Mon, Dec 13, 2021 at 02:31:47PM +0200, Vladimir Oltean wrote:

> With other drivers, all packets injected from the CPU port act as if in
> "god mode", bypassing any STP state. It then becomes the responsibility
> of the software to not send packets on a port that is blocking,
> except for packets for control protocols. Would you agree that ptp4l
> should consider monitoring whether its ports are under a bridge, and
> what STP state that bridge port is in?

Perhaps.  linuxptp TC mode will forward frames out all configured
interfaces.  If the bridge can't drop the PTP frames automatically,
then this could cause loops.

So if switch HW in general won't drop them, then, yes, the TC user
space stack will need to follow the STP state.

> I think this isn't even specific
> to DSA, the same thing would happen with software bridging:

(Linux doesn't support even SW time stamping on SW bridges, so you
can't have a TC running in this case.)

Thanks,
Richard

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Kurt Kanzenbach]

On Mon Dec 13 2021, Richard Cochran wrote:
> On Sat, Dec 11, 2021 at 07:39:26AM -0800, Richard Cochran wrote:
>> On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote:
>> > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote:
>> > > Do we know how PTP is supposed to work in relation to things like STP?
>> > > I.e should you be able to run PTP over a link that is currently in
>> > > blocking?
>> > 
>> > Not sure if I'm missing the real question but IIRC the standard
>> > calls out that PTP clock distribution tree can be different that
>> > the STP tree, ergo PTP ignores STP forwarding state.
>> 
>> That is correct.  The PTP will form its own spanning tree, and that
>> might be different than the STP.  In fact, the Layer2 PTP messages
>> have special MAC addresses that are supposed to be sent
>> unconditionally, even over blocked ports.
>
> Let me correct that statement.
>
> I looked at 1588 again, and for E2E TC it states, "All PTP version 2
> messages shall be forwarded according to the addressing rules of the
> network."  I suppose that includes the STP state.
>
> For P2P TC, there is an exception that the peer delay messages are not
> forwarded.  These are generated and consumed by the switch.
>
> The PTP spanning tree still is formed by the Boundary Clocks (BC), and
> a Transparent Clock (TC) does not participate in forming the PTP
> spanning tree.
>
> What does this mean for Linux DSA switch drivers?
>
> 1. All incoming PTP frames should be forwarded to the CPU port, so
>    that the software stack may perform its BC or TC functions.
>
> 2. When used as a BC, the PTP frames sent from the CPU port should not
>    be dropped.
>
> 3. When used as a TC, PTP frames sent from the CPU port can be dropped
>    on blocked external ports, except for the Peer Delay messages.

Maybe I'm missing something, but how is #2 and #3 supposed to work with
DSA? The switch driver doesn't know whether the user wants to run BC or
TC. For #2 the STP state is ignored, for #3 it is not except for peer
delay measurements.

Thanks,
Kurt

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Kurt Kanzenbach]

On Sun Dec 12 2021, Tobias Waldekranz wrote:
>> Agreed. Some mechanism is required. Any idea how to implement it? In
>> case of PTP the management/policy entries should take precedence.
>
> One approach would be to create a cache containing all static ATU
> entries. That way we can easily track the owner of each entry. There are
> also major performance advantages of being able to update memberships of
> group entries without having to read the entry back from the ATU
> first. This is especially important once we start handling router ports
> correctly, in which case you have to update all active entries on every
> add/remove.
>
> Before going down that route though, I would suggest getting some
> initial feedback from Andrew.
>
> A complicating factor, no matter the implementation, is the relationship
> between the bridge MDB and all other consumers of ATU entries. As an
> example: If the driver first receives an MDB add for one of the L3 PTP
> groups, and then a user starts up ptp4l, the driver can't then go back
> to the bridge and say "remember that group entry that I said I loaded,
> well I have removed it now". So whatever implementation we choose, I
> think it needs to keep a shadow entry for the MDB that can be
> re-inserted if the corresponding management or policy entry is removed.
>
> You may simply want to allow all consumers to register any given group
> with the cache. The cache would then internally elect the "best" entry
> and install that to the ATU. Sort of what zebra/quagga/FRR does for
> dynamic routing. The priority would probably be something like:
>
> 1. Management entry
> 2. Policy entry
> 3. MDB entry
>
> This should still result in the proper forwarding of a registered groups
> that are shadowed by management or policy entries. The bridge would know
> (via skb->offload_fwd_mark) that the packet had not been forwarded in
> hardware and would fallback to software forwarding. If the policy entry
> was later removed (e.g. PTP was shut down) the MDB entry could be
> reinstalled and offloading resumed.

Thanks. This approach makes sense and should solve the problem at hand.

Thanks,
Kurt

Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

[Vladimir Oltean]

On Mon, Dec 13, 2021 at 09:11:40AM -0800, Richard Cochran wrote:
> On Mon, Dec 13, 2021 at 02:31:47PM +0200, Vladimir Oltean wrote:
> 
> > With other drivers, all packets injected from the CPU port act as if in
> > "god mode", bypassing any STP state. It then becomes the responsibility
> > of the software to not send packets on a port that is blocking,
> > except for packets for control protocols. Would you agree that ptp4l
> > should consider monitoring whether its ports are under a bridge, and
> > what STP state that bridge port is in?
> 
> Perhaps.  linuxptp TC mode will forward frames out all configured
> interfaces.  If the bridge can't drop the PTP frames automatically,
> then this could cause loops.

Considering that in this configuration:

ip link add br0 type bridge
ip link set swp0 master br0
ip link set swp1 master br0
ip link set swp2 master br0
ip link set swp3 master br0
ptp4l -i swp0 -i swp1 -i swp2 -i swp3 -f configs/P2P-TC.cfg -m

the kernel code path for PTP packets has nothing to do with the bridge
driver (unless maybe an explicit netfilter rule to tell the bridge RX
handler to not steal those packets from the physical port), I think it's
safe to say that yes, the bridge can't drop the PTP frames automatically.

> So if switch HW in general won't drop them, then, yes, the TC user
> space stack will need to follow the STP state.

The hardware offloads tend to follow the software model as closely as
possible, and as mentioned, I think this has to do with the software
model in this case, rather than a hardware quirk. I would consider the
hardware to be quirky quite in the opposite case: you send a packet
through a physical port rather than the bridge, and that one is affected
by the STP state. We have switch drivers like that too, but let's not go
there, they're rather the exception.

> > I think this isn't even specific
> > to DSA, the same thing would happen with software bridging:
> 
> (Linux doesn't support even SW time stamping on SW bridges, so you
> can't have a TC running in this case.)

As also rephrased in the replies above, the point was that STP state has
nothing to do, in general, with whether this is a DSA switch or not.
It is a bridging service concept, and applies to data plane packets,
which ptp4l isn't sending/receiving, since as you very well point out,
your socket is opened on the physical port and not on the bridge device.
So don't expect the STP state of the port to do something here. If you
send a packet on a socket opened on an interface that is backed by a
physical port, expect that packet to be sent - is the main point.
The fact that I used the "software bridging" term was just to clarify
that it hasn't got anything to do with whether the bridging is offloaded
or not.