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

[Paolo Abeni]

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:
> > > 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