This series consolidates the behavior of the 2 drivers that implement
the ethtool MAC Merge layer by making NXP ENETC commit its preemptible
traffic classes to hardware only when MM TX is active (same as Ocelot).
Then, after resolving an issue with the ENETC driver, it restricts user
space from entering 2 states which don't make sense:
- pmac-enabled off tx-enabled on verify-enabled *
- pmac-enabled * tx-enabled off verify-enabled on
Then, it introduces a selftest (ethtool_mm.sh) which puts everything
together and tests all valid configurations known to me.
This is simultaneously the v2 of "[PATCH net-next 0/2] ethtool mm API
improvements":
https://lore.kernel.org/netdev/[email protected]/
which had caused some problems to openlldp. Those were solved in the
meantime, see:
https://github.com/intel/openlldp/commit/11171b474f6f3cbccac5d608b7f26b32ff72c651
and of "[RFC PATCH net-next] selftests: forwarding: add a test for MAC
Merge layer":
https://lore.kernel.org/netdev/[email protected]/
Petr Machata (2):
selftests: forwarding: sch_tbf_*: Add a pre-run hook
selftests: forwarding: generalize bail_on_lldpad from mlxsw
Vladimir Oltean (7):
net: enetc: fix MAC Merge layer remaining enabled until a link down
event
net: enetc: report mm tx-active based on tx-enabled and verify-status
net: enetc: only commit preemptible TCs to hardware when MM TX is
active
net: enetc: include MAC Merge / FP registers in register dump
net: ethtool: mm: sanitize some UAPI configurations
selftests: forwarding: introduce helper for standard ethtool counters
selftests: forwarding: add a test for MAC Merge layer
drivers/net/ethernet/freescale/enetc/enetc.c | 23 +-
drivers/net/ethernet/freescale/enetc/enetc.h | 5 +-
.../ethernet/freescale/enetc/enetc_ethtool.c | 94 +++++-
.../net/ethernet/freescale/enetc/enetc_hw.h | 3 +
net/ethtool/mm.c | 10 +
.../drivers/net/mlxsw/qos_headroom.sh | 3 +-
.../selftests/drivers/net/mlxsw/qos_lib.sh | 28 --
.../selftests/drivers/net/mlxsw/qos_pfc.sh | 3 +-
.../selftests/drivers/net/mlxsw/sch_ets.sh | 3 +-
.../drivers/net/mlxsw/sch_red_core.sh | 1 -
.../drivers/net/mlxsw/sch_red_ets.sh | 2 +-
.../drivers/net/mlxsw/sch_red_root.sh | 2 +-
.../drivers/net/mlxsw/sch_tbf_ets.sh | 6 +-
.../drivers/net/mlxsw/sch_tbf_prio.sh | 6 +-
.../drivers/net/mlxsw/sch_tbf_root.sh | 6 +-
.../testing/selftests/net/forwarding/Makefile | 1 +
.../selftests/net/forwarding/ethtool_mm.sh | 288 ++++++++++++++++++
tools/testing/selftests/net/forwarding/lib.sh | 60 ++++
.../net/forwarding/sch_tbf_etsprio.sh | 4 +
.../selftests/net/forwarding/sch_tbf_root.sh | 4 +
20 files changed, 486 insertions(+), 66 deletions(-)
create mode 100755 tools/testing/selftests/net/forwarding/ethtool_mm.sh
--
2.34.1
This was left as TODO in commit 01e23b2b3bad ("net: enetc: add support
for preemptible traffic classes") since it's relatively complicated.
Where this makes a difference is with a configuration as follows:
ethtool --set-mm eno0 pmac-enabled on tx-enabled on verify-enabled on
Preemptible packets should only be sent when the MAC Merge TX direction
becomes active (i.o.w. when the verification process succeeds, aka when
the link partner confirms it can process preemptible traffic). But the
tc qdisc with the preemptible traffic classes is offloaded completely
asynchronously w.r.t. the MM becoming active.
The ENETC manual does suggest that this should be handled in the driver:
"On startup, software should wait for the verification process to
complete (MMCSR[VSTS]=011) before initiating traffic".
Adding the necessary logic allows future selftests to uphold the claim
that an inactive or disabled MAC Merge layer should never send data
packets through the pMAC.
This change moves enetc_set_ptcfpr() from enetc.c to enetc_ethtool.c,
where its only caller is now - enetc_mm_commit_preemptible_tcs().
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: patch is new
drivers/net/ethernet/freescale/enetc/enetc.c | 23 ++-----
drivers/net/ethernet/freescale/enetc/enetc.h | 5 +-
.../ethernet/freescale/enetc/enetc_ethtool.c | 62 +++++++++++++++++++
.../net/ethernet/freescale/enetc/enetc_hw.h | 3 +
4 files changed, 75 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 41c194c1672d..3c4fa26f0f9b 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -25,23 +25,12 @@ void enetc_port_mac_wr(struct enetc_si *si, u32 reg, u32 val)
}
EXPORT_SYMBOL_GPL(enetc_port_mac_wr);
-void enetc_set_ptcfpr(struct enetc_hw *hw, unsigned long preemptible_tcs)
+static void enetc_change_preemptible_tcs(struct enetc_ndev_priv *priv,
+ u8 preemptible_tcs)
{
- u32 val;
- int tc;
-
- for (tc = 0; tc < 8; tc++) {
- val = enetc_port_rd(hw, ENETC_PTCFPR(tc));
-
- if (preemptible_tcs & BIT(tc))
- val |= ENETC_PTCFPR_FPE;
- else
- val &= ~ENETC_PTCFPR_FPE;
-
- enetc_port_wr(hw, ENETC_PTCFPR(tc), val);
- }
+ priv->preemptible_tcs = preemptible_tcs;
+ enetc_mm_commit_preemptible_tcs(priv);
}
-EXPORT_SYMBOL_GPL(enetc_set_ptcfpr);
static int enetc_num_stack_tx_queues(struct enetc_ndev_priv *priv)
{
@@ -2659,7 +2648,7 @@ static void enetc_reset_tc_mqprio(struct net_device *ndev)
enetc_debug_tx_ring_prios(priv);
- enetc_set_ptcfpr(hw, 0);
+ enetc_change_preemptible_tcs(priv, 0);
}
int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
@@ -2714,7 +2703,7 @@ int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
enetc_debug_tx_ring_prios(priv);
- enetc_set_ptcfpr(hw, mqprio->preemptible_tcs);
+ enetc_change_preemptible_tcs(priv, mqprio->preemptible_tcs);
return 0;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 143078a9ef16..c97a8e3d7a7f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -355,6 +355,9 @@ struct enetc_ndev_priv {
u16 rx_bd_count, tx_bd_count;
u16 msg_enable;
+
+ u8 preemptible_tcs;
+
enum enetc_active_offloads active_offloads;
u32 speed; /* store speed for compare update pspeed */
@@ -433,6 +436,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
/* ethtool */
void enetc_set_ethtool_ops(struct net_device *ndev);
void enetc_mm_link_state_update(struct enetc_ndev_priv *priv, bool link);
+void enetc_mm_commit_preemptible_tcs(struct enetc_ndev_priv *priv);
/* control buffer descriptor ring (CBDR) */
int enetc_setup_cbdr(struct device *dev, struct enetc_hw *hw, int bd_count,
@@ -486,7 +490,6 @@ static inline void enetc_cbd_free_data_mem(struct enetc_si *si, int size,
void enetc_reset_ptcmsdur(struct enetc_hw *hw);
void enetc_set_ptcmsdur(struct enetc_hw *hw, u32 *queue_max_sdu);
-void enetc_set_ptcfpr(struct enetc_hw *hw, unsigned long preemptible_tcs);
#ifdef CONFIG_FSL_ENETC_QOS
int enetc_qos_query_caps(struct net_device *ndev, void *type_data);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index deb674752851..838a92131963 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -991,6 +991,64 @@ static int enetc_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
return 0;
}
+static int enetc_mm_wait_tx_active(struct enetc_hw *hw, int verify_time)
+{
+ int timeout = verify_time * USEC_PER_MSEC * ENETC_MM_VERIFY_RETRIES;
+ u32 val;
+
+ /* This will time out after the standard value of 3 verification
+ * attempts. To not sleep forever, it relies on a non-zero verify_time,
+ * guarantee which is provided by the ethtool nlattr policy.
+ */
+ return read_poll_timeout(enetc_port_rd, val,
+ ENETC_MMCSR_GET_VSTS(val) == 3,
+ ENETC_MM_VERIFY_SLEEP_US, timeout,
+ true, hw, ENETC_MMCSR);
+}
+
+static void enetc_set_ptcfpr(struct enetc_hw *hw, u8 preemptible_tcs)
+{
+ u32 val;
+ int tc;
+
+ for (tc = 0; tc < 8; tc++) {
+ val = enetc_port_rd(hw, ENETC_PTCFPR(tc));
+
+ if (preemptible_tcs & BIT(tc))
+ val |= ENETC_PTCFPR_FPE;
+ else
+ val &= ~ENETC_PTCFPR_FPE;
+
+ enetc_port_wr(hw, ENETC_PTCFPR(tc), val);
+ }
+}
+
+/* ENETC does not have an IRQ to notify changes to the MAC Merge TX status
+ * (active/inactive), but the preemptible traffic classes should only be
+ * committed to hardware once TX is active. Resort to polling.
+ */
+void enetc_mm_commit_preemptible_tcs(struct enetc_ndev_priv *priv)
+{
+ struct enetc_hw *hw = &priv->si->hw;
+ u8 preemptible_tcs = 0;
+ u32 val;
+ int err;
+
+ val = enetc_port_rd(hw, ENETC_MMCSR);
+ if (!(val & ENETC_MMCSR_ME))
+ goto out;
+
+ if (!(val & ENETC_MMCSR_VDIS)) {
+ err = enetc_mm_wait_tx_active(hw, ENETC_MMCSR_GET_VT(val));
+ if (err)
+ goto out;
+ }
+
+ preemptible_tcs = priv->preemptible_tcs;
+out:
+ enetc_set_ptcfpr(hw, preemptible_tcs);
+}
+
/* FIXME: Workaround for the link partner's verification failing if ENETC
* priorly received too much express traffic. The documentation doesn't
* suggest this is needed.
@@ -1061,6 +1119,8 @@ static int enetc_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
enetc_restart_emac_rx(priv->si);
+ enetc_mm_commit_preemptible_tcs(priv);
+
mutex_unlock(&priv->mm_lock);
return 0;
@@ -1094,6 +1154,8 @@ void enetc_mm_link_state_update(struct enetc_ndev_priv *priv, bool link)
enetc_port_wr(hw, ENETC_MMCSR, val);
+ enetc_mm_commit_preemptible_tcs(priv);
+
mutex_unlock(&priv->mm_lock);
}
EXPORT_SYMBOL_GPL(enetc_mm_link_state_update);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 36bb2d6d5658..1619943fb263 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -3,6 +3,9 @@
#include <linux/bitops.h>
+#define ENETC_MM_VERIFY_SLEEP_US USEC_PER_MSEC
+#define ENETC_MM_VERIFY_RETRIES 3
+
/* ENETC device IDs */
#define ENETC_DEV_ID_PF 0xe100
#define ENETC_DEV_ID_VF 0xef00
--
2.34.1
These have been useful in debugging various problems related to frame
preemption, so make them available through ethtool --register-dump for
later too.
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: patch is new
.../ethernet/freescale/enetc/enetc_ethtool.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 838a92131963..e993ed04ab57 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -32,6 +32,12 @@ static const u32 enetc_port_regs[] = {
ENETC_PM0_CMD_CFG, ENETC_PM0_MAXFRM, ENETC_PM0_IF_MODE
};
+static const u32 enetc_port_mm_regs[] = {
+ ENETC_MMCSR, ENETC_PFPMR, ENETC_PTCFPR(0), ENETC_PTCFPR(1),
+ ENETC_PTCFPR(2), ENETC_PTCFPR(3), ENETC_PTCFPR(4), ENETC_PTCFPR(5),
+ ENETC_PTCFPR(6), ENETC_PTCFPR(7),
+};
+
static int enetc_get_reglen(struct net_device *ndev)
{
struct enetc_ndev_priv *priv = netdev_priv(ndev);
@@ -45,6 +51,9 @@ static int enetc_get_reglen(struct net_device *ndev)
if (hw->port)
len += ARRAY_SIZE(enetc_port_regs);
+ if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU))
+ len += ARRAY_SIZE(enetc_port_mm_regs);
+
len *= sizeof(u32) * 2; /* store 2 entries per reg: addr and value */
return len;
@@ -90,6 +99,14 @@ static void enetc_get_regs(struct net_device *ndev, struct ethtool_regs *regs,
*buf++ = addr;
*buf++ = enetc_rd(hw, addr);
}
+
+ if (priv->si->hw_features & ENETC_SI_F_QBU) {
+ for (i = 0; i < ARRAY_SIZE(enetc_port_mm_regs); i++) {
+ addr = ENETC_PORT_BASE + enetc_port_mm_regs[i];
+ *buf++ = addr;
+ *buf++ = enetc_rd(hw, addr);
+ }
+ }
}
static const struct {
--
2.34.1
From: Petr Machata <[email protected]>
The driver-specific wrappers of these selftests invoke bail_on_lldpad to
make sure that LLDPAD doesn't trample the configuration. The function
bail_on_lldpad is going to move to lib.sh in the next patch. With that, it
won't be visible for the wrappers before sourcing the framework script. And
after sourcing it, it is too late: the selftest will have run by then.
One option might be to source NUM_NETIFS=0 lib.sh from the wrapper, but
even if that worked (it might, it might not), that seems cumbersome. lib.sh
is doing fair amount of stuff, and even if it works today, it does not look
particularly solid as a solution.
Instead, introduce a hook, sch_tbf_pre_hook(), that when available, gets
invoked. Move the bail to the hook.
Signed-off-by: Petr Machata <[email protected]>
Reviewed-by: Danielle Ratson <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: new patch from Petr
tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh | 6 +++++-
tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh | 6 +++++-
tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh | 6 +++++-
tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh | 4 ++++
tools/testing/selftests/net/forwarding/sch_tbf_root.sh | 4 ++++
5 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh
index c6ce0b448bf3..b9b4cdf14ceb 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh
@@ -2,7 +2,11 @@
# SPDX-License-Identifier: GPL-2.0
source qos_lib.sh
-bail_on_lldpad
+
+sch_tbf_pre_hook()
+{
+ bail_on_lldpad
+}
lib_dir=$(dirname $0)/../../../net/forwarding
TCFLAGS=skip_sw
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
index 8d245f331619..dff9810ee04f 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
@@ -2,7 +2,11 @@
# SPDX-License-Identifier: GPL-2.0
source qos_lib.sh
-bail_on_lldpad
+
+sch_tbf_pre_hook()
+{
+ bail_on_lldpad
+}
lib_dir=$(dirname $0)/../../../net/forwarding
TCFLAGS=skip_sw
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
index 013886061f15..75406bd7036e 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
@@ -2,7 +2,11 @@
# SPDX-License-Identifier: GPL-2.0
source qos_lib.sh
-bail_on_lldpad
+
+sch_tbf_pre_hook()
+{
+ bail_on_lldpad
+}
lib_dir=$(dirname $0)/../../../net/forwarding
TCFLAGS=skip_sw
diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh
index 75a37c189ef3..df9bcd6a811a 100644
--- a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh
+++ b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh
@@ -57,6 +57,10 @@ tbf_root_test()
tc qdisc del dev $swp2 root
}
+if type -t sch_tbf_pre_hook >/dev/null; then
+ sch_tbf_pre_hook
+fi
+
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_root.sh b/tools/testing/selftests/net/forwarding/sch_tbf_root.sh
index 72aa21ba88c7..96c997be0d03 100755
--- a/tools/testing/selftests/net/forwarding/sch_tbf_root.sh
+++ b/tools/testing/selftests/net/forwarding/sch_tbf_root.sh
@@ -23,6 +23,10 @@ tbf_test()
tc qdisc del dev $swp2 root
}
+if type -t sch_tbf_pre_hook >/dev/null; then
+ sch_tbf_pre_hook
+fi
+
trap cleanup EXIT
setup_prepare
--
2.34.1
The verify-enabled boolean (ETHTOOL_A_MM_VERIFY_ENABLED) was intended to
be a sub-setting of tx-enabled (ETHTOOL_A_MM_TX_ENABLED). IOW, MAC Merge
TX can be enabled with or without verification, but verification with TX
disabled makes no sense.
The pmac-enabled boolean (ETHTOOL_A_MM_PMAC_ENABLED) was intended to be
a global toggle from an API perspective, whereas tx-enabled just handles
the TX direction. IOW, the pMAC can be enabled with or without TX, but
it doesn't make sense to enable TX if the pMAC is not enabled.
Add two checks which sanitize and reject these invalid cases.
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: add missing "Verification requires TX enabled" check which was
only mentioned in the commit message
net/ethtool/mm.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
index e00d7d5cea7e..4058a557b5a4 100644
--- a/net/ethtool/mm.c
+++ b/net/ethtool/mm.c
@@ -214,6 +214,16 @@ static int ethnl_set_mm(struct ethnl_req_info *req_info, struct genl_info *info)
return -ERANGE;
}
+ if (cfg.verify_enabled && !cfg.tx_enabled) {
+ NL_SET_ERR_MSG(extack, "Verification requires TX enabled");
+ return -EINVAL;
+ }
+
+ if (cfg.tx_enabled && !cfg.pmac_enabled) {
+ NL_SET_ERR_MSG(extack, "TX enabled requires pMAC enabled");
+ return -EINVAL;
+ }
+
ret = dev->ethtool_ops->set_mm(dev, &cfg, extack);
return ret < 0 ? ret : 1;
}
--
2.34.1
The MAC Merge layer (IEEE 802.3-2018 clause 99) does all the heavy
lifting for Frame Preemption (IEEE 802.1Q-2018 clause 6.7.2), a TSN
feature for minimizing latency.
Preemptible traffic is different on the wire from normal traffic in
incompatible ways. If we send a preemptible packet and the link partner
doesn't support preemption, it will drop it as an error frame and we
will never know. The MAC Merge layer has a control plane of its own,
which can be manipulated (using ethtool) in order to negotiate this
capability with the link partner (through LLDP).
Actually the TLV format for LLDP solves this problem only partly,
because both partners only advertise:
- if they support preemption (RX and TX)
- if they have enabled preemption (TX)
so we cannot tell the link partner what to do - we cannot force it to
enable reception of our preemptible packets.
That is fully solved by the verification feature, where the local device
generates some small probe frames which look like preemptible frames
with no useful content, and the link partner is obliged to respond to
them if it supports the standard. If the verification times out, we know
that preemption isn't active in our TX direction on the link.
Having clarified the definition, this selftest exercises the manual
(ethtool) configuration path of 2 link partners (with and without
verification), and the LLDP code path, using the openlldp project.
The test also verifies the TX activity of the MAC Merge layer by
sending traffic through a traffic class configured as preemptible
(using mqprio). There isn't a good way to make this really portable
(user space cannot find out how many traffic classes there are for
a device), but I chose num_tc 4 here, that should work reasonably well.
I also know that some devices (stmmac) only permit TXQ0 to be
preemptible, so this is why PREEMPTIBLE_PRIO was strategically chosen
as 0. Even if other hardware is more configurable, this test should
cover the baseline.
This is not really a "forwarding" selftest, but I put it near the other
"ethtool" selftests.
$ ./ethtool_mm.sh eno0 swp0
TEST: Manual configuration with verification: eno0 to swp0 [ OK ]
TEST: Manual configuration with verification: swp0 to eno0 [ OK ]
TEST: Manual configuration without verification: eno0 to swp0 [ OK ]
TEST: Manual configuration without verification: swp0 to eno0 [ OK ]
TEST: Manual configuration with failed verification: eno0 to swp0 [ OK ]
TEST: Manual configuration with failed verification: swp0 to eno0 [ OK ]
TEST: LLDP [ OK ]
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2:
- bail_on_lldpad is generic
- manual ethtool mm tests are bidirectional
- new "failed verification" test
- testing with traffic as well
.../testing/selftests/net/forwarding/Makefile | 1 +
.../selftests/net/forwarding/ethtool_mm.sh | 288 ++++++++++++++++++
tools/testing/selftests/net/forwarding/lib.sh | 18 ++
3 files changed, 307 insertions(+)
create mode 100755 tools/testing/selftests/net/forwarding/ethtool_mm.sh
diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 236f6b796a52..a474c60fe348 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -15,6 +15,7 @@ TEST_PROGS = bridge_igmp.sh \
custom_multipath_hash.sh \
dual_vxlan_bridge.sh \
ethtool_extended_state.sh \
+ ethtool_mm.sh \
ethtool.sh \
gre_custom_multipath_hash.sh \
gre_inner_v4_multipath.sh \
diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
new file mode 100755
index 000000000000..c580ad623848
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
@@ -0,0 +1,288 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="
+ manual_with_verification_h1_to_h2
+ manual_with_verification_h2_to_h1
+ manual_without_verification_h1_to_h2
+ manual_without_verification_h2_to_h1
+ manual_failed_verification_h1_to_h2
+ manual_failed_verification_h2_to_h1
+ lldp
+"
+
+NUM_NETIFS=2
+REQUIRE_MZ=no
+PREEMPTIBLE_PRIO=0
+source lib.sh
+
+traffic_test()
+{
+ local if=$1; shift
+ local src=$1; shift
+ local num_pkts=10000
+ local before=
+ local after=
+ local delta=
+
+ before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
+
+ $MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
+
+ after=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
+
+ delta=$((after - before))
+
+ # Allow an extra 1% tolerance for random packets sent by the stack
+ [ $delta -ge $num_pkts ] && [ $delta -le $((num_pkts + 100)) ]
+}
+
+manual_with_verification()
+{
+ local tx=$1; shift
+ local rx=$1; shift
+
+ RET=0
+
+ # It isn't completely clear from IEEE 802.3-2018 Figure 99-5: Transmit
+ # Processing state diagram whether the "send_r" variable (send response
+ # to verification frame) should be taken into consideration while the
+ # MAC Merge TX direction is disabled. That being said, at least the
+ # NXP ENETC does not, and requires tx-enabled on in order to respond to
+ # the link partner's verification frames.
+ ethtool --set-mm $rx tx-enabled on
+ ethtool --set-mm $tx verify-enabled on tx-enabled on
+
+ # Wait for verification to finish
+ sleep 1
+
+ ethtool --json --show-mm $tx | jq -r '.[]."verify-status"' | \
+ grep -q 'SUCCEEDED'
+ check_err "$?" "Verification did not succeed"
+
+ ethtool --json --show-mm $tx | jq -r '.[]."tx-active"' | grep -q 'true'
+ check_err "$?" "pMAC TX is not active"
+
+ traffic_test $tx "pmac"
+ check_err "$?" "Traffic did not get sent through $tx's pMAC"
+
+ ethtool --set-mm $tx verify-enabled off tx-enabled off
+ ethtool --set-mm $rx tx-enabled off
+
+ log_test "Manual configuration with verification: $tx to $rx"
+}
+
+manual_with_verification_h1_to_h2()
+{
+ manual_with_verification $h1 $h2
+}
+
+manual_with_verification_h2_to_h1()
+{
+ manual_with_verification $h2 $h1
+}
+
+manual_without_verification()
+{
+ local tx=$1; shift
+ local rx=$1; shift
+
+ RET=0
+
+ ethtool --set-mm $tx verify-enabled off tx-enabled on
+
+ ethtool --json --show-mm $tx | jq -r '.[]."verify-status"' | \
+ grep -q 'DISABLED'
+ check_err "$?" "Verification is not disabled"
+
+ ethtool --json --show-mm $tx | jq -r '.[]."tx-active"' | grep -q 'true'
+ check_err "$?" "pMAC TX is not active"
+
+ traffic_test $tx "pmac"
+ check_err "$?" "Traffic did not get sent through $tx's pMAC"
+
+ ethtool --set-mm $tx verify-enabled off tx-enabled off
+
+ log_test "Manual configuration without verification: $tx to $rx"
+}
+
+manual_without_verification_h1_to_h2()
+{
+ manual_without_verification $h1 $h2
+}
+
+manual_without_verification_h2_to_h1()
+{
+ manual_without_verification $h2 $h1
+}
+
+manual_failed_verification()
+{
+ local tx=$1; shift
+ local rx=$1; shift
+
+ RET=0
+
+ ethtool --set-mm $rx pmac-enabled off
+ ethtool --set-mm $tx verify-enabled on tx-enabled on
+
+ # Wait for verification to time out
+ sleep 1
+
+ ethtool --json --show-mm $tx | jq -r '.[]."verify-status"' | \
+ grep -q 'SUCCEEDED'
+ check_fail "$?" "Verification succeeded when it shouldn't have"
+
+ ethtool --json --show-mm $tx | jq -r '.[]."tx-active"' | grep -q 'true'
+ check_fail "$?" "pMAC TX is active when it shouldn't have"
+
+ traffic_test $tx "emac"
+ check_err "$?" "Traffic did not get sent through $tx's eMAC"
+
+ ethtool --set-mm $tx verify-enabled off tx-enabled off
+ ethtool --set-mm $rx pmac-enabled on
+
+ log_test "Manual configuration with failed verification: $tx to $rx"
+}
+
+manual_failed_verification_h1_to_h2()
+{
+ manual_failed_verification $h1 $h2
+}
+
+manual_failed_verification_h2_to_h1()
+{
+ manual_failed_verification $h2 $h1
+}
+
+lldp_change_add_frag_size()
+{
+ local add_frag_size=$1
+
+ lldptool -T -i $h1 -V addEthCaps addFragSize=$add_frag_size >/dev/null
+ # Wait for TLVs to be received
+ sleep 2
+ lldptool -i $h2 -t -n -V addEthCaps | \
+ grep -q "Additional fragment size: $add_frag_size"
+}
+
+lldp()
+{
+ RET=0
+
+ systemctl start lldpad
+
+ # Configure the interfaces to receive and transmit LLDPDUs
+ lldptool -L -i $h1 adminStatus=rxtx >/dev/null
+ lldptool -L -i $h2 adminStatus=rxtx >/dev/null
+
+ # Enable the transmission of Additional Ethernet Capabilities TLV
+ lldptool -T -i $h1 -V addEthCaps enableTx=yes >/dev/null
+ lldptool -T -i $h2 -V addEthCaps enableTx=yes >/dev/null
+
+ # Wait for TLVs to be received
+ sleep 2
+
+ lldptool -i $h1 -t -n -V addEthCaps | \
+ grep -q "Preemption capability active"
+ check_err "$?" "$h1 pMAC TX is not active"
+
+ lldptool -i $h2 -t -n -V addEthCaps | \
+ grep -q "Preemption capability active"
+ check_err "$?" "$h2 pMAC TX is not active"
+
+ lldp_change_add_frag_size 3
+ check_err "$?" "addFragSize 3"
+
+ lldp_change_add_frag_size 2
+ check_err "$?" "addFragSize 2"
+
+ lldp_change_add_frag_size 1
+ check_err "$?" "addFragSize 1"
+
+ lldp_change_add_frag_size 0
+ check_err "$?" "addFragSize 0"
+
+ traffic_test $h1 "pmac"
+ check_err "$?" "Traffic did not get sent through $h1's pMAC"
+
+ traffic_test $h2 "pmac"
+ check_err "$?" "Traffic did not get sent through $h2's pMAC"
+
+ systemctl stop lldpad
+
+ log_test "LLDP"
+}
+
+h1_create()
+{
+ ip link set dev $h1 up
+
+ tc qdisc add dev $h1 root mqprio num_tc 4 map 0 1 2 3 \
+ queues 1@0 1@1 1@2 1@3 \
+ fp P E E E \
+ hw 1
+
+ ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off
+}
+
+h2_create()
+{
+ ip link set dev $h2 up
+
+ ethtool --set-mm $h2 pmac-enabled on tx-enabled off verify-enabled off
+
+ tc qdisc add dev $h2 root mqprio num_tc 4 map 0 1 2 3 \
+ queues 1@0 1@1 1@2 1@3 \
+ fp P E E E \
+ hw 1
+}
+
+h1_destroy()
+{
+ ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off
+
+ tc qdisc del dev $h1 root
+
+ ip link set dev $h1 down
+}
+
+h2_destroy()
+{
+ tc qdisc del dev $h2 root
+
+ ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off
+
+ ip link set dev $h2 down
+}
+
+setup_prepare()
+{
+ check_ethtool_mm_support
+ check_tc_fp_support
+ require_command lldptool
+ bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
+
+ h1=${NETIFS[p1]}
+ h2=${NETIFS[p2]}
+
+ h1_create
+ h2_create
+}
+
+cleanup()
+{
+ pre_cleanup
+
+ h2_destroy
+ h1_destroy
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 36e47c9d7cca..057c3d0ad620 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -120,6 +120,15 @@ check_tc_action_hw_stats_support()
fi
}
+check_tc_fp_support()
+{
+ tc qdisc add dev lo mqprio help 2>&1 | grep -q "fp "
+ if [[ $? -ne 0 ]]; then
+ echo "SKIP: iproute2 too old; tc is missing frame preemption support"
+ exit $ksft_skip
+ fi
+}
+
check_ethtool_lanes_support()
{
ethtool --help 2>&1| grep lanes &> /dev/null
@@ -129,6 +138,15 @@ check_ethtool_lanes_support()
fi
}
+check_ethtool_mm_support()
+{
+ ethtool --help 2>&1| grep -- '--show-mm' &> /dev/null
+ if [[ $? -ne 0 ]]; then
+ echo "SKIP: ethtool too old; it is missing MAC Merge layer support"
+ exit $ksft_skip
+ fi
+}
+
check_locked_port_support()
{
if ! bridge -d link show | grep -q " locked"; then
--
2.34.1
Counters for the MAC Merge layer and preemptible MAC have standardized
so far on using structured ethtool stats as opposed to the driver
specific names and meanings.
Benefit from that rare opportunity and introduce a helper to lib.sh for
querying standardized counters, in the hope that these will take off for
other uses as well.
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: patch is new
tools/testing/selftests/net/forwarding/lib.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index efd48e1cadd2..36e47c9d7cca 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -787,6 +787,17 @@ ethtool_stats_get()
ethtool -S $dev | grep "^ *$stat:" | head -n 1 | cut -d: -f2
}
+ethtool_std_stats_get()
+{
+ local dev=$1; shift
+ local grp=$1; shift
+ local name=$1; shift
+ local src=$1; shift
+
+ ethtool --json -S $dev --groups $grp -- --src $src | \
+ jq '.[]."'"$grp"'"."'$name'"'
+}
+
qdisc_stats_get()
{
local dev=$1; shift
--
2.34.1
From: Petr Machata <[email protected]>
mlxsw selftests often invoke a bail_on_lldpad() helper to make sure LLDPAD
is not running, to prevent conflicts between the QoS configuration applied
through TC or DCB command line tool, and the DCB configuration that LLDPAD
might apply. This helper might be useful to others. Move the function to
lib.sh, and parameterize to make reusable in other contexts.
Signed-off-by: Petr Machata <[email protected]>
Reviewed-by: Danielle Ratson <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: new patch from Petr
.../drivers/net/mlxsw/qos_headroom.sh | 3 +-
.../selftests/drivers/net/mlxsw/qos_lib.sh | 28 -----------------
.../selftests/drivers/net/mlxsw/qos_pfc.sh | 3 +-
.../selftests/drivers/net/mlxsw/sch_ets.sh | 3 +-
.../drivers/net/mlxsw/sch_red_core.sh | 1 -
.../drivers/net/mlxsw/sch_red_ets.sh | 2 +-
.../drivers/net/mlxsw/sch_red_root.sh | 2 +-
.../drivers/net/mlxsw/sch_tbf_ets.sh | 4 +--
.../drivers/net/mlxsw/sch_tbf_prio.sh | 4 +--
.../drivers/net/mlxsw/sch_tbf_root.sh | 4 +--
tools/testing/selftests/net/forwarding/lib.sh | 31 +++++++++++++++++++
11 files changed, 39 insertions(+), 46 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh
index 3569ff45f7d5..88162b4027c0 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh
@@ -18,7 +18,6 @@ lib_dir=$(dirname $0)/../../../net/forwarding
NUM_NETIFS=0
source $lib_dir/lib.sh
source $lib_dir/devlink_lib.sh
-source qos_lib.sh
swp=$NETIF_NO_CABLE
@@ -371,7 +370,7 @@ test_tc_int_buf()
tc qdisc delete dev $swp root
}
-bail_on_lldpad
+bail_on_lldpad "configure DCB" "configure Qdiscs"
trap cleanup EXIT
setup_wait
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh
index faa51012cdac..5ad092b9bf10 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh
@@ -54,31 +54,3 @@ measure_rate()
echo $ir $er
return $ret
}
-
-bail_on_lldpad()
-{
- if systemctl is-active --quiet lldpad; then
-
- cat >/dev/stderr <<-EOF
- WARNING: lldpad is running
-
- lldpad will likely configure DCB, and this test will
- configure Qdiscs. mlxsw does not support both at the
- same time, one of them is arbitrarily going to overwrite
- the other. That will cause spurious failures (or,
- unlikely, passes) of this test.
- EOF
-
- if [[ -z $ALLOW_LLDPAD ]]; then
- cat >/dev/stderr <<-EOF
-
- If you want to run the test anyway, please set
- an environment variable ALLOW_LLDPAD to a
- non-empty string.
- EOF
- exit 1
- else
- return
- fi
- fi
-}
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
index f9858e221996..42ce602d8d49 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
@@ -79,7 +79,6 @@ lib_dir=$(dirname $0)/../../../net/forwarding
NUM_NETIFS=6
source $lib_dir/lib.sh
source $lib_dir/devlink_lib.sh
-source qos_lib.sh
_1KB=1000
_100KB=$((100 * _1KB))
@@ -393,7 +392,7 @@ test_qos_pfc()
log_test "PFC"
}
-bail_on_lldpad
+bail_on_lldpad "configure DCB" "configure Qdiscs"
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh
index ceaa76b17a43..139175fd03e7 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh
@@ -5,7 +5,6 @@
lib_dir=$(dirname $0)/../../../net/forwarding
source $lib_dir/sch_ets_core.sh
source $lib_dir/devlink_lib.sh
-source qos_lib.sh
ALL_TESTS="
ping_ipv4
@@ -78,5 +77,5 @@ collect_stats()
done
}
-bail_on_lldpad
+bail_on_lldpad "configure DCB" "configure Qdiscs"
ets_run
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
index 45b41b8f3232..299e06a5808c 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
@@ -74,7 +74,6 @@ lib_dir=$(dirname $0)/../../../net/forwarding
source $lib_dir/lib.sh
source $lib_dir/devlink_lib.sh
source mlxsw_lib.sh
-source qos_lib.sh
ipaddr()
{
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
index 0d01c7cd82a1..8ecddafa79b3 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
@@ -166,7 +166,7 @@ ecn_mirror_test()
uninstall_qdisc
}
-bail_on_lldpad
+bail_on_lldpad "configure DCB" "configure Qdiscs"
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
index 860205338e6f..159108d02895 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
@@ -73,7 +73,7 @@ red_mirror_test()
uninstall_qdisc
}
-bail_on_lldpad
+bail_on_lldpad "configure DCB" "configure Qdiscs"
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh
index b9b4cdf14ceb..ecc3664376b3 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh
@@ -1,11 +1,9 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
-source qos_lib.sh
-
sch_tbf_pre_hook()
{
- bail_on_lldpad
+ bail_on_lldpad "configure DCB" "configure Qdiscs"
}
lib_dir=$(dirname $0)/../../../net/forwarding
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
index dff9810ee04f..2e0a4efb1703 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
@@ -1,11 +1,9 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
-source qos_lib.sh
-
sch_tbf_pre_hook()
{
- bail_on_lldpad
+ bail_on_lldpad "configure DCB" "configure Qdiscs"
}
lib_dir=$(dirname $0)/../../../net/forwarding
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
index 75406bd7036e..6679a338dfc4 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
@@ -1,11 +1,9 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
-source qos_lib.sh
-
sch_tbf_pre_hook()
{
- bail_on_lldpad
+ bail_on_lldpad "configure DCB" "configure Qdiscs"
}
lib_dir=$(dirname $0)/../../../net/forwarding
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index d47499ba81c7..efd48e1cadd2 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1887,3 +1887,34 @@ mldv1_done_get()
payload_template_expand_checksum "$hbh$icmpv6" $checksum
}
+
+bail_on_lldpad()
+{
+ local reason1="$1"; shift
+ local reason2="$1"; shift
+
+ if systemctl is-active --quiet lldpad; then
+
+ cat >/dev/stderr <<-EOF
+ WARNING: lldpad is running
+
+ lldpad will likely $reason1, and this test will
+ $reason2. Both are not supported at the same time,
+ one of them is arbitrarily going to overwrite the
+ other. That will cause spurious failures (or, unlikely,
+ passes) of this test.
+ EOF
+
+ if [[ -z $ALLOW_LLDPAD ]]; then
+ cat >/dev/stderr <<-EOF
+
+ If you want to run the test anyway, please set
+ an environment variable ALLOW_LLDPAD to a
+ non-empty string.
+ EOF
+ exit 1
+ else
+ return
+ fi
+ fi
+}
--
2.34.1
On Tue, Apr 18, 2023 at 02:14:54PM +0300, Vladimir Oltean wrote:
> These have been useful in debugging various problems related to frame
> preemption, so make them available through ethtool --register-dump for
> later too.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
> ---
> v1->v2: patch is new
>
> .../ethernet/freescale/enetc/enetc_ethtool.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> index 838a92131963..e993ed04ab57 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> @@ -32,6 +32,12 @@ static const u32 enetc_port_regs[] = {
> ENETC_PM0_CMD_CFG, ENETC_PM0_MAXFRM, ENETC_PM0_IF_MODE
> };
>
> +static const u32 enetc_port_mm_regs[] = {
> + ENETC_MMCSR, ENETC_PFPMR, ENETC_PTCFPR(0), ENETC_PTCFPR(1),
> + ENETC_PTCFPR(2), ENETC_PTCFPR(3), ENETC_PTCFPR(4), ENETC_PTCFPR(5),
> + ENETC_PTCFPR(6), ENETC_PTCFPR(7),
> +};
> +
> static int enetc_get_reglen(struct net_device *ndev)
> {
> struct enetc_ndev_priv *priv = netdev_priv(ndev);
> @@ -45,6 +51,9 @@ static int enetc_get_reglen(struct net_device *ndev)
> if (hw->port)
> len += ARRAY_SIZE(enetc_port_regs);
>
> + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU))
nit: I think you could make the condition.
if (hw->port && priv->si->hw_features & ENETC_SI_F_QBU)
which would be consistent with the condition in the next hunk.
> + len += ARRAY_SIZE(enetc_port_mm_regs);
> +
> len *= sizeof(u32) * 2; /* store 2 entries per reg: addr and value */
>
> return len;
> @@ -90,6 +99,14 @@ static void enetc_get_regs(struct net_device *ndev, struct ethtool_regs *regs,
> *buf++ = addr;
> *buf++ = enetc_rd(hw, addr);
> }
> +
> + if (priv->si->hw_features & ENETC_SI_F_QBU) {
> + for (i = 0; i < ARRAY_SIZE(enetc_port_mm_regs); i++) {
> + addr = ENETC_PORT_BASE + enetc_port_mm_regs[i];
> + *buf++ = addr;
> + *buf++ = enetc_rd(hw, addr);
> + }
> + }
> }
>
> static const struct {
> --
> 2.34.1
>
On Tue, Apr 18, 2023 at 02:14:55PM +0300, Vladimir Oltean wrote:
> The verify-enabled boolean (ETHTOOL_A_MM_VERIFY_ENABLED) was intended to
> be a sub-setting of tx-enabled (ETHTOOL_A_MM_TX_ENABLED). IOW, MAC Merge
> TX can be enabled with or without verification, but verification with TX
> disabled makes no sense.
>
> The pmac-enabled boolean (ETHTOOL_A_MM_PMAC_ENABLED) was intended to be
> a global toggle from an API perspective, whereas tx-enabled just handles
> the TX direction. IOW, the pMAC can be enabled with or without TX, but
> it doesn't make sense to enable TX if the pMAC is not enabled.
>
> Add two checks which sanitize and reject these invalid cases.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Tue, Apr 18, 2023 at 02:14:53PM +0300, Vladimir Oltean wrote:
> This was left as TODO in commit 01e23b2b3bad ("net: enetc: add support
> for preemptible traffic classes") since it's relatively complicated.
>
> Where this makes a difference is with a configuration as follows:
>
> ethtool --set-mm eno0 pmac-enabled on tx-enabled on verify-enabled on
>
> Preemptible packets should only be sent when the MAC Merge TX direction
> becomes active (i.o.w. when the verification process succeeds, aka when
> the link partner confirms it can process preemptible traffic). But the
> tc qdisc with the preemptible traffic classes is offloaded completely
> asynchronously w.r.t. the MM becoming active.
>
> The ENETC manual does suggest that this should be handled in the driver:
> "On startup, software should wait for the verification process to
> complete (MMCSR[VSTS]=011) before initiating traffic".
>
> Adding the necessary logic allows future selftests to uphold the claim
> that an inactive or disabled MAC Merge layer should never send data
> packets through the pMAC.
>
> This change moves enetc_set_ptcfpr() from enetc.c to enetc_ethtool.c,
> where its only caller is now - enetc_mm_commit_preemptible_tcs().
>
> Signed-off-by: Vladimir Oltean <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
> int enetc_qos_query_caps(struct net_device *ndev, void *type_data);
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> index deb674752851..838a92131963 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> @@ -991,6 +991,64 @@ static int enetc_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> return 0;
> }
>
> +static int enetc_mm_wait_tx_active(struct enetc_hw *hw, int verify_time)
> +{
> + int timeout = verify_time * USEC_PER_MSEC * ENETC_MM_VERIFY_RETRIES;
> + u32 val;
> +
> + /* This will time out after the standard value of 3 verification
> + * attempts. To not sleep forever, it relies on a non-zero verify_time,
> + * guarantee which is provided by the ethtool nlattr policy.
> + */
> + return read_poll_timeout(enetc_port_rd, val,
> + ENETC_MMCSR_GET_VSTS(val) == 3,
nit: 3 is doing a lot of work here.
As a follow-up, perhaps it could become part of an enum?
> + ENETC_MM_VERIFY_SLEEP_US, timeout,
> + true, hw, ENETC_MMCSR);
> +}
...
On Thu, Apr 20, 2023 at 04:42:52PM +0200, Simon Horman wrote:
> > + /* This will time out after the standard value of 3 verification
> > + * attempts. To not sleep forever, it relies on a non-zero verify_time,
> > + * guarantee which is provided by the ethtool nlattr policy.
> > + */
> > + return read_poll_timeout(enetc_port_rd, val,
> > + ENETC_MMCSR_GET_VSTS(val) == 3,
>
> nit: 3 is doing a lot of work here.
> As a follow-up, perhaps it could become part of an enum?
IMHO it's easy to abuse enums, when numbers could do just fine. I think
that in context (seeing the entire enetc_ethtool.c), this is not as bad
as just this patch makes it to be, because the other occurrence of
ENETC_MMCSR_GET_VSTS() is:
switch (ENETC_MMCSR_GET_VSTS(val)) {
case 0:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
break;
case 2:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
break;
case 3:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
break;
case 4:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
break;
case 5:
default:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
break;
}
so it's immediately clear what the 3 represents (in vim I just press '*'
to see the other occurrences of ENETC_MMCSR_GET_VSTS).
I considered it, but I don't feel an urgent necessity to add an enum here.
Doing that would essentially transform the code into:
return read_poll_timeout(enetc_port_rd, val,
ENETC_MMCSR_GET_VSTS(val) == ENETC_MM_VSTS_SUCCEEDED,
switch (ENETC_MMCSR_GET_VSTS(val)) {
case ENETC_MMCSR_VSTS_DISABLED:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
break;
case ENETC_MMCSR_VSTS_VERIFYING:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
break;
case ENETC_MMCSR_VSTS_SUCCEEDED:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
break;
case ENETC_MMCSR_VSTS_FAILED:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
break;
case ENETC_MMCSR_VSTS_UNKNOWN:
default:
state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
break;
}
which to my eye is more bloated.
On Thu, Apr 20, 2023 at 07:34:53PM +0300, Vladimir Oltean wrote:
> On Thu, Apr 20, 2023 at 04:42:52PM +0200, Simon Horman wrote:
> > > + /* This will time out after the standard value of 3 verification
> > > + * attempts. To not sleep forever, it relies on a non-zero verify_time,
> > > + * guarantee which is provided by the ethtool nlattr policy.
> > > + */
> > > + return read_poll_timeout(enetc_port_rd, val,
> > > + ENETC_MMCSR_GET_VSTS(val) == 3,
> >
> > nit: 3 is doing a lot of work here.
> > As a follow-up, perhaps it could become part of an enum?
>
> IMHO it's easy to abuse enums, when numbers could do just fine. I think
> that in context (seeing the entire enetc_ethtool.c), this is not as bad
> as just this patch makes it to be, because the other occurrence of
> ENETC_MMCSR_GET_VSTS() is:
>
> switch (ENETC_MMCSR_GET_VSTS(val)) {
> case 0:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> break;
> case 2:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
> break;
> case 3:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
> break;
> case 4:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> break;
> case 5:
> default:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
> break;
> }
>
> so it's immediately clear what the 3 represents (in vim I just press '*'
> to see the other occurrences of ENETC_MMCSR_GET_VSTS).
Thanks.
I did see the code above, and I do agree it is informational
wrt the meaning of the values.
> I considered it, but I don't feel an urgent necessity to add an enum here.
> Doing that would essentially transform the code into:
>
> return read_poll_timeout(enetc_port_rd, val,
> ENETC_MMCSR_GET_VSTS(val) == ENETC_MM_VSTS_SUCCEEDED,
>
> switch (ENETC_MMCSR_GET_VSTS(val)) {
> case ENETC_MMCSR_VSTS_DISABLED:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> break;
> case ENETC_MMCSR_VSTS_VERIFYING:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
> break;
> case ENETC_MMCSR_VSTS_SUCCEEDED:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
> break;
> case ENETC_MMCSR_VSTS_FAILED:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> break;
> case ENETC_MMCSR_VSTS_UNKNOWN:
> default:
> state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
> break;
> }
>
> which to my eye is more bloated.
I guess it's subjective.
I certainly don't feel strongly about this.
And I appreciate you taking the time to respond to my idea.
I have no objections to leaving this patch as is (with '3').
On Thu, Apr 20, 2023 at 04:38:00PM +0200, Simon Horman wrote:
> > + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU))
>
> nit: I think you could make the condition.
>
> if (hw->port && priv->si->hw_features & ENETC_SI_F_QBU)
>
> which would be consistent with the condition in the next hunk.
>
> > + if (priv->si->hw_features & ENETC_SI_F_QBU) {
Maybe, but it generates the exact same object code (tested with
"make drivers/net/ethernet/freescale/enetc/enetc_ethtool.lst").
When I'm debugging, I'm a bit of a conspiracy theorist when it comes
to operator precedence (& vs &&), and so, "A && B & C" doesn't read
particularly well to me, and would be one of my first suspects at
hiding a bug. I do know it would have worked in this case though,
and that modern gcc/clang usually complains about suspicious/
unintuitive precedence.
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Tue, 18 Apr 2023 14:14:50 +0300 you wrote:
> This series consolidates the behavior of the 2 drivers that implement
> the ethtool MAC Merge layer by making NXP ENETC commit its preemptible
> traffic classes to hardware only when MM TX is active (same as Ocelot).
>
> Then, after resolving an issue with the ENETC driver, it restricts user
> space from entering 2 states which don't make sense:
>
> [...]
Here is the summary with links:
- [v2,net-next,1/9] net: enetc: fix MAC Merge layer remaining enabled until a link down event
https://git.kernel.org/netdev/net-next/c/59be75db5966
- [v2,net-next,2/9] net: enetc: report mm tx-active based on tx-enabled and verify-status
https://git.kernel.org/netdev/net-next/c/153b5b1d030d
- [v2,net-next,3/9] net: enetc: only commit preemptible TCs to hardware when MM TX is active
https://git.kernel.org/netdev/net-next/c/827145392a4a
- [v2,net-next,4/9] net: enetc: include MAC Merge / FP registers in register dump
https://git.kernel.org/netdev/net-next/c/16a2c7634442
- [v2,net-next,5/9] net: ethtool: mm: sanitize some UAPI configurations
https://git.kernel.org/netdev/net-next/c/35b288d6e3d4
- [v2,net-next,6/9] selftests: forwarding: sch_tbf_*: Add a pre-run hook
https://git.kernel.org/netdev/net-next/c/54e906f1639e
- [v2,net-next,7/9] selftests: forwarding: generalize bail_on_lldpad from mlxsw
https://git.kernel.org/netdev/net-next/c/8fcac79270ca
- [v2,net-next,8/9] selftests: forwarding: introduce helper for standard ethtool counters
https://git.kernel.org/netdev/net-next/c/b5bf7126a6a0
- [v2,net-next,9/9] selftests: forwarding: add a test for MAC Merge layer
https://git.kernel.org/netdev/net-next/c/e6991384ace5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Thu, Apr 20, 2023 at 07:58:52PM +0300, Vladimir Oltean wrote:
> On Thu, Apr 20, 2023 at 04:38:00PM +0200, Simon Horman wrote:
> > > + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU))
> >
> > nit: I think you could make the condition.
> >
> > if (hw->port && priv->si->hw_features & ENETC_SI_F_QBU)
> >
> > which would be consistent with the condition in the next hunk.
> >
> > > + if (priv->si->hw_features & ENETC_SI_F_QBU) {
>
> Maybe, but it generates the exact same object code (tested with
> "make drivers/net/ethernet/freescale/enetc/enetc_ethtool.lst").
>
> When I'm debugging, I'm a bit of a conspiracy theorist when it comes
> to operator precedence (& vs &&), and so, "A && B & C" doesn't read
> particularly well to me, and would be one of my first suspects at
> hiding a bug. I do know it would have worked in this case though,
> and that modern gcc/clang usually complains about suspicious/
> unintuitive precedence.
Thanks, I guess it's subjective.
And I do understand your point regarding & vs &&.
No need to resend because of this
(or update the code at all if that is your choice).