[PATCH net 0/4] Fixes for SJA1105 DSA tc-gate action

[PATCH net 0/4] Fixes for SJA1105 DSA tc-gate action

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This small series fixes 2 bugs in the tc-gate implementation:
1. The TAS state machine keeps getting rescheduled even after removing
   tc-gate actions on all ports.
2. tc-gate actions with only one gate control list entry are installed
   to hardware with an incorrect interval of zero, which makes the
   switch erroneously drop those packets (since the configuration is
   invalid).

To keep the code palatable, a forward-declaration was avoided by moving
some code around in patch 1/4. I hope that isn't too much of an issue.

Vladimir Oltean (4):
  net: dsa: sja1105: move sja1105_compose_gating_subschedule at the top
  net: dsa: sja1105: unconditionally free old gating config
  net: dsa: sja1105: recalculate gating subschedule after deleting
    tc-gate rules
  net: dsa: sja1105: fix tc-gate schedule with single element

 drivers/net/dsa/sja1105/sja1105_vl.c | 327 ++++++++++++++-------------
 1 file changed, 167 insertions(+), 160 deletions(-)

-- 
2.25.1

[PATCH net 1/4] net: dsa: sja1105: move sja1105_compose_gating_subschedule at the top

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

It turns out that sja1105_compose_gating_subschedule must also be called
from sja1105_vl_delete, to recalculate the overall tc-gate
configuration. Currently this is not possible without introducing a
forward declaration. So move the function at the top of the file, along
with its dependencies.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 320 +++++++++++++--------------
 1 file changed, 160 insertions(+), 160 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 0a33c397d833..2b7966714e55 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -7,6 +7,166 @@
 
 #define SJA1105_SIZE_VL_STATUS			8
 
+/* Insert into the global gate list, sorted by gate action time. */
+static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
+				     struct sja1105_rule *rule,
+				     u8 gate_state, s64 entry_time,
+				     struct netlink_ext_ack *extack)
+{
+	struct sja1105_gate_entry *e;
+	int rc;
+
+	e = kzalloc(sizeof(*e), GFP_KERNEL);
+	if (!e)
+		return -ENOMEM;
+
+	e->rule = rule;
+	e->gate_state = gate_state;
+	e->interval = entry_time;
+
+	if (list_empty(&gating_cfg->entries)) {
+		list_add(&e->list, &gating_cfg->entries);
+	} else {
+		struct sja1105_gate_entry *p;
+
+		list_for_each_entry(p, &gating_cfg->entries, list) {
+			if (p->interval == e->interval) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Gate conflict");
+				rc = -EBUSY;
+				goto err;
+			}
+
+			if (e->interval < p->interval)
+				break;
+		}
+		list_add(&e->list, p->list.prev);
+	}
+
+	gating_cfg->num_entries++;
+
+	return 0;
+err:
+	kfree(e);
+	return rc;
+}
+
+/* The gate entries contain absolute times in their e->interval field. Convert
+ * that to proper intervals (i.e. "0, 5, 10, 15" to "5, 5, 5, 5").
+ */
+static void
+sja1105_gating_cfg_time_to_interval(struct sja1105_gating_config *gating_cfg,
+				    u64 cycle_time)
+{
+	struct sja1105_gate_entry *last_e;
+	struct sja1105_gate_entry *e;
+	struct list_head *prev;
+
+	list_for_each_entry(e, &gating_cfg->entries, list) {
+		struct sja1105_gate_entry *p;
+
+		prev = e->list.prev;
+
+		if (prev == &gating_cfg->entries)
+			continue;
+
+		p = list_entry(prev, struct sja1105_gate_entry, list);
+		p->interval = e->interval - p->interval;
+	}
+	last_e = list_last_entry(&gating_cfg->entries,
+				 struct sja1105_gate_entry, list);
+	if (last_e->list.prev != &gating_cfg->entries)
+		last_e->interval = cycle_time - last_e->interval;
+}
+
+static void sja1105_free_gating_config(struct sja1105_gating_config *gating_cfg)
+{
+	struct sja1105_gate_entry *e, *n;
+
+	list_for_each_entry_safe(e, n, &gating_cfg->entries, list) {
+		list_del(&e->list);
+		kfree(e);
+	}
+}
+
+static int sja1105_compose_gating_subschedule(struct sja1105_private *priv,
+					      struct netlink_ext_ack *extack)
+{
+	struct sja1105_gating_config *gating_cfg = &priv->tas_data.gating_cfg;
+	struct sja1105_rule *rule;
+	s64 max_cycle_time = 0;
+	s64 its_base_time = 0;
+	int i, rc = 0;
+
+	list_for_each_entry(rule, &priv->flow_block.rules, list) {
+		if (rule->type != SJA1105_RULE_VL)
+			continue;
+		if (rule->vl.type != SJA1105_VL_TIME_TRIGGERED)
+			continue;
+
+		if (max_cycle_time < rule->vl.cycle_time) {
+			max_cycle_time = rule->vl.cycle_time;
+			its_base_time = rule->vl.base_time;
+		}
+	}
+
+	if (!max_cycle_time)
+		return 0;
+
+	dev_dbg(priv->ds->dev, "max_cycle_time %lld its_base_time %lld\n",
+		max_cycle_time, its_base_time);
+
+	sja1105_free_gating_config(gating_cfg);
+
+	gating_cfg->base_time = its_base_time;
+	gating_cfg->cycle_time = max_cycle_time;
+	gating_cfg->num_entries = 0;
+
+	list_for_each_entry(rule, &priv->flow_block.rules, list) {
+		s64 time;
+		s64 rbt;
+
+		if (rule->type != SJA1105_RULE_VL)
+			continue;
+		if (rule->vl.type != SJA1105_VL_TIME_TRIGGERED)
+			continue;
+
+		/* Calculate the difference between this gating schedule's
+		 * base time, and the base time of the gating schedule with the
+		 * longest cycle time. We call it the relative base time (rbt).
+		 */
+		rbt = future_base_time(rule->vl.base_time, rule->vl.cycle_time,
+				       its_base_time);
+		rbt -= its_base_time;
+
+		time = rbt;
+
+		for (i = 0; i < rule->vl.num_entries; i++) {
+			u8 gate_state = rule->vl.entries[i].gate_state;
+			s64 entry_time = time;
+
+			while (entry_time < max_cycle_time) {
+				rc = sja1105_insert_gate_entry(gating_cfg, rule,
+							       gate_state,
+							       entry_time,
+							       extack);
+				if (rc)
+					goto err;
+
+				entry_time += rule->vl.cycle_time;
+			}
+			time += rule->vl.entries[i].interval;
+		}
+	}
+
+	sja1105_gating_cfg_time_to_interval(gating_cfg, max_cycle_time);
+
+	return 0;
+err:
+	sja1105_free_gating_config(gating_cfg);
+	return rc;
+}
+
 /* The switch flow classification core implements TTEthernet, which 'thinks' in
  * terms of Virtual Links (VL), a concept borrowed from ARINC 664 part 7.
  * However it also has one other operating mode (VLLUPFORMAT=0) where it acts
@@ -397,166 +557,6 @@ int sja1105_vl_delete(struct sja1105_private *priv, int port,
 	return sja1105_static_config_reload(priv, SJA1105_VIRTUAL_LINKS);
 }
 
-/* Insert into the global gate list, sorted by gate action time. */
-static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
-				     struct sja1105_rule *rule,
-				     u8 gate_state, s64 entry_time,
-				     struct netlink_ext_ack *extack)
-{
-	struct sja1105_gate_entry *e;
-	int rc;
-
-	e = kzalloc(sizeof(*e), GFP_KERNEL);
-	if (!e)
-		return -ENOMEM;
-
-	e->rule = rule;
-	e->gate_state = gate_state;
-	e->interval = entry_time;
-
-	if (list_empty(&gating_cfg->entries)) {
-		list_add(&e->list, &gating_cfg->entries);
-	} else {
-		struct sja1105_gate_entry *p;
-
-		list_for_each_entry(p, &gating_cfg->entries, list) {
-			if (p->interval == e->interval) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Gate conflict");
-				rc = -EBUSY;
-				goto err;
-			}
-
-			if (e->interval < p->interval)
-				break;
-		}
-		list_add(&e->list, p->list.prev);
-	}
-
-	gating_cfg->num_entries++;
-
-	return 0;
-err:
-	kfree(e);
-	return rc;
-}
-
-/* The gate entries contain absolute times in their e->interval field. Convert
- * that to proper intervals (i.e. "0, 5, 10, 15" to "5, 5, 5, 5").
- */
-static void
-sja1105_gating_cfg_time_to_interval(struct sja1105_gating_config *gating_cfg,
-				    u64 cycle_time)
-{
-	struct sja1105_gate_entry *last_e;
-	struct sja1105_gate_entry *e;
-	struct list_head *prev;
-
-	list_for_each_entry(e, &gating_cfg->entries, list) {
-		struct sja1105_gate_entry *p;
-
-		prev = e->list.prev;
-
-		if (prev == &gating_cfg->entries)
-			continue;
-
-		p = list_entry(prev, struct sja1105_gate_entry, list);
-		p->interval = e->interval - p->interval;
-	}
-	last_e = list_last_entry(&gating_cfg->entries,
-				 struct sja1105_gate_entry, list);
-	if (last_e->list.prev != &gating_cfg->entries)
-		last_e->interval = cycle_time - last_e->interval;
-}
-
-static void sja1105_free_gating_config(struct sja1105_gating_config *gating_cfg)
-{
-	struct sja1105_gate_entry *e, *n;
-
-	list_for_each_entry_safe(e, n, &gating_cfg->entries, list) {
-		list_del(&e->list);
-		kfree(e);
-	}
-}
-
-static int sja1105_compose_gating_subschedule(struct sja1105_private *priv,
-					      struct netlink_ext_ack *extack)
-{
-	struct sja1105_gating_config *gating_cfg = &priv->tas_data.gating_cfg;
-	struct sja1105_rule *rule;
-	s64 max_cycle_time = 0;
-	s64 its_base_time = 0;
-	int i, rc = 0;
-
-	list_for_each_entry(rule, &priv->flow_block.rules, list) {
-		if (rule->type != SJA1105_RULE_VL)
-			continue;
-		if (rule->vl.type != SJA1105_VL_TIME_TRIGGERED)
-			continue;
-
-		if (max_cycle_time < rule->vl.cycle_time) {
-			max_cycle_time = rule->vl.cycle_time;
-			its_base_time = rule->vl.base_time;
-		}
-	}
-
-	if (!max_cycle_time)
-		return 0;
-
-	dev_dbg(priv->ds->dev, "max_cycle_time %lld its_base_time %lld\n",
-		max_cycle_time, its_base_time);
-
-	sja1105_free_gating_config(gating_cfg);
-
-	gating_cfg->base_time = its_base_time;
-	gating_cfg->cycle_time = max_cycle_time;
-	gating_cfg->num_entries = 0;
-
-	list_for_each_entry(rule, &priv->flow_block.rules, list) {
-		s64 time;
-		s64 rbt;
-
-		if (rule->type != SJA1105_RULE_VL)
-			continue;
-		if (rule->vl.type != SJA1105_VL_TIME_TRIGGERED)
-			continue;
-
-		/* Calculate the difference between this gating schedule's
-		 * base time, and the base time of the gating schedule with the
-		 * longest cycle time. We call it the relative base time (rbt).
-		 */
-		rbt = future_base_time(rule->vl.base_time, rule->vl.cycle_time,
-				       its_base_time);
-		rbt -= its_base_time;
-
-		time = rbt;
-
-		for (i = 0; i < rule->vl.num_entries; i++) {
-			u8 gate_state = rule->vl.entries[i].gate_state;
-			s64 entry_time = time;
-
-			while (entry_time < max_cycle_time) {
-				rc = sja1105_insert_gate_entry(gating_cfg, rule,
-							       gate_state,
-							       entry_time,
-							       extack);
-				if (rc)
-					goto err;
-
-				entry_time += rule->vl.cycle_time;
-			}
-			time += rule->vl.entries[i].interval;
-		}
-	}
-
-	sja1105_gating_cfg_time_to_interval(gating_cfg, max_cycle_time);
-
-	return 0;
-err:
-	sja1105_free_gating_config(gating_cfg);
-	return rc;
-}
-
 int sja1105_vl_gate(struct sja1105_private *priv, int port,
 		    struct netlink_ext_ack *extack, unsigned long cookie,
 		    struct sja1105_key *key, u32 index, s32 prio,
-- 
2.25.1

[PATCH net 2/4] net: dsa: sja1105: unconditionally free old gating config

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Currently sja1105_compose_gating_subschedule is not prepared to be
called for the case where we want to recompute the global tc-gate
configuration after we've deleted those actions on a port.

After deleting the tc-gate actions on the last port, max_cycle_time
would become zero, and that would incorrectly prevent
sja1105_free_gating_config from getting called.

So move the freeing function above the check for the need to apply a new
configuration.

Fixes: 834f8933d5dd ("net: dsa: sja1105: implement tc-gate using time-triggered virtual links")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 2b7966714e55..936b14cbb45d 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -98,6 +98,8 @@ static int sja1105_compose_gating_subschedule(struct sja1105_private *priv,
 	s64 its_base_time = 0;
 	int i, rc = 0;
 
+	sja1105_free_gating_config(gating_cfg);
+
 	list_for_each_entry(rule, &priv->flow_block.rules, list) {
 		if (rule->type != SJA1105_RULE_VL)
 			continue;
@@ -116,8 +118,6 @@ static int sja1105_compose_gating_subschedule(struct sja1105_private *priv,
 	dev_dbg(priv->ds->dev, "max_cycle_time %lld its_base_time %lld\n",
 		max_cycle_time, its_base_time);
 
-	sja1105_free_gating_config(gating_cfg);
-
 	gating_cfg->base_time = its_base_time;
 	gating_cfg->cycle_time = max_cycle_time;
 	gating_cfg->num_entries = 0;
-- 
2.25.1

[PATCH net 3/4] net: dsa: sja1105: recalculate gating subschedule after deleting tc-gate rules

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Currently, tas_data->enabled would remain true even after deleting all
tc-gate rules from the switch ports, which would cause the
sja1105_tas_state_machine to get unnecessarily scheduled.

Also, if there were any errors which would prevent the hardware from
enabling the gating schedule, the sja1105_tas_state_machine would
continuously detect and print that, spamming the kernel log, even if the
rules were subsequently deleted.

The rules themselves are _not_ active, because sja1105_init_scheduling
does enough of a job to not install the gating schedule in the static
config. But the virtual link rules themselves are still present.

So call the functions that remove the tc-gate configuration from
priv->tas_data.gating_cfg, so that tas_data->enabled can be set to
false, and sja1105_tas_state_machine will stop from being scheduled.

Fixes: 834f8933d5dd ("net: dsa: sja1105: implement tc-gate using time-triggered virtual links")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 936b14cbb45d..8524e15fdc4f 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -550,10 +550,18 @@ int sja1105_vl_delete(struct sja1105_private *priv, int port,
 		kfree(rule);
 	}
 
+	rc = sja1105_compose_gating_subschedule(priv, extack);
+	if (rc)
+		return rc;
+
 	rc = sja1105_init_virtual_links(priv, extack);
 	if (rc)
 		return rc;
 
+	rc = sja1105_init_scheduling(priv);
+	if (rc < 0)
+		return rc;
+
 	return sja1105_static_config_reload(priv, SJA1105_VIRTUAL_LINKS);
 }
 
-- 
2.25.1

[PATCH net 4/4] net: dsa: sja1105: fix tc-gate schedule with single element

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The sja1105_gating_cfg_time_to_interval function does this, as per the
comments:

/* The gate entries contain absolute times in their e->interval field. Convert
 * that to proper intervals (i.e. "0, 5, 10, 15" to "5, 5, 5, 5").
 */

To perform that task, it iterates over gating_cfg->entries, at each step
updating the interval of the _previous_ entry. So one interval remains
to be updated at the end of the loop: the last one (since it isn't
"prev" for anyone else).

But there was an erroneous check, that the last element's interval
should not be updated if it's also the only element. I'm not quite sure
why that check was there, but it's clearly incorrect, as a tc-gate
schedule with a single element would get an e->interval of zero,
regardless of the duration requested by the user. The switch wouldn't
even consider this configuration as valid: it will just drop all traffic
that matches the rule.

Fixes: 834f8933d5dd ("net: dsa: sja1105: implement tc-gate using time-triggered virtual links")
Reported-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 8524e15fdc4f..ffc4042b4502 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -75,8 +75,7 @@ sja1105_gating_cfg_time_to_interval(struct sja1105_gating_config *gating_cfg,
 	}
 	last_e = list_last_entry(&gating_cfg->entries,
 				 struct sja1105_gate_entry, list);
-	if (last_e->list.prev != &gating_cfg->entries)
-		last_e->interval = cycle_time - last_e->interval;
+	last_e->interval = cycle_time - last_e->interval;
 }
 
 static void sja1105_free_gating_config(struct sja1105_gating_config *gating_cfg)
-- 
2.25.1

Re: [PATCH net 0/4] Fixes for SJA1105 DSA tc-gate action

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Wed, 24 Jun 2020 16:54:43 +0300

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This small series fixes 2 bugs in the tc-gate implementation:
> 1. The TAS state machine keeps getting rescheduled even after removing
>    tc-gate actions on all ports.
> 2. tc-gate actions with only one gate control list entry are installed
>    to hardware with an incorrect interval of zero, which makes the
>    switch erroneously drop those packets (since the configuration is
>    invalid).
> 
> To keep the code palatable, a forward-declaration was avoided by moving
> some code around in patch 1/4. I hope that isn't too much of an issue.

Series applied, thank you.