LinuxLists.cc - [PATCH net-next 0/7] Ocelot/Felix driver support for preemptible traffic classes

[permalink] [raw]

Subject: [PATCH net-next 2/7] net: mscc: ocelot: remove struct ocelot_mm_state :: lock

Unfortunately, the workarounds for the hardware bugs make it pointless
to keep fine-grained locking for the MAC Merge state of each port.

Our vsc9959_cut_through_fwd() implementation requires
ocelot->fwd_domain_lock to be held, in order to serialize with changes
to the bridging domains and to port speed changes (which affect which
ports can be cut-through). Simultaneously, the traffic classes which can
be cut-through cannot be preemptible at the same time, and this will
depend on the MAC Merge layer state (which changes from threaded
interrupt context).

Since vsc9959_cut_through_fwd() would have to hold the mm->lock of all
ports for a correct and race-free implementation with respect to
ocelot_mm_irq(), in practice it means that any time a port's mm->lock is
held, it would potentially block holders of ocelot->fwd_domain_lock.

In the interest of simple locking rules, make all MAC Merge layer state
changes (and preemptible traffic class changes) be serialized by the
ocelot->fwd_domain_lock.

Signed-off-by: Vladimir Oltean <[email protected]>
---
Diff: patch is new.

drivers/net/ethernet/mscc/ocelot_mm.c | 20 ++++++++------------
include/soc/mscc/ocelot.h | 1 -
2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index ddaf1fb05e48..d2df47e6f8f6 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -56,8 +56,6 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)
enum ethtool_mm_verify_status verify_status;
u32 val;

- mutex_lock(&mm->lock);
-
val = ocelot_port_readl(ocelot_port, DEV_MM_STATUS);

verify_status = ocelot_mm_verify_status(val);
@@ -88,16 +86,18 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)
}

ocelot_port_writel(ocelot_port, val, DEV_MM_STATUS);
-
- mutex_unlock(&mm->lock);
}

void ocelot_mm_irq(struct ocelot *ocelot)
{
int port;

+ mutex_lock(&ocelot->fwd_domain_lock);
+
for (port = 0; port < ocelot->num_phys_ports; port++)
ocelot_mm_update_port_status(ocelot, port);
+
+ mutex_unlock(&ocelot->fwd_domain_lock);
}
EXPORT_SYMBOL_GPL(ocelot_mm_irq);

@@ -107,14 +107,11 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port,
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
u32 mm_enable = 0, verify_disable = 0, add_frag_size;
- struct ocelot_mm_state *mm;
int err;

if (!ocelot->mm_supported)
return -EOPNOTSUPP;

- mm = &ocelot->mm[port];
-
err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size,
&add_frag_size, extack);
if (err)
@@ -129,7 +126,7 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port,
if (!cfg->verify_enabled)
verify_disable = DEV_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_DIS;

- mutex_lock(&mm->lock);
+ mutex_lock(&ocelot->fwd_domain_lock);

ocelot_port_rmwl(ocelot_port, mm_enable,
DEV_MM_CONFIG_ENABLE_CONFIG_MM_TX_ENA |
@@ -148,7 +145,7 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port,
QSYS_PREEMPTION_CFG,
port);

- mutex_unlock(&mm->lock);
+ mutex_unlock(&ocelot->fwd_domain_lock);

return 0;
}
@@ -166,7 +163,7 @@ int ocelot_port_get_mm(struct ocelot *ocelot, int port,

mm = &ocelot->mm[port];

- mutex_lock(&mm->lock);
+ mutex_lock(&ocelot->fwd_domain_lock);

val = ocelot_port_readl(ocelot_port, DEV_MM_ENABLE_CONFIG);
state->pmac_enabled = !!(val & DEV_MM_CONFIG_ENABLE_CONFIG_MM_RX_ENA);
@@ -185,7 +182,7 @@ int ocelot_port_get_mm(struct ocelot *ocelot, int port,
state->verify_status = mm->verify_status;
state->tx_active = mm->tx_active;

- mutex_unlock(&mm->lock);
+ mutex_unlock(&ocelot->fwd_domain_lock);

return 0;
}
@@ -209,7 +206,6 @@ int ocelot_mm_init(struct ocelot *ocelot)
u32 val;

mm = &ocelot->mm[port];
- mutex_init(&mm->lock);
ocelot_port = ocelot->ports[port];

/* Update initial status variable for the
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index eb8e3935375d..9599be6a0a39 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -744,7 +744,6 @@ struct ocelot_mirror {
};

struct ocelot_mm_state {
- struct mutex lock;
enum ethtool_mm_verify_status verify_status;
bool tx_active;
};
--
2.34.1

2023-04-15 17:07:15

[permalink] [raw]

Subject: [PATCH net-next 1/7] net: mscc: ocelot: export a single ocelot_mm_irq()

When the switch emits an IRQ, we don't know what caused it, and we
iterate through all ports to check the MAC Merge status.

Move that iteration inside the ocelot lib; we will change the locking in
a future change and it would be good to encapsulate that lock completely
within the ocelot lib.

Signed-off-by: Vladimir Oltean <[email protected]>
---
Diff: patch is new.

drivers/net/dsa/ocelot/felix_vsc9959.c | 5 +----
drivers/net/ethernet/mscc/ocelot_mm.c | 12 ++++++++++--
include/soc/mscc/ocelot.h | 2 +-
3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index dddb28984bdf..478893c06f56 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2610,12 +2610,9 @@ static const struct felix_info felix_info_vsc9959 = {
static irqreturn_t felix_irq_handler(int irq, void *data)
{
struct ocelot *ocelot = (struct ocelot *)data;
- int port;

ocelot_get_txtstamp(ocelot);
-
- for (port = 0; port < ocelot->num_phys_ports; port++)
- ocelot_port_mm_irq(ocelot, port);
+ ocelot_mm_irq(ocelot);

return IRQ_HANDLED;
}
diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index 0a8f21ae23f0..ddaf1fb05e48 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -49,7 +49,7 @@ static enum ethtool_mm_verify_status ocelot_mm_verify_status(u32 val)
}
}

-void ocelot_port_mm_irq(struct ocelot *ocelot, int port)
+static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
struct ocelot_mm_state *mm = &ocelot->mm[port];
@@ -91,7 +91,15 @@ void ocelot_port_mm_irq(struct ocelot *ocelot, int port)

mutex_unlock(&mm->lock);
}
-EXPORT_SYMBOL_GPL(ocelot_port_mm_irq);
+
+void ocelot_mm_irq(struct ocelot *ocelot)
+{
+ int port;
+
+ for (port = 0; port < ocelot->num_phys_ports; port++)
+ ocelot_mm_update_port_status(ocelot, port);
+}
+EXPORT_SYMBOL_GPL(ocelot_mm_irq);

int ocelot_port_set_mm(struct ocelot *ocelot, int port,
struct ethtool_mm_cfg *cfg,
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 277e6d1f2096..eb8e3935375d 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -1148,7 +1148,7 @@ int ocelot_vcap_policer_add(struct ocelot *ocelot, u32 pol_ix,
struct ocelot_policer *pol);
int ocelot_vcap_policer_del(struct ocelot *ocelot, u32 pol_ix);

-void ocelot_port_mm_irq(struct ocelot *ocelot, int port);
+void ocelot_mm_irq(struct ocelot *ocelot);
int ocelot_port_set_mm(struct ocelot *ocelot, int port,
struct ethtool_mm_cfg *cfg,
struct netlink_ext_ack *extack);
--
2.34.1

2023-04-15 17:07:17

[permalink] [raw]

Subject: [PATCH net-next 3/7] net: mscc: ocelot: optimize ocelot_mm_irq()

The MAC Merge IRQ of all ports is shared with the PTP TX timestamp IRQ
of all ports, which means that currently, when a PTP TX timestamp is
generated, felix_irq_handler() also polls for the MAC Merge layer status
of all ports, looking for changes. This makes the kernel do more work,
and under certain circumstances may make ptp4l require a
tx_timestamp_timeout argument higher than before.

Changes to the MAC Merge layer status are only to be expected under
certain conditions - its TX direction needs to be enabled - so we can
check early if that is the case, and omit register access otherwise.

Make ocelot_mm_update_port_status() skip register access if
mm->tx_enabled is unset, and also call it once more, outside IRQ
context, from ocelot_port_set_mm(), when mm->tx_enabled transitions from
true to false, because an IRQ is also expected in that case.

Also, a port may have its MAC Merge layer enabled but it may not have
generated the interrupt. In that case, there's no point in writing to
DEV_MM_STATUS to acknowledge that IRQ. We can reduce the number of
register writes per port with MM enabled by keeping an "ack" variable
which writes the "write-one-to-clear" bits. Those are 3 in number:
PRMPT_ACTIVE_STICKY, UNEXP_RX_PFRM_STICKY and UNEXP_TX_PFRM_STICKY.
The other fields in DEV_MM_STATUS are read-only and it doesn't matter
what is written to them, so writing zero is just fine.

Signed-off-by: Vladimir Oltean <[email protected]>
---
Diff: patch is new.

drivers/net/ethernet/mscc/ocelot_mm.c | 30 +++++++++++++++++++++++++--
include/soc/mscc/ocelot.h | 1 +
2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index d2df47e6f8f6..ce6429d46814 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -54,7 +54,10 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)
struct ocelot_port *ocelot_port = ocelot->ports[port];
struct ocelot_mm_state *mm = &ocelot->mm[port];
enum ethtool_mm_verify_status verify_status;
- u32 val;
+ u32 val, ack = 0;
+
+ if (!mm->tx_enabled)
+ return;

val = ocelot_port_readl(ocelot_port, DEV_MM_STATUS);

@@ -71,21 +74,28 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)

dev_dbg(ocelot->dev, "Port %d TX preemption %s\n",
port, mm->tx_active ? "active" : "inactive");
+
+ ack |= DEV_MM_STAT_MM_STATUS_PRMPT_ACTIVE_STICKY;
}

if (val & DEV_MM_STAT_MM_STATUS_UNEXP_RX_PFRM_STICKY) {
dev_err(ocelot->dev,
"Unexpected P-frame received on port %d while verification was unsuccessful or not yet verified\n",
port);
+
+ ack |= DEV_MM_STAT_MM_STATUS_UNEXP_RX_PFRM_STICKY;
}

if (val & DEV_MM_STAT_MM_STATUS_UNEXP_TX_PFRM_STICKY) {
dev_err(ocelot->dev,
"Unexpected P-frame requested to be transmitted on port %d while verification was unsuccessful or not yet verified, or MM_TX_ENA=0\n",
port);
+
+ ack |= DEV_MM_STAT_MM_STATUS_UNEXP_TX_PFRM_STICKY;
}

- ocelot_port_writel(ocelot_port, val, DEV_MM_STATUS);
+ if (ack)
+ ocelot_port_writel(ocelot_port, ack, DEV_MM_STATUS);
}

void ocelot_mm_irq(struct ocelot *ocelot)
@@ -107,11 +117,14 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port,
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
u32 mm_enable = 0, verify_disable = 0, add_frag_size;
+ struct ocelot_mm_state *mm;
int err;

if (!ocelot->mm_supported)
return -EOPNOTSUPP;

+ mm = &ocelot->mm[port];
+
err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size,
&add_frag_size, extack);
if (err)
@@ -145,6 +158,19 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port,
QSYS_PREEMPTION_CFG,
port);

+ /* The switch will emit an IRQ when TX is disabled, to notify that it
+ * has become inactive. We optimize ocelot_mm_update_port_status() to
+ * not bother processing MM IRQs at all for ports with TX disabled,
+ * but we need to ACK this IRQ now, while mm->tx_enabled is still set,
+ * otherwise we get an IRQ storm.
+ */
+ if (mm->tx_enabled && !cfg->tx_enabled) {
+ ocelot_mm_update_port_status(ocelot, port);
+ WARN_ON(mm->tx_active);
+ }
+
+ mm->tx_enabled = cfg->tx_enabled;
+
mutex_unlock(&ocelot->fwd_domain_lock);

return 0;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 9599be6a0a39..ee8d43dc5c06 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -745,6 +745,7 @@ struct ocelot_mirror {

struct ocelot_mm_state {
enum ethtool_mm_verify_status verify_status;
+ bool tx_enabled;
bool tx_active;
};

--
2.34.1

2023-04-15 17:07:31

[permalink] [raw]

Subject: [PATCH net-next 4/7] net: mscc: ocelot: don't rely on cached verify_status in ocelot_port_get_mm()

ocelot_mm_update_port_status() updates mm->verify_status, but when the
verification state of a port changes, an IRQ isn't emitted, but rather,
only when the verification state reaches one of the final states (like
DISABLED, FAILED, SUCCEEDED) - things that would affect mm->tx_active,
which is what the IRQ *is* actually emitted for.

That is to say, user space may miss reports of an intermediary MAC Merge
verification state (like from INITIAL to VERIFYING), unless there was an
IRQ notifying the driver of the change in mm->tx_active as well.

This is not a huge deal, but for reliable reporting to user space, let's
call ocelot_mm_update_port_status() synchronously from
ocelot_port_get_mm(), which makes user space see the current MM status.

Signed-off-by: Vladimir Oltean <[email protected]>
---
Diff: patch is new.

drivers/net/ethernet/mscc/ocelot_mm.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index ce6429d46814..3e458f72f645 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -205,6 +205,7 @@ int ocelot_port_get_mm(struct ocelot *ocelot, int port,
state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(add_frag_size);
state->rx_min_frag_size = ETH_ZLEN;

+ ocelot_mm_update_port_status(ocelot, port);
state->verify_status = mm->verify_status;
state->tx_active = mm->tx_active;

--
2.34.1

2023-04-15 17:07:39

[permalink] [raw]

Subject: [PATCH net-next 5/7] net: mscc: ocelot: add support for mqprio offload

This doesn't apply anything to hardware and in general doesn't do
anything that the software variant doesn't do, except for checking that
there isn't more than 1 TXQ per TC (TXQs for a DSA switch are a dubious
concept anyway). The reason we add this is to be able to parse one more
field added to struct tc_mqprio_qopt_offload, namely preemptible_tcs.

Signed-off-by: Vladimir Oltean <[email protected]>
Reviewed-by: Ferenc Fejes <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
---
Diff vs
https://lore.kernel.org/netdev/[email protected]/:
none.

drivers/net/dsa/ocelot/felix_vsc9959.c | 9 +++++
drivers/net/ethernet/mscc/ocelot.c | 50 ++++++++++++++++++++++++++
include/soc/mscc/ocelot.h | 4 +++
3 files changed, 63 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 478893c06f56..66ec2740e3cb 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1612,6 +1612,13 @@ static int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
static int vsc9959_qos_query_caps(struct tc_query_caps_base *base)
{
switch (base->type) {
+ case TC_SETUP_QDISC_MQPRIO: {
+ struct tc_mqprio_caps *caps = base->caps;
+
+ caps->validate_queue_counts = true;
+
+ return 0;
+ }
case TC_SETUP_QDISC_TAPRIO: {
struct tc_taprio_caps *caps = base->caps;

@@ -1635,6 +1642,8 @@ static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port,
return vsc9959_qos_query_caps(type_data);
case TC_SETUP_QDISC_TAPRIO:
return vsc9959_qos_port_tas_set(ocelot, port, type_data);
+ case TC_SETUP_QDISC_MQPRIO:
+ return ocelot_port_mqprio(ocelot, port, type_data);
case TC_SETUP_QDISC_CBS:
return vsc9959_qos_port_cbs_set(ds, port, type_data);
default:
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 1502bb2c8ea7..8dc5fb1bc61b 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -8,6 +8,7 @@
#include <linux/if_bridge.h>
#include <linux/iopoll.h>
#include <linux/phy/phy.h>
+#include <net/pkt_sched.h>
#include <soc/mscc/ocelot_hsio.h>
#include <soc/mscc/ocelot_vcap.h>
#include "ocelot.h"
@@ -2699,6 +2700,55 @@ void ocelot_port_mirror_del(struct ocelot *ocelot, int from, bool ingress)
}
EXPORT_SYMBOL_GPL(ocelot_port_mirror_del);

+static void ocelot_port_reset_mqprio(struct ocelot *ocelot, int port)
+{
+ struct net_device *dev = ocelot->ops->port_to_netdev(ocelot, port);
+
+ netdev_reset_tc(dev);
+}
+
+int ocelot_port_mqprio(struct ocelot *ocelot, int port,
+ struct tc_mqprio_qopt_offload *mqprio)
+{
+ struct net_device *dev = ocelot->ops->port_to_netdev(ocelot, port);
+ struct netlink_ext_ack *extack = mqprio->extack;
+ struct tc_mqprio_qopt *qopt = &mqprio->qopt;
+ int num_tc = qopt->num_tc;
+ int tc, err;
+
+ if (!num_tc) {
+ ocelot_port_reset_mqprio(ocelot, port);
+ return 0;
+ }
+
+ err = netdev_set_num_tc(dev, num_tc);
+ if (err)
+ return err;
+
+ for (tc = 0; tc < num_tc; tc++) {
+ if (qopt->count[tc] != 1) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Only one TXQ per TC supported");
+ return -EINVAL;
+ }
+
+ err = netdev_set_tc_queue(dev, tc, 1, qopt->offset[tc]);
+ if (err)
+ goto err_reset_tc;
+ }
+
+ err = netif_set_real_num_tx_queues(dev, num_tc);
+ if (err)
+ goto err_reset_tc;
+
+ return 0;
+
+err_reset_tc:
+ ocelot_port_reset_mqprio(ocelot, port);
+ return err;
+}
+EXPORT_SYMBOL_GPL(ocelot_port_mqprio);
+
void ocelot_init_port(struct ocelot *ocelot, int port)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index ee8d43dc5c06..9596c79e9223 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -11,6 +11,8 @@
#include <linux/regmap.h>
#include <net/dsa.h>

+struct tc_mqprio_qopt_offload;
+
/* Port Group IDs (PGID) are masks of destination ports.
*
* For L2 forwarding, the switch performs 3 lookups in the PGID table for each
@@ -1154,6 +1156,8 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port,
struct netlink_ext_ack *extack);
int ocelot_port_get_mm(struct ocelot *ocelot, int port,
struct ethtool_mm_state *state);
+int ocelot_port_mqprio(struct ocelot *ocelot, int port,
+ struct tc_mqprio_qopt_offload *mqprio);

#if IS_ENABLED(CONFIG_BRIDGE_MRP)
int ocelot_mrp_add(struct ocelot *ocelot, int port,
--
2.34.1

2023-04-15 17:08:23

[permalink] [raw]

Subject: [PATCH net-next 7/7] net: mscc: ocelot: add support for preemptible traffic classes

In order to not transmit (preemptible) frames which will be received by
the link partner as corrupted (because it doesn't support FP), the
hardware requires the driver to program the QSYS_PREEMPTION_CFG_P_QUEUES
register only after the MAC Merge layer becomes active (verification
succeeds, or was disabled).

There are some cases when FP is known (through experimentation) to be
broken. Give priority to FP over cut-through switching, and disable FP
for known broken link modes.

Signed-off-by: Vladimir Oltean <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
---
Diff vs
https://lore.kernel.org/netdev/[email protected]/:
- keep track of active_preemptible_tcs separately from preemptible_tcs
- simplified locking, now using just ocelot->fwd_domain_lock
- updating active preemptable TCs directly based on mm->tx_active rather
than based on verification state
- added some debugging prints

drivers/net/dsa/ocelot/felix_vsc9959.c | 7 +++-
drivers/net/ethernet/mscc/ocelot.c | 10 ++++-
drivers/net/ethernet/mscc/ocelot.h | 3 ++
drivers/net/ethernet/mscc/ocelot_mm.c | 54 ++++++++++++++++++++++++++
include/soc/mscc/ocelot.h | 3 ++
5 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index e055b3980ccc..cfb3faeaa5bf 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2519,6 +2519,7 @@ static void vsc9959_cut_through_fwd(struct ocelot *ocelot)

for (port = 0; port < ocelot->num_phys_ports; port++) {
struct ocelot_port *ocelot_port = ocelot->ports[port];
+ struct ocelot_mm_state *mm = &ocelot->mm[port];
int min_speed = ocelot_port->speed;
unsigned long mask = 0;
u32 tmp, val = 0;
@@ -2559,10 +2560,12 @@ static void vsc9959_cut_through_fwd(struct ocelot *ocelot)

/* Enable cut-through forwarding for all traffic classes that
* don't have oversized dropping enabled, since this check is
- * bypassed in cut-through mode.
+ * bypassed in cut-through mode. Also exclude preemptible
+ * traffic classes, since these would hang the port for some
+ * reason, if sent as cut-through.
*/
if (ocelot_port->speed == min_speed) {
- val = GENMASK(7, 0);
+ val = GENMASK(7, 0) & ~mm->active_preemptible_tcs;

for (tc = 0; tc < OCELOT_NUM_TC; tc++)
if (vsc9959_port_qmaxsdu_get(ocelot, port, tc))
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 8dc5fb1bc61b..1f5f00b30441 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1006,7 +1006,12 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
*/
if (ocelot->ops->cut_through_fwd) {
mutex_lock(&ocelot->fwd_domain_lock);
- ocelot->ops->cut_through_fwd(ocelot);
+ /* Workaround for hardware bug - FP doesn't work
+ * at all link speeds for all PHY modes. The function
+ * below also calls ocelot->ops->cut_through_fwd(),
+ * so we don't need to do it twice.
+ */
+ ocelot_port_update_active_preemptible_tcs(ocelot, port);
mutex_unlock(&ocelot->fwd_domain_lock);
}

@@ -2705,6 +2710,7 @@ static void ocelot_port_reset_mqprio(struct ocelot *ocelot, int port)
struct net_device *dev = ocelot->ops->port_to_netdev(ocelot, port);

netdev_reset_tc(dev);
+ ocelot_port_change_fp(ocelot, port, 0);
}

int ocelot_port_mqprio(struct ocelot *ocelot, int port,
@@ -2741,6 +2747,8 @@ int ocelot_port_mqprio(struct ocelot *ocelot, int port,
if (err)
goto err_reset_tc;

+ ocelot_port_change_fp(ocelot, port, mqprio->preemptible_tcs);
+
return 0;

err_reset_tc:
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index d920ca930690..14440a3b04c3 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -119,6 +119,9 @@ int ocelot_stats_init(struct ocelot *ocelot);
void ocelot_stats_deinit(struct ocelot *ocelot);

int ocelot_mm_init(struct ocelot *ocelot);
+void ocelot_port_change_fp(struct ocelot *ocelot, int port,
+ unsigned long preemptible_tcs);
+void ocelot_port_update_active_preemptible_tcs(struct ocelot *ocelot, int port);

extern struct notifier_block ocelot_netdevice_nb;
extern struct notifier_block ocelot_switchdev_nb;
diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index 3e458f72f645..fb3145118d68 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -49,6 +49,59 @@ static enum ethtool_mm_verify_status ocelot_mm_verify_status(u32 val)
}
}

+void ocelot_port_update_active_preemptible_tcs(struct ocelot *ocelot, int port)
+{
+ struct ocelot_port *ocelot_port = ocelot->ports[port];
+ struct ocelot_mm_state *mm = &ocelot->mm[port];
+ u32 val = 0;
+
+ lockdep_assert_held(&ocelot->fwd_domain_lock);
+
+ /* Only commit preemptible TCs when MAC Merge is active.
+ * On NXP LS1028A, when using QSGMII, the port hangs if transmitting
+ * preemptible frames at any other link speed than gigabit, so avoid
+ * preemption at lower speeds in this PHY mode.
+ */
+ if ((ocelot_port->phy_mode != PHY_INTERFACE_MODE_QSGMII ||
+ ocelot_port->speed == SPEED_1000) && mm->tx_active)
+ val = mm->preemptible_tcs;
+
+ /* Cut through switching doesn't work for preemptible priorities,
+ * so first make sure it is disabled.
+ */
+ mm->active_preemptible_tcs = val;
+ ocelot->ops->cut_through_fwd(ocelot);
+
+ dev_dbg(ocelot->dev,
+ "port %d %s/%s, MM TX %s, preemptible TCs 0x%x, active 0x%x\n",
+ port, phy_modes(ocelot_port->phy_mode),
+ phy_speed_to_str(ocelot_port->speed),
+ mm->tx_active ? "active" : "inactive", mm->preemptible_tcs,
+ mm->active_preemptible_tcs);
+
+ ocelot_rmw_rix(ocelot, QSYS_PREEMPTION_CFG_P_QUEUES(val),
+ QSYS_PREEMPTION_CFG_P_QUEUES_M,
+ QSYS_PREEMPTION_CFG, port);
+}
+
+void ocelot_port_change_fp(struct ocelot *ocelot, int port,
+ unsigned long preemptible_tcs)
+{
+ struct ocelot_mm_state *mm = &ocelot->mm[port];
+
+ mutex_lock(&ocelot->fwd_domain_lock);
+
+ if (mm->preemptible_tcs == preemptible_tcs)
+ goto out_unlock;
+
+ mm->preemptible_tcs = preemptible_tcs;
+
+ ocelot_port_update_active_preemptible_tcs(ocelot, port);
+
+out_unlock:
+ mutex_unlock(&ocelot->fwd_domain_lock);
+}
+
static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
@@ -74,6 +127,7 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)

dev_dbg(ocelot->dev, "Port %d TX preemption %s\n",
port, mm->tx_active ? "active" : "inactive");
+ ocelot_port_update_active_preemptible_tcs(ocelot, port);

ack |= DEV_MM_STAT_MM_STATUS_PRMPT_ACTIVE_STICKY;
}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 9596c79e9223..cb8fbb241879 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -749,6 +749,8 @@ struct ocelot_mm_state {
enum ethtool_mm_verify_status verify_status;
bool tx_enabled;
bool tx_active;
+ u8 preemptible_tcs;
+ u8 active_preemptible_tcs;
};

struct ocelot_port;
@@ -1158,6 +1160,7 @@ int ocelot_port_get_mm(struct ocelot *ocelot, int port,
struct ethtool_mm_state *state);
int ocelot_port_mqprio(struct ocelot *ocelot, int port,
struct tc_mqprio_qopt_offload *mqprio);
+void ocelot_port_update_preemptible_tcs(struct ocelot *ocelot, int port);

#if IS_ENABLED(CONFIG_BRIDGE_MRP)
int ocelot_mrp_add(struct ocelot *ocelot, int port,
--
2.34.1

2023-04-15 17:08:24

[permalink] [raw]

Subject: [PATCH net-next 6/7] net: dsa: felix: act upon the mqprio qopt in taprio offload

The mqprio queue configuration can appear either through
TC_SETUP_QDISC_MQPRIO or through TC_SETUP_QDISC_TAPRIO. Make sure both
are treated in the same way.

Code does nothing new for now (except for rejecting multiple TXQs per
TC, which is a useless concept with DSA switches).

Signed-off-by: Vladimir Oltean <[email protected]>
Reviewed-by: Ferenc Fejes <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
---
Diff vs
https://lore.kernel.org/netdev/[email protected]/:
none

drivers/net/dsa/ocelot/felix_vsc9959.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 66ec2740e3cb..e055b3980ccc 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1424,6 +1424,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
mutex_lock(&ocelot->tas_lock);

if (!taprio->enable) {
+ ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
ocelot_rmw_rix(ocelot, 0, QSYS_TAG_CONFIG_ENABLE,
QSYS_TAG_CONFIG, port);

@@ -1436,15 +1437,19 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
return 0;
}

+ ret = ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
+ if (ret)
+ goto err_unlock;
+
if (taprio->cycle_time > NSEC_PER_SEC ||
taprio->cycle_time_extension >= NSEC_PER_SEC) {
ret = -EINVAL;
- goto err;
+ goto err_reset_tc;
}

if (taprio->num_entries > VSC9959_TAS_GCL_ENTRY_MAX) {
ret = -ERANGE;
- goto err;
+ goto err_reset_tc;
}

/* Enable guard band. The switch will schedule frames without taking
@@ -1468,7 +1473,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
val = ocelot_read(ocelot, QSYS_PARAM_STATUS_REG_8);
if (val & QSYS_PARAM_STATUS_REG_8_CONFIG_PENDING) {
ret = -EBUSY;
- goto err;
+ goto err_reset_tc;
}

ocelot_rmw_rix(ocelot,
@@ -1503,12 +1508,19 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
!(val & QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE),
10, 100000);
if (ret)
- goto err;
+ goto err_reset_tc;

ocelot_port->taprio = taprio_offload_get(taprio);
vsc9959_tas_guard_bands_update(ocelot, port);

-err:
+ mutex_unlock(&ocelot->tas_lock);
+
+ return 0;
+
+err_reset_tc:
+ taprio->mqprio.qopt.num_tc = 0;
+ ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
+err_unlock:
mutex_unlock(&ocelot->tas_lock);

return ret;
--
2.34.1

2023-04-17 12:58:16

[permalink] [raw]

Subject: Re: [PATCH net-next 2/7] net: mscc: ocelot: remove struct ocelot_mm_state :: lock

On Sat, Apr 15, 2023 at 08:05:46PM +0300, Vladimir Oltean wrote:
> Unfortunately, the workarounds for the hardware bugs make it pointless
> to keep fine-grained locking for the MAC Merge state of each port.
>
> Our vsc9959_cut_through_fwd() implementation requires
> ocelot->fwd_domain_lock to be held, in order to serialize with changes
> to the bridging domains and to port speed changes (which affect which
> ports can be cut-through). Simultaneously, the traffic classes which can
> be cut-through cannot be preemptible at the same time, and this will
> depend on the MAC Merge layer state (which changes from threaded
> interrupt context).
>
> Since vsc9959_cut_through_fwd() would have to hold the mm->lock of all
> ports for a correct and race-free implementation with respect to
> ocelot_mm_irq(), in practice it means that any time a port's mm->lock is
> held, it would potentially block holders of ocelot->fwd_domain_lock.
>
> In the interest of simple locking rules, make all MAC Merge layer state
> changes (and preemptible traffic class changes) be serialized by the
> ocelot->fwd_domain_lock.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-04-17 12:58:23

[permalink] [raw]

Subject: Re: [PATCH net-next 1/7] net: mscc: ocelot: export a single ocelot_mm_irq()

On Sat, Apr 15, 2023 at 08:05:45PM +0300, Vladimir Oltean wrote:
> When the switch emits an IRQ, we don't know what caused it, and we
> iterate through all ports to check the MAC Merge status.
>
> Move that iteration inside the ocelot lib; we will change the locking in
> a future change and it would be good to encapsulate that lock completely
> within the ocelot lib.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-04-17 12:58:37

[permalink] [raw]

Subject: Re: [PATCH net-next 1/7] net: mscc: ocelot: export a single ocelot_mm_irq()

On 4/15/2023 10:05 AM, Vladimir Oltean wrote:
> When the switch emits an IRQ, we don't know what caused it, and we
> iterate through all ports to check the MAC Merge status.
>
> Move that iteration inside the ocelot lib; we will change the locking in
> a future change and it would be good to encapsulate that lock completely
> within the ocelot lib.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-04-17 12:58:56

[permalink] [raw]

Subject: Re: [PATCH net-next 2/7] net: mscc: ocelot: remove struct ocelot_mm_state :: lock

On 4/15/2023 10:05 AM, Vladimir Oltean wrote:
> Unfortunately, the workarounds for the hardware bugs make it pointless
> to keep fine-grained locking for the MAC Merge state of each port.
>
> Our vsc9959_cut_through_fwd() implementation requires
> ocelot->fwd_domain_lock to be held, in order to serialize with changes
> to the bridging domains and to port speed changes (which affect which
> ports can be cut-through). Simultaneously, the traffic classes which can
> be cut-through cannot be preemptible at the same time, and this will
> depend on the MAC Merge layer state (which changes from threaded
> interrupt context).
>
> Since vsc9959_cut_through_fwd() would have to hold the mm->lock of all
> ports for a correct and race-free implementation with respect to
> ocelot_mm_irq(), in practice it means that any time a port's mm->lock is
> held, it would potentially block holders of ocelot->fwd_domain_lock.
>
> In the interest of simple locking rules, make all MAC Merge layer state
> changes (and preemptible traffic class changes) be serialized by the
> ocelot->fwd_domain_lock.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-04-17 12:59:07

[permalink] [raw]

Subject: Re: [PATCH net-next 3/7] net: mscc: ocelot: optimize ocelot_mm_irq()

On Sat, Apr 15, 2023 at 08:05:47PM +0300, Vladimir Oltean wrote:
> The MAC Merge IRQ of all ports is shared with the PTP TX timestamp IRQ
> of all ports, which means that currently, when a PTP TX timestamp is
> generated, felix_irq_handler() also polls for the MAC Merge layer status
> of all ports, looking for changes. This makes the kernel do more work,
> and under certain circumstances may make ptp4l require a
> tx_timestamp_timeout argument higher than before.
>
> Changes to the MAC Merge layer status are only to be expected under
> certain conditions - its TX direction needs to be enabled - so we can
> check early if that is the case, and omit register access otherwise.
>
> Make ocelot_mm_update_port_status() skip register access if
> mm->tx_enabled is unset, and also call it once more, outside IRQ
> context, from ocelot_port_set_mm(), when mm->tx_enabled transitions from
> true to false, because an IRQ is also expected in that case.
>
> Also, a port may have its MAC Merge layer enabled but it may not have
> generated the interrupt. In that case, there's no point in writing to
> DEV_MM_STATUS to acknowledge that IRQ. We can reduce the number of
> register writes per port with MM enabled by keeping an "ack" variable
> which writes the "write-one-to-clear" bits. Those are 3 in number:
> PRMPT_ACTIVE_STICKY, UNEXP_RX_PFRM_STICKY and UNEXP_TX_PFRM_STICKY.
> The other fields in DEV_MM_STATUS are read-only and it doesn't matter
> what is written to them, so writing zero is just fine.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

No need to respin on my account.
However, I do observe that this patch is doing several things,
and I do wonder if it could have been more than one patch.

2023-04-17 13:05:35

[permalink] [raw]

Subject: Re: [PATCH net-next 3/7] net: mscc: ocelot: optimize ocelot_mm_irq()

On 4/15/2023 10:05 AM, Vladimir Oltean wrote:
> The MAC Merge IRQ of all ports is shared with the PTP TX timestamp IRQ
> of all ports, which means that currently, when a PTP TX timestamp is
> generated, felix_irq_handler() also polls for the MAC Merge layer status
> of all ports, looking for changes. This makes the kernel do more work,
> and under certain circumstances may make ptp4l require a
> tx_timestamp_timeout argument higher than before.
>
> Changes to the MAC Merge layer status are only to be expected under
> certain conditions - its TX direction needs to be enabled - so we can
> check early if that is the case, and omit register access otherwise.
>
> Make ocelot_mm_update_port_status() skip register access if
> mm->tx_enabled is unset, and also call it once more, outside IRQ
> context, from ocelot_port_set_mm(), when mm->tx_enabled transitions from
> true to false, because an IRQ is also expected in that case.
>
> Also, a port may have its MAC Merge layer enabled but it may not have
> generated the interrupt. In that case, there's no point in writing to
> DEV_MM_STATUS to acknowledge that IRQ. We can reduce the number of
> register writes per port with MM enabled by keeping an "ack" variable
> which writes the "write-one-to-clear" bits. Those are 3 in number:
> PRMPT_ACTIVE_STICKY, UNEXP_RX_PFRM_STICKY and UNEXP_TX_PFRM_STICKY.
> The other fields in DEV_MM_STATUS are read-only and it doesn't matter
> what is written to them, so writing zero is just fine.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-04-17 13:06:10

[permalink] [raw]

Subject: Re: [PATCH net-next 4/7] net: mscc: ocelot: don't rely on cached verify_status in ocelot_port_get_mm()

On 4/15/2023 10:05 AM, Vladimir Oltean wrote:
> ocelot_mm_update_port_status() updates mm->verify_status, but when the
> verification state of a port changes, an IRQ isn't emitted, but rather,
> only when the verification state reaches one of the final states (like
> DISABLED, FAILED, SUCCEEDED) - things that would affect mm->tx_active,
> which is what the IRQ *is* actually emitted for.
>
> That is to say, user space may miss reports of an intermediary MAC Merge
> verification state (like from INITIAL to VERIFYING), unless there was an
> IRQ notifying the driver of the change in mm->tx_active as well.
>
> This is not a huge deal, but for reliable reporting to user space, let's
> call ocelot_mm_update_port_status() synchronously from
> ocelot_port_get_mm(), which makes user space see the current MM status.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-04-17 13:06:19

[permalink] [raw]

Subject: Re: [PATCH net-next 5/7] net: mscc: ocelot: add support for mqprio offload

On 4/15/2023 10:05 AM, Vladimir Oltean wrote:
> This doesn't apply anything to hardware and in general doesn't do
> anything that the software variant doesn't do, except for checking that
> there isn't more than 1 TXQ per TC (TXQs for a DSA switch are a dubious
> concept anyway). The reason we add this is to be able to parse one more
> field added to struct tc_mqprio_qopt_offload, namely preemptible_tcs.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Reviewed-by: Ferenc Fejes <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-04-17 13:06:30

[permalink] [raw]

Subject: Re: [PATCH net-next 6/7] net: dsa: felix: act upon the mqprio qopt in taprio offload

On 4/15/2023 10:05 AM, Vladimir Oltean wrote:
> The mqprio queue configuration can appear either through
> TC_SETUP_QDISC_MQPRIO or through TC_SETUP_QDISC_TAPRIO. Make sure both
> are treated in the same way.
>
> Code does nothing new for now (except for rejecting multiple TXQs per
> TC, which is a useless concept with DSA switches).
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Reviewed-by: Ferenc Fejes <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-04-17 13:08:18