Message-ID: <87r1ai21ac.fsf@waldekranz.com>
Date:   Sun, 12 Dec 2021 00:02:19 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Kurt Kanzenbach <kurt@...-computers.de>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic

On Fri, Dec 10, 2021 at 18:39, Kurt Kanzenbach <kurt@...-computers.de> wrote:
> On Fri Dec 10 2021, Tobias Waldekranz wrote:
>> On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@...-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.

Powered by blists - more mailing lists