Currently the ethtool --set-mm API permits the existence of 2
configurations which don't make sense:
- pmac-enabled false tx-enabled true
- tx-enabled false verify-enabled true
By rejecting these, we can give driver-level code more guarantees.
I re-ran the MM selftest posted here (which I need to repost):
https://lore.kernel.org/netdev/[email protected]/
and it didn't cause functional problems.
Vladimir Oltean (2):
net: enetc: fix MAC Merge layer remaining enabled until a link down
event
net: ethtool: mm: sanitize some UAPI configurations
drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 11 +++++++----
net/ethtool/mm.c | 5 +++++
2 files changed, 12 insertions(+), 4 deletions(-)
--
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]>
---
net/ethtool/mm.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
index e00d7d5cea7e..0eb81231f342 100644
--- a/net/ethtool/mm.c
+++ b/net/ethtool/mm.c
@@ -214,6 +214,11 @@ static int ethnl_set_mm(struct ethnl_req_info *req_info, struct genl_info *info)
return -ERANGE;
}
+ 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
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 <[email protected]>
---
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);
--
2.34.1
On Sat, Apr 15, 2023 at 08:34:52PM +0300, Vladimir Oltean wrote:
> Currently the ethtool --set-mm API permits the existence of 2
> configurations which don't make sense:
>
> - pmac-enabled false tx-enabled true
> - tx-enabled false verify-enabled true
>
> By rejecting these, we can give driver-level code more guarantees.
> I re-ran the MM selftest posted here (which I need to repost):
> https://lore.kernel.org/netdev/[email protected]/
>
> and it didn't cause functional problems.
Actually, it looks like that selftest passed by mistake in the
configuration that I tested it in. I actually get these failures:
~/selftests/net/forwarding# journalctl -b -u lldpad
Apr 15 18:05:10 lldpad[705]: arg_path "tlvid00120f07.addFragSize"
Apr 15 18:05:10 lldpad[705]: arg_path "tlvid00120f07.addFragSize"
Apr 15 18:05:10 lldpad[705]: arg_path "tlvid00120f07.addFragSize"
Apr 15 18:05:10 lldpad[705]: arg_path "tlvid00120f07.addFragSize"
Apr 15 18:05:10 lldpad[705]: Signal 15 received - terminating
Apr 15 18:05:10 lldpad[705]: ethtool: kernel reports: TX enabled requires pMAC enabled
Apr 15 18:05:10 lldpad[705]: ethtool: kernel reports: TX enabled requires pMAC enabled
Apr 15 18:05:10 systemd[1]: lldpad.service: Deactivated successfully.
Apr 15 18:05:10 systemd[1]: Stopped Link Layer Discovery Protocol Agent Daemon..
Please disregard at least patch 2. Patch 1 is still perfectly valid as-is.