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

On Fri Dec 10 2021, Tobias Waldekranz wrote:

On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach [off-list ref] wrote:

quoted

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]

quoted

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

quoted

+	};
+	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