Re: [PATCH v2 net 1/2] net: dsa: sja1105: always enable the INCL_SRCPT option

On Thu, 2023-06-29 at 13:19 +0300, Vladimir Oltean wrote:

On Thu, Jun 29, 2023 at 11:36:38AM +0200, Paolo Abeni wrote:

quoted

quoted

The big drawback with INCL_SRCPT is that it makes it impossible to
distinguish between an original MAC DA of 01:80:C2:XX:YY:ZZ and
01:80:C2:AA:BB:ZZ, because the tagger just patches MAC DA bytes 3 and 4
with zeroes. Only if PTP RX timestamping is enabled, the switch will
generate a META follow-up frame containing the RX timestamp and the
original bytes 3 and 4 of the MAC DA. Those will be used to patch up the
original packet. Nonetheless, in the absence of PTP RX timestamping, we
have to live with this limitation, since it is more important to have
the more precise source port information for link-local traffic.

What if 2 different DSA are under the same linux bridge, so that the
host has to forward in S/W the received frames? (and DA is incomplete)

It looks like that such frames will never reach the relevant
destination?

Is such setup possible/relevant?

Thanks,

Paolo

They will have an incorrect MAC DA, with all the consequences of that.

Given the fact that the incl_srcpt was enabled up until now for the
vlan_filtering=1 bridge case only, this was already possible to see.
However it was never reported to me as being a problem, unlike what
is being fixed here.

Ok, the above sounds like a good enough reply to me.

I see no other escape than to unconditionally enable the send_meta
options as well, so that the overwritten MAC DA bytes can always be
reconstructed from the upcoming meta frames, even though the RX
timestamp (main payload of those meta frames) may or may not be useful.
Doing that might also have the benefit that it simplifies the code,
removing the need for tagger_data->rxtstamp_set_state() and
tagger_data->rxtstamp_get_state(), because with that simplification,
the tagger will always expect meta frames.

Because of the lack of complaints, I was considering that as net-next
material though.

[I'm mixing replies to your 2 emails here, I hope overall this is still
human parsable ;) ]

Quickly skimming over the patch you shared I *think* it could be -net
material, too. Given the mentioned lack of complains for the potential
issue, I think it could be a follow-up to this series.

I'm applying it right now.

Thanks!

Paolo