Current enetc_set_mm() is designed to set the priv->active_offloads bit ENETC_F_QBU for enetc_mm_link_state_update() to act on, but if the link is already up, it modifies the ENETC_MMCSR_ME ("Merge Enable") bit directly.

The problem is that it only *sets* ENETC_MMCSR_ME if the link is up, it doesn't *clear* it if needed. So subsequent enetc_get_mm() calls still see tx-enabled as true, up until a link down event, which is when enetc_mm_link_state_update() will get called.

This is not a functional issue as far as I can assess. It has only come up because I'd like to uphold a simple API rule in core ethtool code: the pMAC cannot be disabled if TX is going to be enabled. Currently, the fact that TX remains enabled for longer than expected (after the enetc_set_mm() call that disables it) is going to violate that rule, which is how it was caught.

Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- v1->v2: none

drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c index 838750a03cf6..ee1ea71fe79e 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c @@ -1041,10 +1041,13 @@ static int enetc_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg, else priv->active_offloads &= ~ENETC_F_QBU;

- /* If link is up, enable MAC Merge right away */ - if (!!(priv->active_offloads & ENETC_F_QBU) && - !(val & ENETC_MMCSR_LINK_FAIL)) - val |= ENETC_MMCSR_ME; + /* If link is up, enable/disable MAC Merge right away */ + if (!(val & ENETC_MMCSR_LINK_FAIL)) { + if (!!(priv->active_offloads & ENETC_F_QBU)) + val |= ENETC_MMCSR_ME; + else + val &= ~ENETC_MMCSR_ME; + }

val &= ~ENETC_MMCSR_VT_MASK; val |= ENETC_MMCSR_VT(cfg->verify_time);

The MMCSR register contains 2 fields with overlapping meaning:

- LPA (Local preemption active): This read-only status bit indicates whether preemption is active for this port. This bit will be set if preemption is both enabled and has completed the verification process. - TXSTS (Merge status): This read-only status field provides the state of the MAC Merge sublayer transmit status as defined in IEEE Std 802.3-2018 Clause 99. 00 Transmit preemption is inactive 01 Transmit preemption is active 10 Reserved 11 Reserved

However none of these 2 fields offer reliable reporting to software.

When connecting ENETC to a link partner which is not capable of Frame Preemption, the expectation is that ENETC's verification should fail (VSTS=4) and its MM TX direction should be inactive (LPA=0, TXSTS=00) even though the MM TX is enabled (ME=1). But surprise, the LPA bit of MMCSR stays set even if VSTS=4 and ME=1.

OTOH, the TXSTS field has the opposite problem. I cannot get its value to change from 0, even when connecting to a link partner capable of frame preemption, which does respond to its verification frames (ME=1 and VSTS=3, "SUCCEEDED").

The only option with such buggy hardware seems to be to reimplement the formula for calculating tx-active in software, which is for tx-enabled to be true, and for the verify-status to be either SUCCEEDED, or DISABLED.

Without reliable tx-active reporting, we have no good indication when to commit the preemptible traffic classes to hardware, which makes it possible (but not desirable) to send preemptible traffic to a link partner incapable of receiving it. However, currently we do not have the logic to wait for TX to be active yet, so the impact is limited.

Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- v1->v2: patch is new

drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c index ee1ea71fe79e..deb674752851 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c @@ -976,7 +976,9 @@ static int enetc_get_mm(struct net_device *ndev, struct ethtool_mm_state *state) lafs = ENETC_MMCSR_GET_LAFS(val); state->rx_min_frag_size = ethtool_mm_frag_size_add_to_min(lafs); state->tx_enabled = !!(val & ENETC_MMCSR_LPE); /* mirror of MMCSR_ME */ - state->tx_active = !!(val & ENETC_MMCSR_LPA); + state->tx_active = state->tx_enabled && + (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED || + state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED); state->verify_enabled = !(val & ENETC_MMCSR_VDIS); state->verify_time = ENETC_MMCSR_GET_VT(val); /* A verifyTime of 128 ms would exceed the 7 bit width

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 vladimir.oltean@nxp.com --- 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

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 vladimir.oltean@nxp.com --- 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 {

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 vladimir.oltean@nxp.com --- 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; }

From: Petr Machata petrm@nvidia.com

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 petrm@nvidia.com Reviewed-by: Danielle Ratson danieller@nvidia.com Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- 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

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 vladimir.oltean@nxp.com --- 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