netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP
@ 2019-08-04 22:38 Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 1/5] net: dsa: sja1105: Fix broken learning with vlan_filtering disabled Vladimir Oltean
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Vladimir Oltean @ 2019-08-04 22:38 UTC (permalink / raw)
  To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean

This is an assortment of functional fixes for the sja1105 switch driver
targeted for the "net" tree (although they apply on net-next just as
well).

Patch 1/5 ("net: dsa: sja1105: Fix broken learning with vlan_filtering
disabled") repairs a breakage introduced in the early development stages
of the driver: support for traffic from the CPU has broken "normal"
frame forwarding (based on DMAC) - there is connectivity through the
switch only because all frames are flooded.
I debated whether this patch qualifies as a fix, since it puts the
switch into a mode it has never operated in before (aka SVL). But
"normal" forwarding did use to work before the "Traffic support for
SJA1105 DSA driver" patchset, and arguably this patch should have been
part of that.
Also, it would be strange for this feature to be broken in the 5.2 LTS.

Patch 2/5 ("net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as
well") is a simplification of a previous FDB-related patch that is
currently in the 5.3 rc's.

Patches 3/5 - 5/5 fix various crashes found while running linuxptp over the
switch ports for extended periods of time, or in conjunction with other
error conditions. The fixed-up commits were all introduced in 5.2.

Vladimir Oltean (5):
  net: dsa: sja1105: Fix broken learning with vlan_filtering disabled
  net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as well
  net: dsa: sja1105: Really fix panic on unregistering PTP clock
  net: dsa: sja1105: Fix memory leak on meta state machine normal path
  net: dsa: sja1105: Fix memory leak on meta state machine error path

 .../net/dsa/sja1105/sja1105_dynamic_config.c  |  14 +-
 drivers/net/dsa/sja1105/sja1105_main.c        | 140 +++++++-----------
 drivers/net/dsa/sja1105/sja1105_ptp.c         |   7 +-
 net/dsa/tag_sja1105.c                         |  12 +-
 4 files changed, 75 insertions(+), 98 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 1/5] net: dsa: sja1105: Fix broken learning with vlan_filtering disabled
  2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
@ 2019-08-04 22:38 ` Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 2/5] net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as well Vladimir Oltean
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2019-08-04 22:38 UTC (permalink / raw)
  To: f.fainelli, vivien.didelot, andrew, davem
  Cc: netdev, Vladimir Oltean, Georg Waibel

When put under a bridge with vlan_filtering 0, the SJA1105 ports will
flood all traffic as if learning was broken. This is because learning
interferes with the rx_vid's configured by dsa_8021q as unique pvid's.

So learning technically still *does* work, it's just that the learnt
entries never get matched due to their unique VLAN ID.

The setting that saves the day is Shared VLAN Learning, which on this
switch family works exactly as desired: VLAN tagging still works
(untagged traffic gets the correct pvid) and FDB entries are still
populated with the correct contents including VID. Also, a frame cannot
violate the forwarding domain restrictions enforced by its classified
VLAN. It is just that the VID is ignored when looking up the FDB for
taking a forwarding decision (selecting the egress port).

This patch activates SVL, and the result is that frames with a learnt
DMAC are no longer flooded in the scenario described above.

Now exactly *because* SVL works as desired, we have to revisit some
earlier patches:

- It is no longer necessary to manipulate the VID of the 'bridge fdb
  {add,del}' command when vlan_filtering is off. This is because now,
  SVL is enabled for that case, so the actual VID does not matter*.

- It is still desirable to hide dsa_8021q VID's in the FDB dump
  callback. But right now the dump callback should no longer hide
  duplicates (one per each front panel port's pvid, plus one for the
  VLAN that the CPU port is going to tag a TX frame with), because there
  shouldn't be any (the switch will match a single FDB entry no matter
  its VID anyway).

* Not really... It's no longer necessary to transform a 'bridge fdb add'
  into 5 fdb add operations, but the user might still add a fdb entry with
  any vid, and all of them would appear as duplicates in 'bridge fdb
  show'. So force a 'bridge fdb add' to insert the VID of 0**, so that we
  can prune the duplicates at insertion time.

** The VID of 0 is better than 1 because it is always guaranteed to be
   in the ports' hardware filter. DSA also avoids putting the VID inside
   the netlink response message towards the bridge driver when we return
   this particular VID, which makes it suitable for FDB entries learnt
   with vlan_filtering off.

Fixes: 227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through standalone ports")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Georg Waibel <georg.waibel@sensor-technik.de>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 121 +++++++++++--------------
 1 file changed, 55 insertions(+), 66 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 32bf3a7cc3b6..dc6ab834f0cc 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -218,7 +218,7 @@ static int sja1105_init_l2_lookup_params(struct sja1105_private *priv)
 		/* This selects between Independent VLAN Learning (IVL) and
 		 * Shared VLAN Learning (SVL)
 		 */
-		.shared_learn = false,
+		.shared_learn = true,
 		/* Don't discard management traffic based on ENFPORT -
 		 * we don't perform SMAC port enforcement anyway, so
 		 * what we are setting here doesn't matter.
@@ -1089,8 +1089,13 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 	l2_lookup.vlanid = vid;
 	l2_lookup.iotag = SJA1105_S_TAG;
 	l2_lookup.mask_macaddr = GENMASK_ULL(ETH_ALEN * 8 - 1, 0);
-	l2_lookup.mask_vlanid = VLAN_VID_MASK;
-	l2_lookup.mask_iotag = BIT(0);
+	if (dsa_port_is_vlan_filtering(&ds->ports[port])) {
+		l2_lookup.mask_vlanid = VLAN_VID_MASK;
+		l2_lookup.mask_iotag = BIT(0);
+	} else {
+		l2_lookup.mask_vlanid = 0;
+		l2_lookup.mask_iotag = 0;
+	}
 	l2_lookup.destports = BIT(port);
 
 	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
@@ -1147,8 +1152,13 @@ int sja1105pqrs_fdb_del(struct dsa_switch *ds, int port,
 	l2_lookup.vlanid = vid;
 	l2_lookup.iotag = SJA1105_S_TAG;
 	l2_lookup.mask_macaddr = GENMASK_ULL(ETH_ALEN * 8 - 1, 0);
-	l2_lookup.mask_vlanid = VLAN_VID_MASK;
-	l2_lookup.mask_iotag = BIT(0);
+	if (dsa_port_is_vlan_filtering(&ds->ports[port])) {
+		l2_lookup.mask_vlanid = VLAN_VID_MASK;
+		l2_lookup.mask_iotag = BIT(0);
+	} else {
+		l2_lookup.mask_vlanid = 0;
+		l2_lookup.mask_iotag = 0;
+	}
 	l2_lookup.destports = BIT(port);
 
 	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
@@ -1178,60 +1188,31 @@ static int sja1105_fdb_add(struct dsa_switch *ds, int port,
 			   const unsigned char *addr, u16 vid)
 {
 	struct sja1105_private *priv = ds->priv;
-	u16 rx_vid, tx_vid;
-	int rc, i;
 
-	if (dsa_port_is_vlan_filtering(&ds->ports[port]))
-		return priv->info->fdb_add_cmd(ds, port, addr, vid);
-
-	/* Since we make use of VLANs even when the bridge core doesn't tell us
-	 * to, translate these FDB entries into the correct dsa_8021q ones.
-	 * The basic idea (also repeats for removal below) is:
-	 * - Each of the other front-panel ports needs to be able to forward a
-	 *   pvid-tagged (aka tagged with their rx_vid) frame that matches this
-	 *   DMAC.
-	 * - The CPU port (aka the tx_vid of this port) needs to be able to
-	 *   send a frame matching this DMAC to the specified port.
-	 * For a better picture see net/dsa/tag_8021q.c.
+	/* dsa_8021q is in effect when the bridge's vlan_filtering isn't,
+	 * so the switch still does some VLAN processing internally.
+	 * But Shared VLAN Learning (SVL) is also active, and it will take
+	 * care of autonomous forwarding between the unique pvid's of each
+	 * port.  Here we just make sure that users can't add duplicate FDB
+	 * entries when in this mode - the actual VID doesn't matter except
+	 * for what gets printed in 'bridge fdb show'.  In the case of zero,
+	 * no VID gets printed at all.
 	 */
-	for (i = 0; i < SJA1105_NUM_PORTS; i++) {
-		if (i == port)
-			continue;
-		if (i == dsa_upstream_port(priv->ds, port))
-			continue;
+	if (!dsa_port_is_vlan_filtering(&ds->ports[port]))
+		vid = 0;
 
-		rx_vid = dsa_8021q_rx_vid(ds, i);
-		rc = priv->info->fdb_add_cmd(ds, port, addr, rx_vid);
-		if (rc < 0)
-			return rc;
-	}
-	tx_vid = dsa_8021q_tx_vid(ds, port);
-	return priv->info->fdb_add_cmd(ds, port, addr, tx_vid);
+	return priv->info->fdb_add_cmd(ds, port, addr, vid);
 }
 
 static int sja1105_fdb_del(struct dsa_switch *ds, int port,
 			   const unsigned char *addr, u16 vid)
 {
 	struct sja1105_private *priv = ds->priv;
-	u16 rx_vid, tx_vid;
-	int rc, i;
 
-	if (dsa_port_is_vlan_filtering(&ds->ports[port]))
-		return priv->info->fdb_del_cmd(ds, port, addr, vid);
+	if (!dsa_port_is_vlan_filtering(&ds->ports[port]))
+		vid = 0;
 
-	for (i = 0; i < SJA1105_NUM_PORTS; i++) {
-		if (i == port)
-			continue;
-		if (i == dsa_upstream_port(priv->ds, port))
-			continue;
-
-		rx_vid = dsa_8021q_rx_vid(ds, i);
-		rc = priv->info->fdb_del_cmd(ds, port, addr, rx_vid);
-		if (rc < 0)
-			return rc;
-	}
-	tx_vid = dsa_8021q_tx_vid(ds, port);
-	return priv->info->fdb_del_cmd(ds, port, addr, tx_vid);
+	return priv->info->fdb_del_cmd(ds, port, addr, vid);
 }
 
 static int sja1105_fdb_dump(struct dsa_switch *ds, int port,
@@ -1285,24 +1266,9 @@ static int sja1105_fdb_dump(struct dsa_switch *ds, int port,
 			l2_lookup.lockeds = (match >= 0);
 		}
 
-		/* We need to hide the dsa_8021q VLANs from the user. This
-		 * basically means hiding the duplicates and only showing
-		 * the pvid that is supposed to be active in standalone and
-		 * non-vlan_filtering modes (aka 1).
-		 * - For statically added FDB entries (bridge fdb add), we
-		 *   can convert the TX VID (coming from the CPU port) into the
-		 *   pvid and ignore the RX VIDs of the other ports.
-		 * - For dynamically learned FDB entries, a single entry with
-		 *   no duplicates is learned - that which has the real port's
-		 *   pvid, aka RX VID.
-		 */
-		if (!dsa_port_is_vlan_filtering(&ds->ports[port])) {
-			if (l2_lookup.vlanid == tx_vid ||
-			    l2_lookup.vlanid == rx_vid)
-				l2_lookup.vlanid = 1;
-			else
-				continue;
-		}
+		/* We need to hide the dsa_8021q VLANs from the user. */
+		if (!dsa_port_is_vlan_filtering(&ds->ports[port]))
+			l2_lookup.vlanid = 0;
 		cb(macaddr, l2_lookup.vlanid, l2_lookup.lockeds, data);
 	}
 	return 0;
@@ -1594,6 +1560,7 @@ static int sja1105_vlan_prepare(struct dsa_switch *ds, int port,
  */
 static int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled)
 {
+	struct sja1105_l2_lookup_params_entry *l2_lookup_params;
 	struct sja1105_general_params_entry *general_params;
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_table *table;
@@ -1622,6 +1589,28 @@ static int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled)
 	general_params->incl_srcpt1 = enabled;
 	general_params->incl_srcpt0 = enabled;
 
+	/* VLAN filtering => independent VLAN learning.
+	 * No VLAN filtering => shared VLAN learning.
+	 *
+	 * In shared VLAN learning mode, untagged traffic still gets
+	 * pvid-tagged, and the FDB table gets populated with entries
+	 * containing the "real" (pvid or from VLAN tag) VLAN ID.
+	 * However the switch performs a masked L2 lookup in the FDB,
+	 * effectively only looking up a frame's DMAC (and not VID) for the
+	 * forwarding decision.
+	 *
+	 * This is extremely convenient for us, because in modes with
+	 * vlan_filtering=0, dsa_8021q actually installs unique pvid's into
+	 * each front panel port. This is good for identification but breaks
+	 * learning badly - the VID of the learnt FDB entry is unique, aka
+	 * no frames coming from any other port are going to have it. So
+	 * for forwarding purposes, this is as though learning was broken
+	 * (all frames get flooded).
+	 */
+	table = &priv->static_config.tables[BLK_IDX_L2_LOOKUP_PARAMS];
+	l2_lookup_params = table->entries;
+	l2_lookup_params->shared_learn = !enabled;
+
 	rc = sja1105_static_config_reload(priv);
 	if (rc)
 		dev_err(ds->dev, "Failed to change VLAN Ethertype\n");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net 2/5] net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as well
  2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 1/5] net: dsa: sja1105: Fix broken learning with vlan_filtering disabled Vladimir Oltean
@ 2019-08-04 22:38 ` Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 3/5] net: dsa: sja1105: Really fix panic on unregistering PTP clock Vladimir Oltean
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2019-08-04 22:38 UTC (permalink / raw)
  To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean

It looks like the FDB dump taken from first-generation switches also
contains information on whether entries are static or not. So use that
instead of searching through the driver's tables.

Fixes: d763778224ea ("net: dsa: sja1105: Implement is_static for FDB entries on E/T")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_dynamic_config.c | 14 +++++++++++++-
 drivers/net/dsa/sja1105/sja1105_main.c           | 15 ---------------
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_dynamic_config.c b/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
index 6bfb1696a6f2..9988c9d18567 100644
--- a/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
@@ -277,6 +277,18 @@ sja1105et_l2_lookup_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
 			SJA1105ET_SIZE_L2_LOOKUP_ENTRY, op);
 }
 
+static size_t sja1105et_dyn_l2_lookup_entry_packing(void *buf, void *entry_ptr,
+						    enum packing_op op)
+{
+	struct sja1105_l2_lookup_entry *entry = entry_ptr;
+	u8 *cmd = buf + SJA1105ET_SIZE_L2_LOOKUP_ENTRY;
+	const int size = SJA1105_SIZE_DYN_CMD;
+
+	sja1105_packing(cmd, &entry->lockeds, 28, 28, size, op);
+
+	return sja1105et_l2_lookup_entry_packing(buf, entry_ptr, op);
+}
+
 static void
 sja1105et_mgmt_route_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
 				 enum packing_op op)
@@ -477,7 +489,7 @@ sja1105et_general_params_entry_packing(void *buf, void *entry_ptr,
 /* SJA1105E/T: First generation */
 struct sja1105_dynamic_table_ops sja1105et_dyn_ops[BLK_IDX_MAX_DYN] = {
 	[BLK_IDX_L2_LOOKUP] = {
-		.entry_packing = sja1105et_l2_lookup_entry_packing,
+		.entry_packing = sja1105et_dyn_l2_lookup_entry_packing,
 		.cmd_packing = sja1105et_l2_lookup_cmd_packing,
 		.access = (OP_READ | OP_WRITE | OP_DEL),
 		.max_entry_count = SJA1105_MAX_L2_LOOKUP_COUNT,
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index dc6ab834f0cc..fd036bf0a819 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1251,21 +1251,6 @@ static int sja1105_fdb_dump(struct dsa_switch *ds, int port,
 			continue;
 		u64_to_ether_addr(l2_lookup.macaddr, macaddr);
 
-		/* On SJA1105 E/T, the switch doesn't implement the LOCKEDS
-		 * bit, so it doesn't tell us whether a FDB entry is static
-		 * or not.
-		 * But, of course, we can find out - we're the ones who added
-		 * it in the first place.
-		 */
-		if (priv->info->device_id == SJA1105E_DEVICE_ID ||
-		    priv->info->device_id == SJA1105T_DEVICE_ID) {
-			int match;
-
-			match = sja1105_find_static_fdb_entry(priv, port,
-							      &l2_lookup);
-			l2_lookup.lockeds = (match >= 0);
-		}
-
 		/* We need to hide the dsa_8021q VLANs from the user. */
 		if (!dsa_port_is_vlan_filtering(&ds->ports[port]))
 			l2_lookup.vlanid = 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net 3/5] net: dsa: sja1105: Really fix panic on unregistering PTP clock
  2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 1/5] net: dsa: sja1105: Fix broken learning with vlan_filtering disabled Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 2/5] net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as well Vladimir Oltean
@ 2019-08-04 22:38 ` Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 4/5] net: dsa: sja1105: Fix memory leak on meta state machine normal path Vladimir Oltean
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2019-08-04 22:38 UTC (permalink / raw)
  To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean

The IS_ERR_OR_NULL(priv->clock) check inside
sja1105_ptp_clock_unregister() is preventing cancel_delayed_work_sync
from actually being run.

Additionally, sja1105_ptp_clock_unregister() does not actually get run,
when placed in sja1105_remove(). The DSA switch gets torn down, but the
sja1105 module does not get unregistered. So sja1105_ptp_clock_unregister
needs to be moved to sja1105_teardown, to be symmetrical with
sja1105_ptp_clock_register which is called from the DSA sja1105_setup.

It is strange to fix a "fixes" patch, but the probe failure can only be
seen when the attached PHY does not respond to MDIO (issue which I can't
pinpoint the reason to) and it goes away after I power-cycle the board.
This time the patch was validated on a failing board, and the kernel
panic from the fixed commit's message can no longer be seen.

Fixes: 29dd908d355f ("net: dsa: sja1105: Cancel PTP delayed work on unregister")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 4 ++--
 drivers/net/dsa/sja1105/sja1105_ptp.c  | 7 +++----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index fd036bf0a819..13c22f51d476 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1725,6 +1725,8 @@ static void sja1105_teardown(struct dsa_switch *ds)
 
 	cancel_work_sync(&priv->tagger_data.rxtstamp_work);
 	skb_queue_purge(&priv->tagger_data.skb_rxtstamp_queue);
+	sja1105_ptp_clock_unregister(priv);
+	sja1105_static_config_free(&priv->static_config);
 }
 
 static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
@@ -2182,9 +2184,7 @@ static int sja1105_remove(struct spi_device *spi)
 {
 	struct sja1105_private *priv = spi_get_drvdata(spi);
 
-	sja1105_ptp_clock_unregister(priv);
 	dsa_unregister_switch(priv->ds);
-	sja1105_static_config_free(&priv->static_config);
 	return 0;
 }
 
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index d19cfdf681af..d8e8dd59f3d1 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -369,16 +369,15 @@ int sja1105_ptp_clock_register(struct sja1105_private *priv)
 		.mult = SJA1105_CC_MULT,
 	};
 	mutex_init(&priv->ptp_lock);
-	INIT_DELAYED_WORK(&priv->refresh_work, sja1105_ptp_overflow_check);
-
-	schedule_delayed_work(&priv->refresh_work, SJA1105_REFRESH_INTERVAL);
-
 	priv->ptp_caps = sja1105_ptp_caps;
 
 	priv->clock = ptp_clock_register(&priv->ptp_caps, ds->dev);
 	if (IS_ERR_OR_NULL(priv->clock))
 		return PTR_ERR(priv->clock);
 
+	INIT_DELAYED_WORK(&priv->refresh_work, sja1105_ptp_overflow_check);
+	schedule_delayed_work(&priv->refresh_work, SJA1105_REFRESH_INTERVAL);
+
 	return sja1105_ptp_reset(priv);
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net 4/5] net: dsa: sja1105: Fix memory leak on meta state machine normal path
  2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
                   ` (2 preceding siblings ...)
  2019-08-04 22:38 ` [PATCH net 3/5] net: dsa: sja1105: Really fix panic on unregistering PTP clock Vladimir Oltean
@ 2019-08-04 22:38 ` Vladimir Oltean
  2019-08-04 22:38 ` [PATCH net 5/5] net: dsa: sja1105: Fix memory leak on meta state machine error path Vladimir Oltean
  2019-08-06 21:38 ` [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP David Miller
  5 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2019-08-04 22:38 UTC (permalink / raw)
  To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean

After a meta frame is received, it is associated with the cached
sp->data->stampable_skb from the DSA tagger private structure.

Cached means its refcount is incremented with skb_get() in order for
dsa_switch_rcv() to not free it when the tagger .rcv returns NULL.

The mistake is that skb_unref() is not the correct function to use. It
will correctly decrement the refcount (which will go back to zero) but
the skb memory will not be freed.  That is the job of kfree_skb(), which
also calls skb_unref().

But it turns out that freeing the cached stampable_skb is in fact not
necessary.  It is still a perfectly valid skb, and now it is even
annotated with the partial RX timestamp.  So remove the skb_copy()
altogether and simply pass the stampable_skb with a refcount of 1
(incremented by us, decremented by dsa_switch_rcv) up the stack.

Fixes: f3097be21bf1 ("net: dsa: sja1105: Add a state machine for RX timestamping")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/tag_sja1105.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 26363d72d25b..8fa8dda8a15b 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -211,17 +211,8 @@ static struct sk_buff
 		 * for further processing up the network stack.
 		 */
 		kfree_skb(skb);
-
-		skb = skb_copy(stampable_skb, GFP_ATOMIC);
-		if (!skb) {
-			dev_err_ratelimited(dp->ds->dev,
-					    "Failed to copy stampable skb\n");
-			spin_unlock(&sp->data->meta_lock);
-			return NULL;
-		}
+		skb = stampable_skb;
 		sja1105_transfer_meta(skb, meta);
-		/* The cached copy will be freed now */
-		skb_unref(stampable_skb);
 
 		spin_unlock(&sp->data->meta_lock);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net 5/5] net: dsa: sja1105: Fix memory leak on meta state machine error path
  2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
                   ` (3 preceding siblings ...)
  2019-08-04 22:38 ` [PATCH net 4/5] net: dsa: sja1105: Fix memory leak on meta state machine normal path Vladimir Oltean
@ 2019-08-04 22:38 ` Vladimir Oltean
  2019-08-06 21:38 ` [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP David Miller
  5 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2019-08-04 22:38 UTC (permalink / raw)
  To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean

When RX timestamping is enabled and two link-local (non-meta) frames are
received in a row, this constitutes an error.

The tagger is always caching the last link-local frame, in an attempt to
merge it with the meta follow-up frame when that arrives. To recover
from the above error condition, the initial cached link-local frame is
dropped and the second frame in a row is cached (in expectance of the
second meta frame).

However, when dropping the initial link-local frame, its backing memory
was being leaked.

Fixes: f3097be21bf1 ("net: dsa: sja1105: Add a state machine for RX timestamping")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/tag_sja1105.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 8fa8dda8a15b..47ee88163a9d 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -165,6 +165,7 @@ static struct sk_buff
 					    "Expected meta frame, is %12llx "
 					    "in the DSA master multicast filter?\n",
 					    SJA1105_META_DMAC);
+			kfree_skb(sp->data->stampable_skb);
 		}
 
 		/* Hold a reference to avoid dsa_switch_rcv
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP
  2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
                   ` (4 preceding siblings ...)
  2019-08-04 22:38 ` [PATCH net 5/5] net: dsa: sja1105: Fix memory leak on meta state machine error path Vladimir Oltean
@ 2019-08-06 21:38 ` David Miller
  5 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2019-08-06 21:38 UTC (permalink / raw)
  To: olteanv; +Cc: f.fainelli, vivien.didelot, andrew, netdev

From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon,  5 Aug 2019 01:38:43 +0300

> This is an assortment of functional fixes for the sja1105 switch driver
> targeted for the "net" tree (although they apply on net-next just as
> well).
> 
> Patch 1/5 ("net: dsa: sja1105: Fix broken learning with vlan_filtering
> disabled") repairs a breakage introduced in the early development stages
> of the driver: support for traffic from the CPU has broken "normal"
> frame forwarding (based on DMAC) - there is connectivity through the
> switch only because all frames are flooded.
> I debated whether this patch qualifies as a fix, since it puts the
> switch into a mode it has never operated in before (aka SVL). But
> "normal" forwarding did use to work before the "Traffic support for
> SJA1105 DSA driver" patchset, and arguably this patch should have been
> part of that.
> Also, it would be strange for this feature to be broken in the 5.2 LTS.
> 
> Patch 2/5 ("net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as
> well") is a simplification of a previous FDB-related patch that is
> currently in the 5.3 rc's.
> 
> Patches 3/5 - 5/5 fix various crashes found while running linuxptp over the
> switch ports for extended periods of time, or in conjunction with other
> error conditions. The fixed-up commits were all introduced in 5.2.

Series applied.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-08-06 21:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-04 22:38 [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP Vladimir Oltean
2019-08-04 22:38 ` [PATCH net 1/5] net: dsa: sja1105: Fix broken learning with vlan_filtering disabled Vladimir Oltean
2019-08-04 22:38 ` [PATCH net 2/5] net: dsa: sja1105: Use the LOCKEDS bit for SJA1105 E/T as well Vladimir Oltean
2019-08-04 22:38 ` [PATCH net 3/5] net: dsa: sja1105: Really fix panic on unregistering PTP clock Vladimir Oltean
2019-08-04 22:38 ` [PATCH net 4/5] net: dsa: sja1105: Fix memory leak on meta state machine normal path Vladimir Oltean
2019-08-04 22:38 ` [PATCH net 5/5] net: dsa: sja1105: Fix memory leak on meta state machine error path Vladimir Oltean
2019-08-06 21:38 ` [PATCH net 0/5] Fixes for SJA1105 DSA: FDBs, Learning and PTP David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).