[PATCH v2 net 0/2] Fix PTP received on wrong port with bridged SJA1105 DSA

[PATCH v2 net 0/2] Fix PTP received on wrong port with bridged SJA1105 DSA

[Vladimir Oltean]

Since the changes were made to tag_8021q to support imprecise RX for
bridged ports, the tag_sja1105 driver still prefers the source port
information deduced from the VLAN headers for link-local traffic, even
though the switch can theoretically do better and report the precise
source port.

The problem is that the tagger doesn't know when to trust one source of
information over another, because the INCL_SRCPT option (to "tag" link
local frames) is sometimes enabled and sometimes it isn't.

The first patch makes the switch provide the hardware tag for link local
traffic under all circumstances, and the second patch makes the tagger
always use that hardware tag as primary source of information for link
local packets.

Vladimir Oltean (2):
  net: dsa: sja1105: always enable the INCL_SRCPT option
  net: dsa: tag_sja1105: always prefer source port information from
    INCL_SRCPT

 drivers/net/dsa/sja1105/sja1105_main.c |  9 ++----
 net/dsa/tag_sja1105.c                  | 38 ++++++++++++++++++++------
 2 files changed, 31 insertions(+), 16 deletions(-)

-- 
2.34.1

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

[Vladimir Oltean]

Link-local traffic on bridged SJA1105 ports is sometimes tagged by the
hardware with source port information (when the port is under a VLAN
aware bridge).

The tag_8021q source port identification has become more loose
("imprecise") and will report a plausible rather than exact bridge port,
when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
traffic always needs to know the precise source port.

Modify the driver logic (and therefore: the tagging protocol itself) to
always include the source port information with link-local packets,
regardless of whether the port is standalone, under a VLAN-aware or
VLAN-unaware bridge. This makes it possible for the tagging driver to
give priority to that information over the tag_8021q VLAN header.

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.

Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 drivers/net/dsa/sja1105/sja1105_main.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index a55a6436fc05..dd154b2b9680 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -866,11 +866,11 @@ static int sja1105_init_general_params(struct sja1105_private *priv)
 		.hostprio = 7,
 		.mac_fltres1 = SJA1105_LINKLOCAL_FILTER_A,
 		.mac_flt1    = SJA1105_LINKLOCAL_FILTER_A_MASK,
-		.incl_srcpt1 = false,
+		.incl_srcpt1 = true,
 		.send_meta1  = false,
 		.mac_fltres0 = SJA1105_LINKLOCAL_FILTER_B,
 		.mac_flt0    = SJA1105_LINKLOCAL_FILTER_B_MASK,
-		.incl_srcpt0 = false,
+		.incl_srcpt0 = true,
 		.send_meta0  = false,
 		/* Default to an invalid value */
 		.mirr_port = priv->ds->num_ports,
@@ -2405,11 +2405,6 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
 	general_params->tpid = tpid;
 	/* EtherType used to identify outer tagged (S-tag) VLAN traffic */
 	general_params->tpid2 = tpid2;
-	/* When VLAN filtering is on, we need to at least be able to
-	 * decode management traffic through the "backup plan".
-	 */
-	general_params->incl_srcpt1 = enabled;
-	general_params->incl_srcpt0 = enabled;
 
 	for (port = 0; port < ds->num_ports; port++) {
 		if (dsa_is_unused_port(ds, port))
-- 
2.34.1

[PATCH v2 net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Vladimir Oltean]

Currently the sja1105 tagging protocol prefers using the source port
information from the VLAN header if that is available, falling back to
the INCL_SRCPT option if it isn't. The VLAN header is available for all
frames except for META frames initiated by the switch (containing RX
timestamps), and thus, the "if (is_link_local)" branch is practically
dead.

The tag_8021q source port identification has become more loose
("imprecise") and will report a plausible rather than exact bridge port,
when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
traffic always needs to know the precise source port. With incorrect
source port reporting, for example PTP traffic over 2 bridged ports will
all be seen on sockets opened on the first such port, which is incorrect.

Now that the tagging protocol has been changed to make link-local frames
always contain source port information, we can reverse the order of the
checks so that we always give precedence to that information (which is
always precise) in lieu of the tag_8021q VID which is only precise for a
standalone port.

Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: protect against malformed input packets ("vid" variable may be
uninitialized)

 net/dsa/tag_sja1105.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index a5f3b73da417..92a626a05e82 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -545,10 +545,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 	is_link_local = sja1105_is_link_local(skb);
 	is_meta = sja1105_is_meta_frame(skb);
 
-	if (sja1105_skb_has_tag_8021q(skb)) {
-		/* Normal traffic path. */
-		sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid);
-	} else if (is_link_local) {
+	if (is_link_local) {
 		/* Management traffic path. Switch embeds the switch ID and
 		 * port ID into bytes of the destination MAC, courtesy of
 		 * the incl_srcpt options.
@@ -562,16 +559,39 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 		sja1105_meta_unpack(skb, &meta);
 		source_port = meta.source_port;
 		switch_id = meta.switch_id;
-	} else {
+	}
+
+	/* Normal data plane traffic and link-local frames are tagged with
+	 * a tag_8021q VLAN which we have to strip
+	 */
+	if (sja1105_skb_has_tag_8021q(skb)) {
+		int tmp_source_port = -1, tmp_switch_id = -1;
+
+		sja1105_vlan_rcv(skb, &tmp_source_port, &tmp_switch_id, &vbid,
+				 &vid);
+		/* Preserve the source information from the INCL_SRCPT option,
+		 * if available. This allows us to not overwrite a valid source
+		 * port and switch ID with zeroes when receiving link-local
+		 * frames from a VLAN-unaware bridged port (non-zero vbid) or a
+		 * VLAN-aware bridged port (non-zero vid).
+		 */
+		if (source_port == -1)
+			source_port = tmp_source_port;
+		if (switch_id == -1)
+			switch_id = tmp_switch_id;
+	} else if (source_port == -1 && switch_id == -1) {
+		/* Packets with no source information have no chance of
+		 * getting accepted, drop them straight away.
+		 */
 		return NULL;
 	}
 
-	if (vbid >= 1)
+	if (source_port != -1 && switch_id != -1)
+		skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
+	else if (vbid >= 1)
 		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
-	else if (source_port == -1 || switch_id == -1)
-		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
 	else
-		skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
+		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
 	if (!skb->dev) {
 		netdev_warn(netdev, "Couldn't decode source port\n");
 		return NULL;
-- 
2.34.1

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

[Simon Horman]

On Tue, Jun 27, 2023 at 12:42:06PM +0300, Vladimir Oltean wrote:
> Link-local traffic on bridged SJA1105 ports is sometimes tagged by the
> hardware with source port information (when the port is under a VLAN
> aware bridge).
> 
> The tag_8021q source port identification has become more loose
> ("imprecise") and will report a plausible rather than exact bridge port,
> when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
> traffic always needs to know the precise source port.
> 
> Modify the driver logic (and therefore: the tagging protocol itself) to
> always include the source port information with link-local packets,
> regardless of whether the port is standalone, under a VLAN-aware or
> VLAN-unaware bridge. This makes it possible for the tagging driver to
> give priority to that information over the tag_8021q VLAN header.
> 
> 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.
> 
> Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
> Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

Re: [PATCH v2 net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Simon Horman]

On Tue, Jun 27, 2023 at 12:42:07PM +0300, Vladimir Oltean wrote:
> Currently the sja1105 tagging protocol prefers using the source port
> information from the VLAN header if that is available, falling back to
> the INCL_SRCPT option if it isn't. The VLAN header is available for all
> frames except for META frames initiated by the switch (containing RX
> timestamps), and thus, the "if (is_link_local)" branch is practically
> dead.
> 
> The tag_8021q source port identification has become more loose
> ("imprecise") and will report a plausible rather than exact bridge port,
> when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
> traffic always needs to know the precise source port. With incorrect
> source port reporting, for example PTP traffic over 2 bridged ports will
> all be seen on sockets opened on the first such port, which is incorrect.
> 
> Now that the tagging protocol has been changed to make link-local frames
> always contain source port information, we can reverse the order of the
> checks so that we always give precedence to that information (which is
> always precise) in lieu of the tag_8021q VID which is only precise for a
> standalone port.
> 
> Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
> Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: protect against malformed input packets ("vid" variable may be
> uninitialized)

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

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

[Paolo Abeni]

On Tue, 2023-06-27 at 12:42 +0300, Vladimir Oltean wrote:
> Link-local traffic on bridged SJA1105 ports is sometimes tagged by the
> hardware with source port information (when the port is under a VLAN
> aware bridge).
> 
> The tag_8021q source port identification has become more loose
> ("imprecise") and will report a plausible rather than exact bridge port,
> when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
> traffic always needs to know the precise source port.
> 
> Modify the driver logic (and therefore: the tagging protocol itself) to
> always include the source port information with link-local packets,
> regardless of whether the port is standalone, under a VLAN-aware or
> VLAN-unaware bridge. This makes it possible for the tagging driver to
> give priority to that information over the tag_8021q VLAN header.
> 
> 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

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

[Vladimir Oltean]

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.

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.

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

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

On Thu, Jun 29, 2023 at 01:19:50PM +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.
> 
> 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.

This is the patch that would avoid that. Not sure whether I should
include it as part of the "net" series or if it's better to retarget
it to net-next. It has an interesting diffstat of "5 files changed,
7 insertions(+), 93 deletions(-)", so there's that.

The patch is attached because I'm trying not to post code that patchwork
would think it needs to process.

[-- Attachment #2: 0001-net-dsa-sja1105-always-enable-the-send_meta-options.patch --]
[-- Type: text/x-diff, Size: 10100 bytes --]

From b450f96d8f313791909ac70a913a55fc65fdc1e8 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Thu, 29 Jun 2023 13:41:59 +0300
Subject: [PATCH] net: dsa: sja1105: always enable the send_meta options

incl_srcpt has the limitation, mentioned in commit "net: dsa: sja1105:
always enable the INCL_SRCPT option" (FIXME not yet merged), 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 "net: dsa: sja1105:
always enable the INCL_SRCPT option". Bottom line is that it needs to be
always enabled, otherwise the driver in the current form does not have a
more reliable source of information regarding source_port/switch_id for
link-local traffic (tag_8021q VLANs may be imprecise).

If we accept that PTP RX timestamping (and therefore, meta frame
generation) is always enabled in hardware, then that limitation could be
avoided, 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
simplifies 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: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch")
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                  | 41 ----------------------
 5 files changed, 7 insertions(+), 93 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index fb1549a5fe32..dee35ba924ad 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -252,6 +252,7 @@ struct sja1105_private {
 	unsigned long ucast_egress_floods;
 	unsigned long bcast_egress_floods;
 	unsigned long hwts_tx_en;
+	unsigned long hwts_rx_en;
 	const struct sja1105_info *info;
 	size_t max_xfer_len;
 	struct spi_device *spidev;
@@ -289,7 +290,6 @@ struct sja1105_spi_message {
 /* From sja1105_main.c */
 enum sja1105_reset_reason {
 	SJA1105_VLAN_FILTERING = 0,
-	SJA1105_RX_HWTSTAMPING,
 	SJA1105_AGEING_TIME,
 	SJA1105_SCHEDULING,
 	SJA1105_BEST_EFFORT_POLICING,
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index cdbadae923dc..0e21a789ca4a 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -870,11 +870,11 @@ static int sja1105_init_general_params(struct sja1105_private *priv)
 		.mac_fltres1 = SJA1105_LINKLOCAL_FILTER_A,
 		.mac_flt1    = SJA1105_LINKLOCAL_FILTER_A_MASK,
 		.incl_srcpt1 = true,
-		.send_meta1  = false,
+		.send_meta1  = true,
 		.mac_fltres0 = SJA1105_LINKLOCAL_FILTER_B,
 		.mac_flt0    = SJA1105_LINKLOCAL_FILTER_B_MASK,
 		.incl_srcpt0 = true,
-		.send_meta0  = false,
+		.send_meta0  = true,
 		/* Default to an invalid value */
 		.mirr_port = priv->ds->num_ports,
 		/* No TTEthernet */
@@ -2218,7 +2218,6 @@ static int sja1105_reload_cbs(struct sja1105_private *priv)
 
 static const char * const sja1105_reset_reasons[] = {
 	[SJA1105_VLAN_FILTERING] = "VLAN filtering",
-	[SJA1105_RX_HWTSTAMPING] = "RX timestamping",
 	[SJA1105_AGEING_TIME] = "Ageing time",
 	[SJA1105_SCHEDULING] = "Time-aware scheduling",
 	[SJA1105_BEST_EFFORT_POLICING] = "Best-effort policing",
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index f47ad55a811a..0bb99ee5ebf9 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -61,35 +61,10 @@ enum sja1105_ptp_clk_mode {
 #define SJA1105_PTPEGR_SLEEP_US		50
 #define SJA1105_PTPEGR_TIMEOUT_US	1000000
 
-/* Must be called only while the RX timestamping state of the tagger
- * is turned off
- */
-static int sja1105_change_rxtstamping(struct sja1105_private *priv,
-				      bool on)
-{
-	struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
-	struct sja1105_general_params_entry *general_params;
-	struct sja1105_table *table;
-
-	table = &priv->static_config.tables[BLK_IDX_GENERAL_PARAMS];
-	general_params = table->entries;
-	general_params->send_meta1 = on;
-	general_params->send_meta0 = on;
-
-	ptp_cancel_worker_sync(ptp_data->clock);
-	skb_queue_purge(&ptp_data->skb_txtstamp_queue);
-	skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
-
-	return sja1105_static_config_reload(priv, SJA1105_RX_HWTSTAMPING);
-}
-
 int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 {
-	struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
 	struct sja1105_private *priv = ds->priv;
 	struct hwtstamp_config config;
-	bool rx_on;
-	int rc;
 
 	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
 		return -EFAULT;
@@ -107,26 +82,13 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 
 	switch (config.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		rx_on = false;
+		priv->hwts_rx_en &= ~BIT(port);
 		break;
 	default:
-		rx_on = true;
+		priv->hwts_rx_en |= BIT(port);
 		break;
 	}
 
-	if (rx_on != tagger_data->rxtstamp_get_state(ds)) {
-		tagger_data->rxtstamp_set_state(ds, false);
-
-		rc = sja1105_change_rxtstamping(priv, rx_on);
-		if (rc < 0) {
-			dev_err(ds->dev,
-				"Failed to change RX timestamping: %d\n", rc);
-			return rc;
-		}
-		if (rx_on)
-			tagger_data->rxtstamp_set_state(ds, true);
-	}
-
 	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
 		return -EFAULT;
 	return 0;
@@ -134,7 +96,6 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 
 int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
 {
-	struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
 	struct sja1105_private *priv = ds->priv;
 	struct hwtstamp_config config;
 
@@ -143,7 +104,7 @@ int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
 		config.tx_type = HWTSTAMP_TX_ON;
 	else
 		config.tx_type = HWTSTAMP_TX_OFF;
-	if (tagger_data->rxtstamp_get_state(ds))
+	if (priv->hwts_rx_en & BIT(port))
 		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 	else
 		config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -419,11 +380,10 @@ static long sja1105_rxtstamp_work(struct ptp_clock_info *ptp)
 
 bool sja1105_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
 {
-	struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
 
-	if (!tagger_data->rxtstamp_get_state(ds))
+	if (!(priv->hwts_rx_en & BIT(port)))
 		return false;
 
 	/* We need to read the full PTP clock to reconstruct the Rx
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index 159e43171ccc..c177322f793d 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -48,13 +48,9 @@ struct sja1105_deferred_xmit_work {
 
 /* Global tagger data */
 struct sja1105_tagger_data {
-	/* Tagger to switch */
 	void (*xmit_work_fn)(struct kthread_work *work);
 	void (*meta_tstamp_handler)(struct dsa_switch *ds, int port, u8 ts_id,
 				    enum sja1110_meta_tstamp dir, u64 tstamp);
-	/* Switch to tagger */
-	bool (*rxtstamp_get_state)(struct dsa_switch *ds);
-	void (*rxtstamp_set_state)(struct dsa_switch *ds, bool on);
 };
 
 struct sja1105_skb_cb {
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 92a626a05e82..7f041c8a4d7a 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -58,11 +58,8 @@
 #define SJA1110_TX_TRAILER_LEN			4
 #define SJA1110_MAX_PADDING_LEN			15
 
-#define SJA1105_HWTS_RX_EN			0
-
 struct sja1105_tagger_private {
 	struct sja1105_tagger_data data; /* Must be first */
-	unsigned long state;
 	/* Protects concurrent access to the meta state machine
 	 * from taggers running on multiple ports on SMP systems
 	 */
@@ -392,10 +389,6 @@ static struct sk_buff
 
 		priv = sja1105_tagger_private(ds);
 
-		if (!test_bit(SJA1105_HWTS_RX_EN, &priv->state))
-			/* Do normal processing. */
-			return skb;
-
 		spin_lock(&priv->meta_lock);
 		/* Was this a link-local frame instead of the meta
 		 * that we were expecting?
@@ -431,12 +424,6 @@ static struct sk_buff
 
 		priv = sja1105_tagger_private(ds);
 
-		/* Drop the meta frame if we're not in the right state
-		 * to process it.
-		 */
-		if (!test_bit(SJA1105_HWTS_RX_EN, &priv->state))
-			return NULL;
-
 		spin_lock(&priv->meta_lock);
 
 		stampable_skb = priv->stampable_skb;
@@ -472,30 +459,6 @@ static struct sk_buff
 	return skb;
 }
 
-static bool sja1105_rxtstamp_get_state(struct dsa_switch *ds)
-{
-	struct sja1105_tagger_private *priv = sja1105_tagger_private(ds);
-
-	return test_bit(SJA1105_HWTS_RX_EN, &priv->state);
-}
-
-static void sja1105_rxtstamp_set_state(struct dsa_switch *ds, bool on)
-{
-	struct sja1105_tagger_private *priv = sja1105_tagger_private(ds);
-
-	if (on)
-		set_bit(SJA1105_HWTS_RX_EN, &priv->state);
-	else
-		clear_bit(SJA1105_HWTS_RX_EN, &priv->state);
-
-	/* Initialize the meta state machine to a known state */
-	if (!priv->stampable_skb)
-		return;
-
-	kfree_skb(priv->stampable_skb);
-	priv->stampable_skb = NULL;
-}
-
 static bool sja1105_skb_has_tag_8021q(const struct sk_buff *skb)
 {
 	u16 tpid = ntohs(eth_hdr(skb)->h_proto);
@@ -802,10 +765,6 @@ static int sja1105_connect(struct dsa_switch *ds)
 	}
 
 	priv->xmit_worker = xmit_worker;
-	/* Export functions for switch driver use */
-	tagger_data = &priv->data;
-	tagger_data->rxtstamp_get_state = sja1105_rxtstamp_get_state;
-	tagger_data->rxtstamp_set_state = sja1105_rxtstamp_set_state;
 	ds->tagger_data = priv;
 
 	return 0;
-- 
2.34.1

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

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

[Vladimir Oltean]

On Thu, Jun 29, 2023 at 02:38:35PM +0200, Paolo Abeni wrote:
> [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
>

Ok, I'll formally submit this third patch as a follow-up to net.git once
this 2-patch series gets applied, and see what feedback I get for it.

Re: [PATCH v2 net 0/2] Fix PTP received on wrong port with bridged SJA1105 DSA

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 27 Jun 2023 12:42:05 +0300 you wrote:
> Since the changes were made to tag_8021q to support imprecise RX for
> bridged ports, the tag_sja1105 driver still prefers the source port
> information deduced from the VLAN headers for link-local traffic, even
> though the switch can theoretically do better and report the precise
> source port.
> 
> The problem is that the tagger doesn't know when to trust one source of
> information over another, because the INCL_SRCPT option (to "tag" link
> local frames) is sometimes enabled and sometimes it isn't.
> 
> [...]

Here is the summary with links:
  - [v2,net,1/2] net: dsa: sja1105: always enable the INCL_SRCPT option
    https://git.kernel.org/netdev/net/c/b4638af8885a
  - [v2,net,2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT
    https://git.kernel.org/netdev/net/c/c1ae02d87689

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html