Re: [PATCH net 1/2] net: dsa: tag_sja1105: fix MAC DA patching from meta frames

Re: [PATCH net 1/2] net: dsa: tag_sja1105: fix MAC DA patching from meta frames

[Simon Horman]

On Thu, Jun 29, 2023 at 05:14:52PM +0300, Vladimir Oltean wrote:
> The SJA1105 manual says that at offset 4 into the meta frame payload we
> have "MAC destination byte 2" and at offset 5 we have "MAC destination
> byte 1". These are counted from the LSB, so byte 1 is h_dest[ETH_HLEN-2]
> aka h_dest[4] and byte 2 is h_dest[ETH_HLEN-3] aka h_dest[3].
> 
> The sja1105_meta_unpack() function decodes these the other way around,
> so a frame with MAC DA 01:80:c2:11:22:33 is received by the network
> stack as having 01:80:c2:22:11:33.
> 
> Fixes: e53e18a6fe4d ("net: dsa: sja1105: Receive and decode meta frames")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net 1/2] net: dsa: tag_sja1105: fix MAC DA patching from meta frames

[Florian Fainelli]

On 6/29/2023 4:14 PM, Vladimir Oltean wrote:
> The SJA1105 manual says that at offset 4 into the meta frame payload we
> have "MAC destination byte 2" and at offset 5 we have "MAC destination
> byte 1". These are counted from the LSB, so byte 1 is h_dest[ETH_HLEN-2]
> aka h_dest[4] and byte 2 is h_dest[ETH_HLEN-3] aka h_dest[3].
> 
> The sja1105_meta_unpack() function decodes these the other way around,
> so a frame with MAC DA 01:80:c2:11:22:33 is received by the network
> stack as having 01:80:c2:22:11:33.
> 
> Fixes: e53e18a6fe4d ("net: dsa: sja1105: Receive and decode meta frames")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 2/2] net: dsa: sja1105: always enable the send_meta options

[Simon Horman]

On Thu, Jun 29, 2023 at 05:14:53PM +0300, Vladimir Oltean wrote:
> incl_srcpt has the limitation, mentioned in commit b4638af8885a ("net:
> dsa: sja1105: always enable the INCL_SRCPT option"), that frames with a
> MAC DA of 01:80:c2:xx:yy:zz will be received as 01:80:c2:00:00:zz unless
> PTP RX timestamping is enabled.
> 
> The incl_srcpt option was initially unconditionally enabled, then that
> changed with commit 42824463d38d ("net: dsa: sja1105: Limit use of
> incl_srcpt to bridge+vlan mode"), then again with b4638af8885a ("net:
> dsa: sja1105: always enable the INCL_SRCPT option"). Bottom line is that
> it now needs to be always enabled, otherwise the driver does not have a
> reliable source of information regarding source_port and switch_id for
> link-local traffic (tag_8021q VLANs may be imprecise since now they
> identify an entire bridging domain when ports are not standalone).
> 
> If we accept that PTP RX timestamping (and therefore, meta frame
> generation) is always enabled in hardware, then that limitation could be
> avoided and packets with any MAC DA can be properly received, because
> meta frames do contain the original bytes from the MAC DA of their
> associated link-local packet.
> 
> This change enables meta frame generation unconditionally, which also
> has the nice side effects of simplifying the switch control path
> (a switch reset is no longer required on hwtstamping settings change)
> and the tagger data path (it no longer needs to be informed whether to
> expect meta frames or not - it always does).
> 
> Fixes: 227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through standalone ports")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/sja1105/sja1105.h      |  2 +-
>  drivers/net/dsa/sja1105/sja1105_main.c |  5 ++-
>  drivers/net/dsa/sja1105/sja1105_ptp.c  | 48 +++-----------------------
>  include/linux/dsa/sja1105.h            |  4 ---
>  net/dsa/tag_sja1105.c                  | 45 ------------------------
>  5 files changed, 7 insertions(+), 97 deletions(-)

Hi Vladimir,

this patch isn't that big, so I'm ok with it.  But it also isn't that
small, so I'd just like to mention that a different approach might be a
small patch that enables meta frame generation unconditionally, as a fix.
And then, later, some cleanup, which seems to comprise most of this patch.

I do admit that I didn't try this. So it might not be sensible.  And as I
said, I am ok with this patch. But I did think it was worth mentioning.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

...

Re: [PATCH net 2/2] net: dsa: sja1105: always enable the send_meta options

[Vladimir Oltean]

Hi Simon,

On Fri, Jun 30, 2023 at 03:35:23PM +0200, Simon Horman wrote:
> Hi Vladimir,
> 
> this patch isn't that big, so I'm ok with it.  But it also isn't that
> small, so I'd just like to mention that a different approach might be a
> small patch that enables meta frame generation unconditionally, as a fix.
> And then, later, some cleanup, which seems to comprise most of this patch.
> 
> I do admit that I didn't try this. So it might not be sensible.  And as I
> said, I am ok with this patch. But I did think it was worth mentioning.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> ...

Thanks for the feedback.

You're saying that it's preferable to leave sja1105_rxtstamp_set_state()
and sja1105_rxtstamp_get_state() where they are, accessible through
tagger_data->rxtstamp_set_state() and tagger_data->rxtstamp_get_state(),
but to only call tagger_data->rxtstamp_set_state() once, statically with
"true", and to never call tagger_data->rxtstamp_get_state()?

I probably could, but I'd have to delay the tagger_data->rxtstamp_set_state()
call until sja1105_connect_tag_protocol(); it's not going to be available
earlier. And this is going to be just filler code that I will then delete
as soon as net-next reopens.

Re: [PATCH net 2/2] net: dsa: sja1105: always enable the send_meta options

[Simon Horman]

On Fri, Jun 30, 2023 at 07:53:00PM +0300, Vladimir Oltean wrote:
> Hi Simon,
> 
> On Fri, Jun 30, 2023 at 03:35:23PM +0200, Simon Horman wrote:
> > Hi Vladimir,
> > 
> > this patch isn't that big, so I'm ok with it.  But it also isn't that
> > small, so I'd just like to mention that a different approach might be a
> > small patch that enables meta frame generation unconditionally, as a fix.
> > And then, later, some cleanup, which seems to comprise most of this patch.
> > 
> > I do admit that I didn't try this. So it might not be sensible.  And as I
> > said, I am ok with this patch. But I did think it was worth mentioning.
> > 
> > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > 
> > ...
> 
> Thanks for the feedback.
> 
> You're saying that it's preferable to leave sja1105_rxtstamp_set_state()
> and sja1105_rxtstamp_get_state() where they are, accessible through
> tagger_data->rxtstamp_set_state() and tagger_data->rxtstamp_get_state(),
> but to only call tagger_data->rxtstamp_set_state() once, statically with
> "true", and to never call tagger_data->rxtstamp_get_state()?

I'm not the ultimate authority on what is and isn't acceptable.
But, yes, I was suggesting something like that.

> I probably could, but I'd have to delay the tagger_data->rxtstamp_set_state()
> call until sja1105_connect_tag_protocol(); it's not going to be available
> earlier. And this is going to be just filler code that I will then delete
> as soon as net-next reopens.

Right, so there is complexity. And dead code.
Or we could just cut to the chase, which is what you have done.

So it seems your approach is best.

Re: [PATCH net 2/2] net: dsa: sja1105: always enable the send_meta options

[Florian Fainelli]

On 6/29/2023 4:14 PM, Vladimir Oltean wrote:
> incl_srcpt has the limitation, mentioned in commit b4638af8885a ("net:
> dsa: sja1105: always enable the INCL_SRCPT option"), that frames with a
> MAC DA of 01:80:c2:xx:yy:zz will be received as 01:80:c2:00:00:zz unless
> PTP RX timestamping is enabled.
> 
> The incl_srcpt option was initially unconditionally enabled, then that
> changed with commit 42824463d38d ("net: dsa: sja1105: Limit use of
> incl_srcpt to bridge+vlan mode"), then again with b4638af8885a ("net:
> dsa: sja1105: always enable the INCL_SRCPT option"). Bottom line is that
> it now needs to be always enabled, otherwise the driver does not have a
> reliable source of information regarding source_port and switch_id for
> link-local traffic (tag_8021q VLANs may be imprecise since now they
> identify an entire bridging domain when ports are not standalone).
> 
> If we accept that PTP RX timestamping (and therefore, meta frame
> generation) is always enabled in hardware, then that limitation could be
> avoided and packets with any MAC DA can be properly received, because
> meta frames do contain the original bytes from the MAC DA of their
> associated link-local packet.
> 
> This change enables meta frame generation unconditionally, which also
> has the nice side effects of simplifying the switch control path
> (a switch reset is no longer required on hwtstamping settings change)
> and the tagger data path (it no longer needs to be informed whether to
> expect meta frames or not - it always does).
> 
> Fixes: 227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through standalone ports")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 0/2] Fix mangled link-local MAC DAs with SJA1105 DSA

[Jakub Kicinski]

On Thu, 29 Jun 2023 17:14:51 +0300 Vladimir Oltean wrote:
> The SJA1105 hardware tagging protocol is weird and will put DSA
> information (source port, switch ID) in the MAC DA of the packets sent
> to the CPU, and then send some additional (meta) packets which contain
> the original bytes from the previous packet's MAC DA.
> 
> The tagging protocol driver contains logic to handle this, but the meta
> frames are optional functionality, and there are configurations when
> they aren't received (no PTP RX timestamping). Thus, the MAC DA from
> packets sent to the stack is not correct in all cases.
> 
> Also, during testing it was found that the MAC DA patching procedure was
> incorrect.
> 
> The investigation comes as a result of this discussion with Paolo:
> https://lore.kernel.org/netdev/f494387c8d55d9b1d5a3e88beedeeb448f2e6cc3.camel@redhat.com/

This series got eaten by vger, I think. Could you repost?