[PATCH v2 net 0/6] Fixes for NXP ENETC driver

[PATCH v2 net 0/6] Fixes for NXP ENETC driver

[Vladimir Oltean]

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

This contains an assorted set of fixes collected over the past week on
the enetc driver. Some are related to VLAN processing, some to physical
link settings, some are fixups of previous hardware workarounds.

Vladimir Oltean (6):
  net: enetc: don't overwrite the RSS indirection table when
    initializing
  net: enetc: initialize RFS/RSS memories for unused ports too
  net: enetc: take the MDIO lock only once per NAPI poll cycle
  net: enetc: fix incorrect TPID when receiving 802.1ad tagged packets
  net: enetc: don't disable VLAN filtering in IFF_PROMISC mode
  net: enetc: force the RGMII speed and duplex instead of operating in
    inband mode

 drivers/net/ethernet/freescale/enetc/enetc.c  | 130 +++++-------------
 drivers/net/ethernet/freescale/enetc/enetc.h  |   5 +
 .../net/ethernet/freescale/enetc/enetc_cbdr.c |  54 ++++++++
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  18 ++-
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 103 +++++++++++---
 .../net/ethernet/freescale/enetc/enetc_vf.c   |   7 +
 6 files changed, 203 insertions(+), 114 deletions(-)

-- 
2.25.1

[PATCH v2 net 1/6] net: enetc: don't overwrite the RSS indirection table when initializing

[Vladimir Oltean]

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

After the blamed patch, all RX traffic gets hashed to CPU 0 because the
hashing indirection table set up in:

enetc_pf_probe
-> enetc_alloc_si_resources
   -> enetc_configure_si
      -> enetc_setup_default_rss_table

is overwritten later in:

enetc_pf_probe
-> enetc_init_port_rss_memory

which zero-initializes the entire port RSS table in order to avoid ECC errors.

The trouble really is that enetc_init_port_rss_memory really neads
enetc_alloc_si_resources to be called, because it depends upon
enetc_alloc_cbdr and enetc_setup_cbdr. But that whole enetc_configure_si
thing could have been better thought out, it has nothing to do in a
function called "alloc_si_resources", especially since its counterpart,
"free_si_resources", does nothing to unwind the configuration of the SI.

The point is, we need to pull out enetc_configure_si out of
enetc_alloc_resources, and move it after enetc_init_port_rss_memory.
This allows us to set up the default RSS indirection table after
initializing the memory.

Fixes: 07bf34a50e32 ("net: enetc: initialize the RFS and RSS memories")
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

 drivers/net/ethernet/freescale/enetc/enetc.c    | 11 +++--------
 drivers/net/ethernet/freescale/enetc/enetc.h    |  1 +
 drivers/net/ethernet/freescale/enetc/enetc_pf.c |  7 +++++++
 drivers/net/ethernet/freescale/enetc/enetc_vf.c |  7 +++++++
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index c78d12229730..fdb6b9e8da78 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1058,13 +1058,12 @@ static int enetc_setup_default_rss_table(struct enetc_si *si, int num_groups)
 	return 0;
 }
 
-static int enetc_configure_si(struct enetc_ndev_priv *priv)
+int enetc_configure_si(struct enetc_ndev_priv *priv)
 {
 	struct enetc_si *si = priv->si;
 	struct enetc_hw *hw = &si->hw;
 	int err;
 
-	enetc_setup_cbdr(hw, &si->cbd_ring);
 	/* set SI cache attributes */
 	enetc_wr(hw, ENETC_SICAR0,
 		 ENETC_SICAR_RD_COHERENT | ENETC_SICAR_WR_COHERENT);
@@ -1112,6 +1111,8 @@ int enetc_alloc_si_resources(struct enetc_ndev_priv *priv)
 	if (err)
 		return err;
 
+	enetc_setup_cbdr(&si->hw, &si->cbd_ring);
+
 	priv->cls_rules = kcalloc(si->num_fs_entries, sizeof(*priv->cls_rules),
 				  GFP_KERNEL);
 	if (!priv->cls_rules) {
@@ -1119,14 +1120,8 @@ int enetc_alloc_si_resources(struct enetc_ndev_priv *priv)
 		goto err_alloc_cls;
 	}
 
-	err = enetc_configure_si(priv);
-	if (err)
-		goto err_config_si;
-
 	return 0;
 
-err_config_si:
-	kfree(priv->cls_rules);
 err_alloc_cls:
 	enetc_clear_cbdr(&si->hw);
 	enetc_free_cbdr(priv->dev, &si->cbd_ring);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 8532d23b54f5..f8275cef3b5c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -292,6 +292,7 @@ void enetc_get_si_caps(struct enetc_si *si);
 void enetc_init_si_rings_params(struct enetc_ndev_priv *priv);
 int enetc_alloc_si_resources(struct enetc_ndev_priv *priv);
 void enetc_free_si_resources(struct enetc_ndev_priv *priv);
+int enetc_configure_si(struct enetc_ndev_priv *priv);
 
 int enetc_open(struct net_device *ndev);
 int enetc_close(struct net_device *ndev);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 515c5b29d7aa..d02ecb2e46ae 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -1108,6 +1108,12 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 		goto err_init_port_rss;
 	}
 
+	err = enetc_configure_si(priv);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to configure SI\n");
+		goto err_config_si;
+	}
+
 	err = enetc_alloc_msix(priv);
 	if (err) {
 		dev_err(&pdev->dev, "MSIX alloc failed\n");
@@ -1136,6 +1142,7 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 	enetc_mdiobus_destroy(pf);
 err_mdiobus_create:
 	enetc_free_msix(priv);
+err_config_si:
 err_init_port_rss:
 err_init_port_rfs:
 err_alloc_msix:
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
index 39c1a09e69a9..9b755a84c2d6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
@@ -171,6 +171,12 @@ static int enetc_vf_probe(struct pci_dev *pdev,
 		goto err_alloc_si_res;
 	}
 
+	err = enetc_configure_si(priv);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to configure SI\n");
+		goto err_config_si;
+	}
+
 	err = enetc_alloc_msix(priv);
 	if (err) {
 		dev_err(&pdev->dev, "MSIX alloc failed\n");
@@ -187,6 +193,7 @@ static int enetc_vf_probe(struct pci_dev *pdev,
 
 err_reg_netdev:
 	enetc_free_msix(priv);
+err_config_si:
 err_alloc_msix:
 	enetc_free_si_resources(priv);
 err_alloc_si_res:
-- 
2.25.1

[PATCH v2 net 2/6] net: enetc: initialize RFS/RSS memories for unused ports too

[Vladimir Oltean]

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

Michael reports that since linux-next-20210211, the AER messages for ECC
errors have started reappearing, and this time they can be reliably
reproduced with the first ping on one of his LS1028A boards.

$ ping 1[   33.258069] pcieport 0000:00:1f.0: AER: Multiple Corrected error received: 0000:00:00.0
72.16.0.1
PING [   33.267050] pcieport 0000:00:1f.0: AER: can't find device of ID0000
172.16.0.1 (172.16.0.1): 56 data bytes
64 bytes from 172.16.0.1: seq=0 ttl=64 time=17.124 ms
64 bytes from 172.16.0.1: seq=1 ttl=64 time=0.273 ms

$ devmem 0x1f8010e10 32
0xC0000006

It isn't clear why this is necessary, but it seems that for the errors
to go away, we must clear the entire RFS and RSS memory, not just for
the ports in use.

Sadly the code is structured in such a way that we can't have unified
logic for the used and unused ports. For the minimal initialization of
an unused port, we need just to enable and ioremap the PF memory space,
and a control buffer descriptor ring. Unused ports must then free the
CBDR because the driver will exit, but used ports can not pick up from
where that code path left, since the CBDR API does not reinitialize a
ring when setting it up, so its producer and consumer indices are out of
sync between the software and hardware state. So a separate
enetc_init_unused_port function was created, and it gets called right
after the PF memory space is enabled.

Note that we need access from enetc_pf.c to the CBDR creation and
deletion methods, which were for some reason put in enetc.c. While
changing their definitions to be non-static, also move them to
enetc_cbdr.c which seems like a better place to hold these.

Fixes: 07bf34a50e32 ("net: enetc: initialize the RFS and RSS memories")
Reported-by: Michael Walle <michael@walle.cc>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

 drivers/net/ethernet/freescale/enetc/enetc.c  | 54 -------------------
 drivers/net/ethernet/freescale/enetc/enetc.h  |  4 ++
 .../net/ethernet/freescale/enetc/enetc_cbdr.c | 54 +++++++++++++++++++
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 33 ++++++++++--
 4 files changed, 86 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index fdb6b9e8da78..43f0fae30080 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -984,60 +984,6 @@ static void enetc_free_rxtx_rings(struct enetc_ndev_priv *priv)
 		enetc_free_tx_ring(priv->tx_ring[i]);
 }
 
-static int enetc_alloc_cbdr(struct device *dev, struct enetc_cbdr *cbdr)
-{
-	int size = cbdr->bd_count * sizeof(struct enetc_cbd);
-
-	cbdr->bd_base = dma_alloc_coherent(dev, size, &cbdr->bd_dma_base,
-					   GFP_KERNEL);
-	if (!cbdr->bd_base)
-		return -ENOMEM;
-
-	/* h/w requires 128B alignment */
-	if (!IS_ALIGNED(cbdr->bd_dma_base, 128)) {
-		dma_free_coherent(dev, size, cbdr->bd_base, cbdr->bd_dma_base);
-		return -EINVAL;
-	}
-
-	cbdr->next_to_clean = 0;
-	cbdr->next_to_use = 0;
-
-	return 0;
-}
-
-static void enetc_free_cbdr(struct device *dev, struct enetc_cbdr *cbdr)
-{
-	int size = cbdr->bd_count * sizeof(struct enetc_cbd);
-
-	dma_free_coherent(dev, size, cbdr->bd_base, cbdr->bd_dma_base);
-	cbdr->bd_base = NULL;
-}
-
-static void enetc_setup_cbdr(struct enetc_hw *hw, struct enetc_cbdr *cbdr)
-{
-	/* set CBDR cache attributes */
-	enetc_wr(hw, ENETC_SICAR2,
-		 ENETC_SICAR_RD_COHERENT | ENETC_SICAR_WR_COHERENT);
-
-	enetc_wr(hw, ENETC_SICBDRBAR0, lower_32_bits(cbdr->bd_dma_base));
-	enetc_wr(hw, ENETC_SICBDRBAR1, upper_32_bits(cbdr->bd_dma_base));
-	enetc_wr(hw, ENETC_SICBDRLENR, ENETC_RTBLENR_LEN(cbdr->bd_count));
-
-	enetc_wr(hw, ENETC_SICBDRPIR, 0);
-	enetc_wr(hw, ENETC_SICBDRCIR, 0);
-
-	/* enable ring */
-	enetc_wr(hw, ENETC_SICBDRMR, BIT(31));
-
-	cbdr->pir = hw->reg + ENETC_SICBDRPIR;
-	cbdr->cir = hw->reg + ENETC_SICBDRCIR;
-}
-
-static void enetc_clear_cbdr(struct enetc_hw *hw)
-{
-	enetc_wr(hw, ENETC_SICBDRMR, 0);
-}
-
 static int enetc_setup_default_rss_table(struct enetc_si *si, int num_groups)
 {
 	int *rss_table;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index f8275cef3b5c..8b380fc13314 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -310,6 +310,10 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
 void enetc_set_ethtool_ops(struct net_device *ndev);
 
 /* control buffer descriptor ring (CBDR) */
+int enetc_alloc_cbdr(struct device *dev, struct enetc_cbdr *cbdr);
+void enetc_free_cbdr(struct device *dev, struct enetc_cbdr *cbdr);
+void enetc_setup_cbdr(struct enetc_hw *hw, struct enetc_cbdr *cbdr);
+void enetc_clear_cbdr(struct enetc_hw *hw);
 int enetc_set_mac_flt_entry(struct enetc_si *si, int index,
 			    char *mac_addr, int si_map);
 int enetc_clear_mac_flt_entry(struct enetc_si *si, int index);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
index 201cbc362e33..ad6aecda6b47 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
@@ -3,6 +3,60 @@
 
 #include "enetc.h"
 
+int enetc_alloc_cbdr(struct device *dev, struct enetc_cbdr *cbdr)
+{
+	int size = cbdr->bd_count * sizeof(struct enetc_cbd);
+
+	cbdr->bd_base = dma_alloc_coherent(dev, size, &cbdr->bd_dma_base,
+					   GFP_KERNEL);
+	if (!cbdr->bd_base)
+		return -ENOMEM;
+
+	/* h/w requires 128B alignment */
+	if (!IS_ALIGNED(cbdr->bd_dma_base, 128)) {
+		dma_free_coherent(dev, size, cbdr->bd_base, cbdr->bd_dma_base);
+		return -EINVAL;
+	}
+
+	cbdr->next_to_clean = 0;
+	cbdr->next_to_use = 0;
+
+	return 0;
+}
+
+void enetc_free_cbdr(struct device *dev, struct enetc_cbdr *cbdr)
+{
+	int size = cbdr->bd_count * sizeof(struct enetc_cbd);
+
+	dma_free_coherent(dev, size, cbdr->bd_base, cbdr->bd_dma_base);
+	cbdr->bd_base = NULL;
+}
+
+void enetc_setup_cbdr(struct enetc_hw *hw, struct enetc_cbdr *cbdr)
+{
+	/* set CBDR cache attributes */
+	enetc_wr(hw, ENETC_SICAR2,
+		 ENETC_SICAR_RD_COHERENT | ENETC_SICAR_WR_COHERENT);
+
+	enetc_wr(hw, ENETC_SICBDRBAR0, lower_32_bits(cbdr->bd_dma_base));
+	enetc_wr(hw, ENETC_SICBDRBAR1, upper_32_bits(cbdr->bd_dma_base));
+	enetc_wr(hw, ENETC_SICBDRLENR, ENETC_RTBLENR_LEN(cbdr->bd_count));
+
+	enetc_wr(hw, ENETC_SICBDRPIR, 0);
+	enetc_wr(hw, ENETC_SICBDRCIR, 0);
+
+	/* enable ring */
+	enetc_wr(hw, ENETC_SICBDRMR, BIT(31));
+
+	cbdr->pir = hw->reg + ENETC_SICBDRPIR;
+	cbdr->cir = hw->reg + ENETC_SICBDRCIR;
+}
+
+void enetc_clear_cbdr(struct enetc_hw *hw)
+{
+	enetc_wr(hw, ENETC_SICBDRMR, 0);
+}
+
 static void enetc_clean_cbdr(struct enetc_si *si)
 {
 	struct enetc_cbdr *ring = &si->cbd_ring;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index d02ecb2e46ae..62ba4bf56f0d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -1041,6 +1041,26 @@ static int enetc_init_port_rss_memory(struct enetc_si *si)
 	return err;
 }
 
+static void enetc_init_unused_port(struct enetc_si *si)
+{
+	struct device *dev = &si->pdev->dev;
+	struct enetc_hw *hw = &si->hw;
+	int err;
+
+	si->cbd_ring.bd_count = ENETC_CBDR_DEFAULT_SIZE;
+	err = enetc_alloc_cbdr(dev, &si->cbd_ring);
+	if (err)
+		return;
+
+	enetc_setup_cbdr(hw, &si->cbd_ring);
+
+	enetc_init_port_rfs_memory(si);
+	enetc_init_port_rss_memory(si);
+
+	enetc_clear_cbdr(hw);
+	enetc_free_cbdr(dev, &si->cbd_ring);
+}
+
 static int enetc_pf_probe(struct pci_dev *pdev,
 			  const struct pci_device_id *ent)
 {
@@ -1051,11 +1071,6 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 	struct enetc_pf *pf;
 	int err;
 
-	if (node && !of_device_is_available(node)) {
-		dev_info(&pdev->dev, "device is disabled, skipping\n");
-		return -ENODEV;
-	}
-
 	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
 	if (err) {
 		dev_err(&pdev->dev, "PCI probing failed\n");
@@ -1069,6 +1084,13 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 		goto err_map_pf_space;
 	}
 
+	if (node && !of_device_is_available(node)) {
+		enetc_init_unused_port(si);
+		dev_info(&pdev->dev, "device is disabled, skipping\n");
+		err = -ENODEV;
+		goto err_device_disabled;
+	}
+
 	pf = enetc_si_priv(si);
 	pf->si = si;
 	pf->total_vfs = pci_sriov_get_totalvfs(pdev);
@@ -1151,6 +1173,7 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 	si->ndev = NULL;
 	free_netdev(ndev);
 err_alloc_netdev:
+err_device_disabled:
 err_map_pf_space:
 	enetc_pci_remove(pdev);
 
-- 
2.25.1

Re: [PATCH v2 net 2/6] net: enetc: initialize RFS/RSS memories for unused ports too

[Michael Walle]

Am 2021-02-25 13:18, schrieb Vladimir Oltean:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Michael reports that since linux-next-20210211, the AER messages for 
> ECC
> errors have started reappearing, and this time they can be reliably
> reproduced with the first ping on one of his LS1028A boards.
> 
> $ ping 1[   33.258069] pcieport 0000:00:1f.0: AER: Multiple Corrected
> error received: 0000:00:00.0
> 72.16.0.1
> PING [   33.267050] pcieport 0000:00:1f.0: AER: can't find device of 
> ID0000
> 172.16.0.1 (172.16.0.1): 56 data bytes
> 64 bytes from 172.16.0.1: seq=0 ttl=64 time=17.124 ms
> 64 bytes from 172.16.0.1: seq=1 ttl=64 time=0.273 ms
> 
> $ devmem 0x1f8010e10 32
> 0xC0000006
> 
> It isn't clear why this is necessary, but it seems that for the errors
> to go away, we must clear the entire RFS and RSS memory, not just for
> the ports in use.
> 
> Sadly the code is structured in such a way that we can't have unified
> logic for the used and unused ports. For the minimal initialization of
> an unused port, we need just to enable and ioremap the PF memory space,
> and a control buffer descriptor ring. Unused ports must then free the
> CBDR because the driver will exit, but used ports can not pick up from
> where that code path left, since the CBDR API does not reinitialize a
> ring when setting it up, so its producer and consumer indices are out 
> of
> sync between the software and hardware state. So a separate
> enetc_init_unused_port function was created, and it gets called right
> after the PF memory space is enabled.
> 
> Note that we need access from enetc_pf.c to the CBDR creation and
> deletion methods, which were for some reason put in enetc.c. While
> changing their definitions to be non-static, also move them to
> enetc_cbdr.c which seems like a better place to hold these.
> 
> Fixes: 07bf34a50e32 ("net: enetc: initialize the RFS and RSS memories")
> Reported-by: Michael Walle <michael@walle.cc>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I had this patch in my tree for a while now. As we've learned, it
really depends on a particular power-up state for the error to happen.
So take this with a grain of salt: I haven't seen the error anymore,
albeit multiple power-cycles. Thus:

Tested-by: Michael Walle <michael@walle.cc>

[PATCH v2 net 3/6] net: enetc: take the MDIO lock only once per NAPI poll cycle

[Vladimir Oltean]

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

The workaround for the ENETC MDIO erratum caused a performance
degradation of 82 Kpps (seen with IP forwarding of two 1Gbps streams of
64B packets). This is due to excessive locking and unlocking in the fast
path, which can be avoided.

By taking the MDIO read-side lock only once per NAPI poll cycle, we are
able to regain 54 Kpps (65%) of the performance hit. The rest of the
performance degradation comes from the TX data path, but unfortunately
it doesn't look like we can optimize that away easily, even with
netdev_xmit_more(), there just isn't any skb batching done, to help with
taking the MDIO lock less often than once per packet.

Fixes: fd5736bf9f23 ("enetc: Workaround for MDIO register access issue")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

 drivers/net/ethernet/freescale/enetc/enetc.c  | 31 ++++++-------------
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  2 ++
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 43f0fae30080..eebe08a99270 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -281,6 +281,8 @@ static int enetc_poll(struct napi_struct *napi, int budget)
 	int work_done;
 	int i;
 
+	enetc_lock_mdio();
+
 	for (i = 0; i < v->count_tx_rings; i++)
 		if (!enetc_clean_tx_ring(&v->tx_ring[i], budget))
 			complete = false;
@@ -291,8 +293,10 @@ static int enetc_poll(struct napi_struct *napi, int budget)
 	if (work_done)
 		v->rx_napi_work = true;
 
-	if (!complete)
+	if (!complete) {
+		enetc_unlock_mdio();
 		return budget;
+	}
 
 	napi_complete_done(napi, work_done);
 
@@ -301,8 +305,6 @@ static int enetc_poll(struct napi_struct *napi, int budget)
 
 	v->rx_napi_work = false;
 
-	enetc_lock_mdio();
-
 	/* enable interrupts */
 	enetc_wr_reg_hot(v->rbier, ENETC_RBIER_RXTIE);
 
@@ -327,8 +329,8 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd,
 {
 	u32 lo, hi, tstamp_lo;
 
-	lo = enetc_rd(hw, ENETC_SICTR0);
-	hi = enetc_rd(hw, ENETC_SICTR1);
+	lo = enetc_rd_hot(hw, ENETC_SICTR0);
+	hi = enetc_rd_hot(hw, ENETC_SICTR1);
 	tstamp_lo = le32_to_cpu(txbd->wb.tstamp);
 	if (lo <= tstamp_lo)
 		hi -= 1;
@@ -358,9 +360,7 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 	i = tx_ring->next_to_clean;
 	tx_swbd = &tx_ring->tx_swbd[i];
 
-	enetc_lock_mdio();
 	bds_to_clean = enetc_bd_ready_count(tx_ring, i);
-	enetc_unlock_mdio();
 
 	do_tstamp = false;
 
@@ -403,8 +403,6 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 			tx_swbd = tx_ring->tx_swbd;
 		}
 
-		enetc_lock_mdio();
-
 		/* BD iteration loop end */
 		if (is_eof) {
 			tx_frm_cnt++;
@@ -415,8 +413,6 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 
 		if (unlikely(!bds_to_clean))
 			bds_to_clean = enetc_bd_ready_count(tx_ring, i);
-
-		enetc_unlock_mdio();
 	}
 
 	tx_ring->next_to_clean = i;
@@ -660,8 +656,6 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 		u32 bd_status;
 		u16 size;
 
-		enetc_lock_mdio();
-
 		if (cleaned_cnt >= ENETC_RXBD_BUNDLE) {
 			int count = enetc_refill_rx_ring(rx_ring, cleaned_cnt);
 
@@ -672,19 +666,15 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		rxbd = enetc_rxbd(rx_ring, i);
 		bd_status = le32_to_cpu(rxbd->r.lstatus);
-		if (!bd_status) {
-			enetc_unlock_mdio();
+		if (!bd_status)
 			break;
-		}
 
 		enetc_wr_reg_hot(rx_ring->idr, BIT(rx_ring->index));
 		dma_rmb(); /* for reading other rxbd fields */
 		size = le16_to_cpu(rxbd->r.buf_len);
 		skb = enetc_map_rx_buff_to_skb(rx_ring, i, size);
-		if (!skb) {
-			enetc_unlock_mdio();
+		if (!skb)
 			break;
-		}
 
 		enetc_get_offloads(rx_ring, rxbd, skb);
 
@@ -696,7 +686,6 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		if (unlikely(bd_status &
 			     ENETC_RXBD_LSTATUS(ENETC_RXBD_ERR_MASK))) {
-			enetc_unlock_mdio();
 			dev_kfree_skb(skb);
 			while (!(bd_status & ENETC_RXBD_LSTATUS_F)) {
 				dma_rmb();
@@ -736,8 +725,6 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		enetc_process_skb(rx_ring, skb);
 
-		enetc_unlock_mdio();
-
 		napi_gro_receive(napi, skb);
 
 		rx_frm_cnt++;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index c71fe8d751d5..8b54562f5da6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -453,6 +453,8 @@ static inline u64 _enetc_rd_reg64_wa(void __iomem *reg)
 #define enetc_wr_reg(reg, val)		_enetc_wr_reg_wa((reg), (val))
 #define enetc_rd(hw, off)		enetc_rd_reg((hw)->reg + (off))
 #define enetc_wr(hw, off, val)		enetc_wr_reg((hw)->reg + (off), val)
+#define enetc_rd_hot(hw, off)		enetc_rd_reg_hot((hw)->reg + (off))
+#define enetc_wr_hot(hw, off, val)	enetc_wr_reg_hot((hw)->reg + (off), val)
 #define enetc_rd64(hw, off)		_enetc_rd_reg64_wa((hw)->reg + (off))
 /* port register accessors - PF only */
 #define enetc_port_rd(hw, off)		enetc_rd_reg((hw)->port + (off))
-- 
2.25.1

Re: [PATCH v2 net 3/6] net: enetc: take the MDIO lock only once per NAPI poll cycle

[Andrew Lunn]

On Thu, Feb 25, 2021 at 02:18:32PM +0200, Vladimir Oltean wrote:
> @@ -327,8 +329,8 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd,
>  {
>  	u32 lo, hi, tstamp_lo;
>  
> -	lo = enetc_rd(hw, ENETC_SICTR0);
> -	hi = enetc_rd(hw, ENETC_SICTR1);
> +	lo = enetc_rd_hot(hw, ENETC_SICTR0);
> +	hi = enetc_rd_hot(hw, ENETC_SICTR1);
>  	tstamp_lo = le32_to_cpu(txbd->wb.tstamp);
>  	if (lo <= tstamp_lo)
>  		hi -= 1;

Hi Vladimir

This change is not obvious, and there is no mention of it in the
commit message. Please could you explain it. I guess it is to do with
enetc_get_tx_tstamp() being called with the MDIO lock held now, when
it was not before?

Thanks
	Andrew

Re: [PATCH v2 net 3/6] net: enetc: take the MDIO lock only once per NAPI poll cycle

[Vladimir Oltean]

On Thu, Feb 25, 2021 at 11:52:19PM +0100, Andrew Lunn wrote:
> On Thu, Feb 25, 2021 at 02:18:32PM +0200, Vladimir Oltean wrote:
> > @@ -327,8 +329,8 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd,
> >  {
> >  	u32 lo, hi, tstamp_lo;
> >  
> > -	lo = enetc_rd(hw, ENETC_SICTR0);
> > -	hi = enetc_rd(hw, ENETC_SICTR1);
> > +	lo = enetc_rd_hot(hw, ENETC_SICTR0);
> > +	hi = enetc_rd_hot(hw, ENETC_SICTR1);
> >  	tstamp_lo = le32_to_cpu(txbd->wb.tstamp);
> >  	if (lo <= tstamp_lo)
> >  		hi -= 1;
> 
> Hi Vladimir
> 
> This change is not obvious, and there is no mention of it in the
> commit message. Please could you explain it. I guess it is to do with
> enetc_get_tx_tstamp() being called with the MDIO lock held now, when
> it was not before?

I realize this is an uncharacteristically short commit message and I'm
sorry for that, if needed I can resend.

Your assumption is correct, the new call path is:

enetc_msix
-> napi_schedule
   -> enetc_poll
      -> enetc_lock_mdio
      -> enetc_clean_tx_ring
         -> enetc_get_tx_tstamp
      -> enetc_clean_rx_ring
      -> enetc_unlock_mdio

The 'hot' accessors are for normal, 'unlocked' register reads and
writes, while enetc_rd contains enetc_lock_mdio, followed by the actual
read, followed by enetc_unlock_mdio.

The goal is to eventually get rid of all the _hot stuff and always take
the lock from the top level, this would allow us to do more register
read/write batching and that would amortize the cost of the locking
overall.

Re: [PATCH v2 net 3/6] net: enetc: take the MDIO lock only once per NAPI poll cycle

[Vladimir Oltean]

On Fri, Feb 26, 2021 at 01:00:26AM +0200, Vladimir Oltean wrote:
> The goal is to eventually get rid of all the _hot stuff and always take
> the lock from the top level, this would allow us to do more register
> read/write batching and that would amortize the cost of the locking
> overall.

Of course when I say 'get rid of the hot stuff', I mean get rid of the
'hot' _naming_ and not the behavior, since the goal is for all register
accessors to be 'hot' aka unlocked.

[PATCH v2 net 4/6] net: enetc: fix incorrect TPID when receiving 802.1ad tagged packets

[Vladimir Oltean]

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

When the enetc ports have rx-vlan-offload enabled, they report a TPID of
ETH_P_8021Q regardless of what was actually in the packet. When
rx-vlan-offload is disabled, packets have the proper TPID. Fix this
inconsistency by finishing the TODO left in the code.

Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
The "priv" variable needs to be used regardless of CONFIG_FSL_ENETC_PTP_CLOCK
now.

 drivers/net/ethernet/freescale/enetc/enetc.c  | 34 ++++++++++++++-----
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  3 ++
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index eebe08a99270..0064cfe5438e 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -523,9 +523,8 @@ static void enetc_get_rx_tstamp(struct net_device *ndev,
 static void enetc_get_offloads(struct enetc_bdr *rx_ring,
 			       union enetc_rx_bd *rxbd, struct sk_buff *skb)
 {
-#ifdef CONFIG_FSL_ENETC_PTP_CLOCK
 	struct enetc_ndev_priv *priv = netdev_priv(rx_ring->ndev);
-#endif
+
 	/* TODO: hashing */
 	if (rx_ring->ndev->features & NETIF_F_RXCSUM) {
 		u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum);
@@ -534,12 +533,31 @@ static void enetc_get_offloads(struct enetc_bdr *rx_ring,
 		skb->ip_summed = CHECKSUM_COMPLETE;
 	}
 
-	/* copy VLAN to skb, if one is extracted, for now we assume it's a
-	 * standard TPID, but HW also supports custom values
-	 */
-	if (le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_VLAN)
-		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
-				       le16_to_cpu(rxbd->r.vlan_opt));
+	if (le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_VLAN) {
+		__be16 tpid = 0;
+
+		switch (le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_TPID) {
+		case 0:
+			tpid = htons(ETH_P_8021Q);
+			break;
+		case 1:
+			tpid = htons(ETH_P_8021AD);
+			break;
+		case 2:
+			tpid = htons(enetc_port_rd(&priv->si->hw,
+						   ENETC_PCVLANR1));
+			break;
+		case 3:
+			tpid = htons(enetc_port_rd(&priv->si->hw,
+						   ENETC_PCVLANR2));
+			break;
+		default:
+			break;
+		}
+
+		__vlan_hwaccel_put_tag(skb, tpid, le16_to_cpu(rxbd->r.vlan_opt));
+	}
+
 #ifdef CONFIG_FSL_ENETC_PTP_CLOCK
 	if (priv->active_offloads & ENETC_F_RX_TSTAMP)
 		enetc_get_rx_tstamp(rx_ring->ndev, rxbd, skb);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 8b54562f5da6..a62604a1e54e 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -172,6 +172,8 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_PSIPMAR0(n)	(0x0100 + (n) * 0x8) /* n = SI index */
 #define ENETC_PSIPMAR1(n)	(0x0104 + (n) * 0x8)
 #define ENETC_PVCLCTR		0x0208
+#define ENETC_PCVLANR1		0x0210
+#define ENETC_PCVLANR2		0x0214
 #define ENETC_VLAN_TYPE_C	BIT(0)
 #define ENETC_VLAN_TYPE_S	BIT(1)
 #define ENETC_PVCLCTR_OVTPIDL(bmp)	((bmp) & 0xff) /* VLAN_TYPE */
@@ -570,6 +572,7 @@ union enetc_rx_bd {
 #define ENETC_RXBD_LSTATUS(flags)	((flags) << 16)
 #define ENETC_RXBD_FLAG_VLAN	BIT(9)
 #define ENETC_RXBD_FLAG_TSTMP	BIT(10)
+#define ENETC_RXBD_FLAG_TPID	GENMASK(1, 0)
 
 #define ENETC_MAC_ADDR_FILT_CNT	8 /* # of supported entries per port */
 #define EMETC_MAC_ADDR_FILT_RES	3 /* # of reserved entries at the beginning */
-- 
2.25.1

[PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

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

Quoting from the blamed commit:

    In promiscuous mode, it is more intuitive that all traffic is received,
    including VLAN tagged traffic. It appears that it is necessary to set
    the flag in PSIPVMR for that to be the case, so VLAN promiscuous mode is
    also temporarily enabled. On exit from promiscuous mode, the setting
    made by ethtool is restored.

Intuitive or not, there isn't any definition issued by a standards body
which says that promiscuity has anything to do with VLAN filtering - it
only has to do with accepting packets regardless of destination MAC address.

In fact people are already trying to use this misunderstanding/bug of
the enetc driver as a justification to transform promiscuity into
something it never was about: accepting every packet (maybe that would
be the "rx-all" netdev feature?):
https://lore.kernel.org/netdev/20201110153958.ci5ekor3o2ekg3ky@ipetronik.com/

So we should avoid that and delete the bogus logic in enetc.

Fixes: 7070eea5e95a ("enetc: permit configuration of rx-vlan-filter with ethtool")
Cc: Markus Blöchl <Markus.Bloechl@ipetronik.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

 drivers/net/ethernet/freescale/enetc/enetc_pf.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 62ba4bf56f0d..49681a0566ed 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -190,7 +190,6 @@ static void enetc_pf_set_rx_mode(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	struct enetc_pf *pf = enetc_si_priv(priv->si);
-	char vlan_promisc_simap = pf->vlan_promisc_simap;
 	struct enetc_hw *hw = &priv->si->hw;
 	bool uprom = false, mprom = false;
 	struct enetc_mac_filter *filter;
@@ -203,16 +202,12 @@ static void enetc_pf_set_rx_mode(struct net_device *ndev)
 		psipmr = ENETC_PSIPMR_SET_UP(0) | ENETC_PSIPMR_SET_MP(0);
 		uprom = true;
 		mprom = true;
-		/* Enable VLAN promiscuous mode for SI0 (PF) */
-		vlan_promisc_simap |= BIT(0);
 	} else if (ndev->flags & IFF_ALLMULTI) {
 		/* enable multi cast promisc mode for SI0 (PF) */
 		psipmr = ENETC_PSIPMR_SET_MP(0);
 		mprom = true;
 	}
 
-	enetc_set_vlan_promisc(&pf->si->hw, vlan_promisc_simap);
-
 	/* first 2 filter entries belong to PF */
 	if (!uprom) {
 		/* Update unicast filters */
-- 
2.25.1

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Jakub Kicinski]

On Thu, 25 Feb 2021 14:18:34 +0200 Vladimir Oltean wrote:
> Quoting from the blamed commit:
> 
>     In promiscuous mode, it is more intuitive that all traffic is received,
>     including VLAN tagged traffic. It appears that it is necessary to set
>     the flag in PSIPVMR for that to be the case, so VLAN promiscuous mode is
>     also temporarily enabled. On exit from promiscuous mode, the setting
>     made by ethtool is restored.
> 
> Intuitive or not, there isn't any definition issued by a standards body
> which says that promiscuity has anything to do with VLAN filtering - it
> only has to do with accepting packets regardless of destination MAC address.
> 
> In fact people are already trying to use this misunderstanding/bug of
> the enetc driver as a justification to transform promiscuity into
> something it never was about: accepting every packet (maybe that would
> be the "rx-all" netdev feature?):
> https://lore.kernel.org/netdev/20201110153958.ci5ekor3o2ekg3ky@ipetronik.com/
> 
> So we should avoid that and delete the bogus logic in enetc.

I don't understand what you're fixing tho.

Are we trying to establish vlan-filter-on as the expected behavior?

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote:
> I don't understand what you're fixing tho.
>
> Are we trying to establish vlan-filter-on as the expected behavior?

What I'm fixing is unexpected behavior, according to the applicable
standards I could find. If I don't mark this change as a bug fix but as
a simple patch, somebody could claim it's a regression, since promiscuity
used to be enough to see packets with unknown VLANs, and now it no
longer is...

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Jakub Kicinski]

On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote:
> On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote:
> > I don't understand what you're fixing tho.
> >
> > Are we trying to establish vlan-filter-on as the expected behavior?  
> 
> What I'm fixing is unexpected behavior, according to the applicable
> standards I could find. If I don't mark this change as a bug fix but as
> a simple patch, somebody could claim it's a regression, since promiscuity
> used to be enough to see packets with unknown VLANs, and now it no
> longer is...

Can we take it into net-next? What's your feeling on that option?

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote:
> On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote:
> > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote:
> > > I don't understand what you're fixing tho.
> > >
> > > Are we trying to establish vlan-filter-on as the expected behavior?
> >
> > What I'm fixing is unexpected behavior, according to the applicable
> > standards I could find. If I don't mark this change as a bug fix but as
> > a simple patch, somebody could claim it's a regression, since promiscuity
> > used to be enough to see packets with unknown VLANs, and now it no
> > longer is...
>
> Can we take it into net-next? What's your feeling on that option?

I see how you can view this patch as pointless, but there is some
context to it. It isn't just for tcpdump/debugging, instead NXP has some
TSN use cases which involve some asymmetric tc-vlan rules, which is how
I arrived at this topic in the first place. I've already established
that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off:
https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/
and that's what we recommend doing, but while adding the support for
rx-vlan-filter in enetc I accidentally created another possibility for
this to work on enetc, by turning IFF_PROMISC on. This is not portable,
so if somebody develops a solution based on that in parallel, it will
most certainly break on other non-enetc drivers.
NXP has not released a kernel based on the v5.10 stable yet, so there is
still time to change the behavior, but if this goes in through net-next,
the apparent regression will only be visible when the next LTS comes
around (whatever the number of that might be). Now, I'm going to
backport this to the NXP v5.10 anyway, so that's not an issue, but there
will still be the mild annoyance that the upstream v5.10 will behave
differently in this regard compared to the NXP v5.10, which is again a
point of potential confusion, but that seems to be out of my control.

So if you're still "yeah, don't care", then I guess I'm ok with leaving
things alone on stable kernels.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Jakub Kicinski]

On Sat, 27 Feb 2021 02:16:51 +0200 Vladimir Oltean wrote:
> On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote:
> > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote:  
> > > What I'm fixing is unexpected behavior, according to the applicable
> > > standards I could find. If I don't mark this change as a bug fix but as
> > > a simple patch, somebody could claim it's a regression, since promiscuity
> > > used to be enough to see packets with unknown VLANs, and now it no
> > > longer is...  
> >
> > Can we take it into net-next? What's your feeling on that option?  
> 
> I see how you can view this patch as pointless, but there is some
> context to it. It isn't just for tcpdump/debugging, instead NXP has some
> TSN use cases which involve some asymmetric tc-vlan rules, which is how
> I arrived at this topic in the first place. I've already established
> that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off:
> https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/
> and that's what we recommend doing, but while adding the support for
> rx-vlan-filter in enetc I accidentally created another possibility for
> this to work on enetc, by turning IFF_PROMISC on. This is not portable,
> so if somebody develops a solution based on that in parallel, it will
> most certainly break on other non-enetc drivers.
> NXP has not released a kernel based on the v5.10 stable yet, so there is
> still time to change the behavior, but if this goes in through net-next,
> the apparent regression will only be visible when the next LTS comes
> around (whatever the number of that might be). Now, I'm going to
> backport this to the NXP v5.10 anyway, so that's not an issue, but there
> will still be the mild annoyance that the upstream v5.10 will behave
> differently in this regard compared to the NXP v5.10, which is again a
> point of potential confusion, but that seems to be out of my control.
> 
> So if you're still "yeah, don't care", then I guess I'm ok with leaving
> things alone on stable kernels.

I see, so this is indeed of practical importance to NXP.

Would you mind re-spinning with an expanded justification?

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

On Fri, Feb 26, 2021 at 04:45:19PM -0800, Jakub Kicinski wrote:
> I see, so this is indeed of practical importance to NXP.
>
> Would you mind re-spinning with an expanded justification?

Sure, I'll do that tomorrow. Thanks.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Michael Walle]

Am 2021-02-27 01:16, schrieb Vladimir Oltean:
> On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote:
>> On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote:
>> > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote:
>> > > I don't understand what you're fixing tho.
>> > >
>> > > Are we trying to establish vlan-filter-on as the expected behavior?
>> >
>> > What I'm fixing is unexpected behavior, according to the applicable
>> > standards I could find.

In the referenced thread you quoted from the IEEE802.3 about the promisc
mode.
   The MAC sublayer may also provide the capability of operating in the
   promiscuous receive mode. In this mode of operation, the MAC sublayer
   recognizes and accepts all valid frames, regardless of their 
Destination
   Address field values.

Your argument was that the standard just talks about disabling the DMAC
filter. But was that really the _intention_ of the standard? Does the
standard even mention a possible vlan tag? What I mean is: maybe the
standard just mention the DMAC because it is the only filtering 
mechanism
in this standard and it's enough to disable it to "accept all valid 
frames".

I was biten by "the NIC drops frames with an unknown VLAN" even if
promisc mode was enabled. And IMHO it was quite suprising for me.

>> > If I don't mark this change as a bug fix but as
>> > a simple patch, somebody could claim it's a regression, since promiscuity
>> > used to be enough to see packets with unknown VLANs, and now it no
>> > longer is...
>> 
>> Can we take it into net-next? What's your feeling on that option?
> 
> I see how you can view this patch as pointless, but there is some
> context to it. It isn't just for tcpdump/debugging, instead NXP has 
> some
> TSN use cases which involve some asymmetric tc-vlan rules, which is how
> I arrived at this topic in the first place. I've already established
> that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off:
> https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/

Wasn't the conclusion that the VID should be added to the filter so it
also works with vlan filter enabled? Am I missing another discussion?

-michael

> and that's what we recommend doing, but while adding the support for
> rx-vlan-filter in enetc I accidentally created another possibility for
> this to work on enetc, by turning IFF_PROMISC on. This is not portable,
> so if somebody develops a solution based on that in parallel, it will
> most certainly break on other non-enetc drivers.
> NXP has not released a kernel based on the v5.10 stable yet, so there 
> is
> still time to change the behavior, but if this goes in through 
> net-next,
> the apparent regression will only be visible when the next LTS comes
> around (whatever the number of that might be). Now, I'm going to
> backport this to the NXP v5.10 anyway, so that's not an issue, but 
> there
> will still be the mild annoyance that the upstream v5.10 will behave
> differently in this regard compared to the NXP v5.10, which is again a
> point of potential confusion, but that seems to be out of my control.
> 
> So if you're still "yeah, don't care", then I guess I'm ok with leaving
> things alone on stable kernels.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

On Sat, Feb 27, 2021 at 02:18:20PM +0100, Michael Walle wrote:
> Am 2021-02-27 01:16, schrieb Vladimir Oltean:
> > On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote:
> > > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote:
> > > > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote:
> > > > > I don't understand what you're fixing tho.
> > > > >
> > > > > Are we trying to establish vlan-filter-on as the expected behavior?
> > > >
> > > > What I'm fixing is unexpected behavior, according to the applicable
> > > > standards I could find.
>
> In the referenced thread you quoted from the IEEE802.3 about the promisc
> mode.
>   The MAC sublayer may also provide the capability of operating in the
>   promiscuous receive mode. In this mode of operation, the MAC sublayer
>   recognizes and accepts all valid frames, regardless of their Destination
>   Address field values.
>
> Your argument was that the standard just talks about disabling the DMAC
> filter. But was that really the _intention_ of the standard? Does the
> standard even mention a possible vlan tag? What I mean is: maybe the
> standard just mention the DMAC because it is the only filtering mechanism
> in this standard and it's enough to disable it to "accept all valid frames".
>
> I was biten by "the NIC drops frames with an unknown VLAN" even if
> promisc mode was enabled. And IMHO it was quite suprising for me.

In short, promiscuity is a function of the MAC sublayer, which is the
lower portion of the Data Link Layer (the higher portion being the
Logical Link Control layer - LLC). The MAC sublayer is governed by IEEE
802.3, and IEEE 802.1Q does not change anything related to promiscuity,
so everything still applies.

The MAC sublayer provides its services to the MAC client through
something called the MAC service, which uses the following primitives:

MA_DATA.request(
	destination_address,
	source_address,
	mac_service_data_unit,
	frame_check_sequence)

to send a frame, and

MA_DATA.indication(
	destination_address,
	source_address,
	mac_service_data_unit,
	frame_check_sequence,
	ReceiveStatus)

to receive a frame.

One particular component of the MAC sublayer seems to be called the
Internal Sublayer Service (ISS), and this one is defined in IEEE
802.1AC-2016. To be frank, I don't quite grok why there needs to exist
this extra layering, but nonetheless, the ISS has some similar service
primitives to the MAC sublayer as well, and these are:

M_UNITDATA.indication(
	destination_address,
	source_address,
	mac_service_data_unit,
	priority,
	drop_eligible,
	frame_check_sequence,
	service_access_point_identifier,
	connection_identifier)

M_UNITDATA.request(
	destination_address,
	source_address,
	mac_service_data_unit,
	priority,
	drop_eligible,
	frame_check_sequence,
	service_access_point_identifier,
	connection_identifier)

where a "unit of data" is basically just very pompous speak for
"a frame", I guess.

Promiscuity is defined in IEEE 802.3 clause 4A.2.9 Frame reception,
which _in_context_ talks about the interface between the MAC client and
the MAC sublayer, so that means about the M_UNITDATA.indication or the
MA_DATA.indication.

Whereas VLAN filtering, as well as adding and removing VLAN tags, is
governed by IEEE 802.1Q, as a function of the Enhanced Internal Sublayer
Service (EISS), i.e. clause 6.8. In fact, the EISS is just an ISS
enhanced for VLAN filtering, as the naming and definition implies.

Of course (why not), the EISS has its own service primitives towards its
higher-level clients for transmitting and receiving a frame. These are:

EM_UNITDATA.request(
	destination_address,
	source_address,
	mac_service_data_unit,
	priority,
	drop_eligible,
	vlan_identifier,
	frame_check_sequence,
	service_access_point_identifier,
	connection_identifier,
	flow_hash,
	time_to_live)

EM_UNITDATA.indication(
	destination_address,
	source_address,
	mac_service_data_unit,
	priority,
	drop_eligible,
	vlan_identifier,
	frame_check_sequence,
	service_access_point_identifier,
	connection_identifier,
	flow_hash,
	time_to_live)

There's a big note in IEEE 802.1Q that says:

The destination_address, source_address, mac_service_data_unit,
priority, drop_eligible, service_access_point_identifier,
connection_identifier, and frame_check_sequence parameters are as
defined for the ISS.

So basically, although the EISS extends the ISS, it has not changed the
aspects of it regarding what constitutes a destination_address. So there
is nothing that redefines the promiscuity concept to extend it with the
vlan_identifier.

Additionally, the 802.1Q spec talks about this EISS Multiplex Entity
thing, which can be used by a VLAN-aware end station to provide a SAP
(Service Access Point, in context it means an instance of the Internal
Sublayer Service), one per VID of interest, to separate MAC clients.
That is to say, the EISS Multiplex Entity provides multiple
M_UNITDATA.indication and M_UNITDATA.request services to multiple MAC
clients, one per VLAN. Each individual service can be in "promiscuous"
mode. This is similar to how in Linux, each 8021q upper of a physical
interface can be promiscuous or not.

> > > > If I don't mark this change as a bug fix but as
> > > > a simple patch, somebody could claim it's a regression, since promiscuity
> > > > used to be enough to see packets with unknown VLANs, and now it no
> > > > longer is...
> > >
> > > Can we take it into net-next? What's your feeling on that option?
> >
> > I see how you can view this patch as pointless, but there is some
> > context to it. It isn't just for tcpdump/debugging, instead NXP has some
> > TSN use cases which involve some asymmetric tc-vlan rules, which is how
> > I arrived at this topic in the first place. I've already established
> > that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off:
> > https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/
>
> Wasn't the conclusion that the VID should be added to the filter so it
> also works with vlan filter enabled? Am I missing another discussion?

Well, the conclusion was just that a tc-flower key that contains a VLAN
ID will not be accepted by a VLAN-filtering NIC. Similarly, a tc-flower
key that contains a destination MAC address will not be accepted by a
NIC with IFF_UNICAST_FLT.

There was no further discussion, it is just an elementary deduction from
that point. There are two equally valid options:
- make tc-flower use the vlan_vid_add API when it installs a vlan_id
  key, and the dev_uc_add/dev_mc_add API when it installs a dst_mac key
OR
- disable VLAN filtering if you're using vlan_id keys on VLAN-aware
  NICs, and put the interface in promiscuous mode if you're using
  dst_mac keys that are different from the NIC's filtering list.

I chose option 2 because it was way simpler and was just as correct.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Michael Walle]

Am 2021-02-28 23:48, schrieb Vladimir Oltean:
> On Sat, Feb 27, 2021 at 02:18:20PM +0100, Michael Walle wrote:
>> Am 2021-02-27 01:16, schrieb Vladimir Oltean:
>> > On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote:
>> > > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote:
>> > > > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote:
>> > > > > I don't understand what you're fixing tho.
>> > > > >
>> > > > > Are we trying to establish vlan-filter-on as the expected behavior?
>> > > >
>> > > > What I'm fixing is unexpected behavior, according to the applicable
>> > > > standards I could find.
>> 
>> In the referenced thread you quoted from the IEEE802.3 about the 
>> promisc
>> mode.
>>   The MAC sublayer may also provide the capability of operating in the
>>   promiscuous receive mode. In this mode of operation, the MAC 
>> sublayer
>>   recognizes and accepts all valid frames, regardless of their 
>> Destination
>>   Address field values.
>> 
>> Your argument was that the standard just talks about disabling the 
>> DMAC
>> filter. But was that really the _intention_ of the standard? Does the
>> standard even mention a possible vlan tag? What I mean is: maybe the
>> standard just mention the DMAC because it is the only filtering 
>> mechanism
>> in this standard and it's enough to disable it to "accept all valid 
>> frames".
>> 
>> I was biten by "the NIC drops frames with an unknown VLAN" even if
>> promisc mode was enabled. And IMHO it was quite suprising for me.
> 
> In short, promiscuity is a function of the MAC sublayer, which is the
> lower portion of the Data Link Layer (the higher portion being the
> Logical Link Control layer - LLC). The MAC sublayer is governed by IEEE
> 802.3, and IEEE 802.1Q does not change anything related to promiscuity,
> so everything still applies.
> 
> The MAC sublayer provides its services to the MAC client through
> something called the MAC service, which uses the following primitives:
> 
> MA_DATA.request(
> 	destination_address,
> 	source_address,
> 	mac_service_data_unit,
> 	frame_check_sequence)
> 
> to send a frame, and
> 
> MA_DATA.indication(
> 	destination_address,
> 	source_address,
> 	mac_service_data_unit,
> 	frame_check_sequence,
> 	ReceiveStatus)
> 
> to receive a frame.
> 
> One particular component of the MAC sublayer seems to be called the
> Internal Sublayer Service (ISS), and this one is defined in IEEE
> 802.1AC-2016. To be frank, I don't quite grok why there needs to exist
> this extra layering, but nonetheless, the ISS has some similar service
> primitives to the MAC sublayer as well, and these are:
> 
> M_UNITDATA.indication(
> 	destination_address,
> 	source_address,
> 	mac_service_data_unit,
> 	priority,
> 	drop_eligible,
> 	frame_check_sequence,
> 	service_access_point_identifier,
> 	connection_identifier)
> 
> M_UNITDATA.request(
> 	destination_address,
> 	source_address,
> 	mac_service_data_unit,
> 	priority,
> 	drop_eligible,
> 	frame_check_sequence,
> 	service_access_point_identifier,
> 	connection_identifier)
> 
> where a "unit of data" is basically just very pompous speak for
> "a frame", I guess.
> 
> Promiscuity is defined in IEEE 802.3 clause 4A.2.9 Frame reception,
> which _in_context_ talks about the interface between the MAC client and
> the MAC sublayer, so that means about the M_UNITDATA.indication or the
> MA_DATA.indication.
> 
> Whereas VLAN filtering, as well as adding and removing VLAN tags, is
> governed by IEEE 802.1Q, as a function of the Enhanced Internal 
> Sublayer
> Service (EISS), i.e. clause 6.8. In fact, the EISS is just an ISS
> enhanced for VLAN filtering, as the naming and definition implies.
> 
> Of course (why not), the EISS has its own service primitives towards 
> its
> higher-level clients for transmitting and receiving a frame. These are:
> 
> EM_UNITDATA.request(
> 	destination_address,
> 	source_address,
> 	mac_service_data_unit,
> 	priority,
> 	drop_eligible,
> 	vlan_identifier,
> 	frame_check_sequence,
> 	service_access_point_identifier,
> 	connection_identifier,
> 	flow_hash,
> 	time_to_live)
> 
> EM_UNITDATA.indication(
> 	destination_address,
> 	source_address,
> 	mac_service_data_unit,
> 	priority,
> 	drop_eligible,
> 	vlan_identifier,
> 	frame_check_sequence,
> 	service_access_point_identifier,
> 	connection_identifier,
> 	flow_hash,
> 	time_to_live)
> 
> There's a big note in IEEE 802.1Q that says:
> 
> The destination_address, source_address, mac_service_data_unit,
> priority, drop_eligible, service_access_point_identifier,
> connection_identifier, and frame_check_sequence parameters are as
> defined for the ISS.
> 
> So basically, although the EISS extends the ISS, it has not changed the
> aspects of it regarding what constitutes a destination_address. So 
> there
> is nothing that redefines the promiscuity concept to extend it with the
> vlan_identifier.
> 
> Additionally, the 802.1Q spec talks about this EISS Multiplex Entity
> thing, which can be used by a VLAN-aware end station to provide a SAP
> (Service Access Point, in context it means an instance of the Internal
> Sublayer Service), one per VID of interest, to separate MAC clients.
> That is to say, the EISS Multiplex Entity provides multiple
> M_UNITDATA.indication and M_UNITDATA.request services to multiple MAC
> clients, one per VLAN. Each individual service can be in "promiscuous"
> mode. This is similar to how in Linux, each 8021q upper of a physical
> interface can be promiscuous or not.

Ok, I see, so your proposed behavior is backed by the standards. But
OTOH there was a summary by Markus of the behavior of other drivers:
https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/
And a conclusion by Jakub:
https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t
And a propsed core change to disable vlan filtering with promisc mode.
Do I understand you correctly, that this shouldn't be done either?

Don't get me wrong, I don't vote against or in favor of this patch.
I just want to understand the behavior.

I haven't had time to actually test this, but what if you do:
  - don't load the 8021q module (or don't enable kernel support)
  - enable promisc
  (1)
  - load 8021q module
  (2)
  - add a vlan interface
  (3)
  - add another vlan interface
  (4)

What frames would you actually receive on the base interface
in (1), (2), (3), (4) and what is the user expectation?
I'd say its the same every time. (IIRC there is already some
discrepancy due to the VLAN filter hardware offloading)

>> > > > If I don't mark this change as a bug fix but as
>> > > > a simple patch, somebody could claim it's a regression, since promiscuity
>> > > > used to be enough to see packets with unknown VLANs, and now it no
>> > > > longer is...
>> > >
>> > > Can we take it into net-next? What's your feeling on that option?
>> >
>> > I see how you can view this patch as pointless, but there is some
>> > context to it. It isn't just for tcpdump/debugging, instead NXP has some
>> > TSN use cases which involve some asymmetric tc-vlan rules, which is how
>> > I arrived at this topic in the first place. I've already established
>> > that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off:
>> > https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/
>> 
>> Wasn't the conclusion that the VID should be added to the filter so it
>> also works with vlan filter enabled? Am I missing another discussion?
> 
> Well, the conclusion was just that a tc-flower key that contains a VLAN
> ID will not be accepted by a VLAN-filtering NIC. Similarly, a tc-flower
> key that contains a destination MAC address will not be accepted by a
> NIC with IFF_UNICAST_FLT.
> 
> There was no further discussion, it is just an elementary deduction 
> from
> that point. There are two equally valid options:
> - make tc-flower use the vlan_vid_add API when it installs a vlan_id
>   key, and the dev_uc_add/dev_mc_add API when it installs a dst_mac key
> OR
> - disable VLAN filtering if you're using vlan_id keys on VLAN-aware
>   NICs, and put the interface in promiscuous mode if you're using
>   dst_mac keys that are different from the NIC's filtering list.
> 
> I chose option 2 because it was way simpler and was just as correct.

Fair, but it will also put additional burden to the user to also
disable the vlan filtering, right?. Otherwise it would just work. And
it will waste CPU cycles for unwanted frames.
Although your new patch version contains a new "(yet)" ;)

-michael

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

On Mon, Mar 01, 2021 at 03:36:15PM +0100, Michael Walle wrote:
> Ok, I see, so your proposed behavior is backed by the standards. But
> OTOH there was a summary by Markus of the behavior of other drivers:
> https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/
> And a conclusion by Jakub:
> https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t
> And a propsed core change to disable vlan filtering with promisc mode.
> Do I understand you correctly, that this shouldn't be done either?
>
> Don't get me wrong, I don't vote against or in favor of this patch.
> I just want to understand the behavior.

So you can involuntarily ignore a standard, or you can ignore it
deliberately. I can't force anyone to not ignore it in the latter case,
but indeed, now that I tried to look it up, I personally don't think
that promiscuity should disable VLAN filtering unless somebody comes up
with a good reason for which Linux should basically disregard IEEE 802.3.
In particular, Jakub seems to have been convinced in that thread by no
other argument except that other drivers ignore the standards too, which
I'm not sure is a convincing enough argument.

In my opinion, the fact that some drivers disable VLAN filtering should
be treated like a marginal condition, similar to how, when you set the
MTU on an interface to N octets, it might happen that it accepts packets
larger than N octets, but it isn't guaranteed.

> I haven't had time to actually test this, but what if you do:
>  - don't load the 8021q module (or don't enable kernel support)
>  - enable promisc
>  (1)
>  - load 8021q module
>  (2)
>  - add a vlan interface
>  (3)
>  - add another vlan interface
>  (4)
>
> What frames would you actually receive on the base interface
> in (1), (2), (3), (4) and what is the user expectation?
> I'd say its the same every time. (IIRC there is already some
> discrepancy due to the VLAN filter hardware offloading)

The default value is:
ethtool -k eno0 | grep rx-vlan-filter
rx-vlan-filter: off

so we receive all VLAN-tagged packets by default in enetc, unless VLAN
filtering is turned on.

> > I chose option 2 because it was way simpler and was just as correct.
>
> Fair, but it will also put additional burden to the user to also
> disable the vlan filtering, right?. Otherwise it would just work. And
> it will waste CPU cycles for unwanted frames.
> Although your new patch version contains a new "(yet)" ;)

True, nobody said it's optimal, but you can't make progress if you
always try to do things optimally the first time (but at least you
should do something that's not wrong).
Adding the dev_uc_add, dev_mc_add and vlan_vid_add calls to
net/sched/cls_flower.c doesn't seem an impossible task (especially since
all of them are refcounted, it should be pretty simple to avoid strange
interactions with other layers such as 8021q), but nonetheless, it just
wasn't (and still isn't) high enough on my list of priorities.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Markus Blöchl]

On Mon, Mar 01, 2021 at 05:08:52PM +0200, Vladimir Oltean wrote:
> On Mon, Mar 01, 2021 at 03:36:15PM +0100, Michael Walle wrote:
> > Ok, I see, so your proposed behavior is backed by the standards. But
> > OTOH there was a summary by Markus of the behavior of other drivers:
> > https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/
> > And a conclusion by Jakub:
> > https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t
> > And a propsed core change to disable vlan filtering with promisc mode.
> > Do I understand you correctly, that this shouldn't be done either?
> >
> > Don't get me wrong, I don't vote against or in favor of this patch.
> > I just want to understand the behavior.
> 
> So you can involuntarily ignore a standard, or you can ignore it
> deliberately. I can't force anyone to not ignore it in the latter case,
> but indeed, now that I tried to look it up, I personally don't think
> that promiscuity should disable VLAN filtering unless somebody comes up
> with a good reason for which Linux should basically disregard IEEE 802.3.
> In particular, Jakub seems to have been convinced in that thread by no
> other argument except that other drivers ignore the standards too, which
> I'm not sure is a convincing enough argument.

Admittedly, I am still not entirely convinced myself.
I don't know why the other drivers do what they do, why they do it and
whether that's correct.

That is one of the reasons (next to quite a few issues I had with patching
net/core) why I ungracefully abandoned the mentioned thread for now
( sorry and shame on me :-/ ).

The main problem here could also just be that almost everybody _thinks_
that promiscuity means receiving all frames and no one is aware of the
standards definition.
In fact, I can't blame them, as the standard is hard to come by and not
enjoyable to read, imho. And all secondary documentation I could find
on the internet explain promiscuous mode as a "mode of operation" in which
"the card accepts every Ethernet packet sent on the network" or similar.
Even libpcap, which I consider the reference on network sniffing, thinks
that "Promiscuous mode [...] sniffs all traffic on the wire."

Thus I still think that this issue is also fixable by proper
documentation of promiscuity.
At least the meaning and guarantees of IFF_PROMISC in this kernel should
be clearly defined - in one way or the other - such that users with
different expectations can be directed there and drivers with different
behavior can be fixed with that definition as justification.

> 
> In my opinion, the fact that some drivers disable VLAN filtering should
> be treated like a marginal condition, similar to how, when you set the
> MTU on an interface to N octets, it might happen that it accepts packets
> larger than N octets, but it isn't guaranteed.
> 
> > I haven't had time to actually test this, but what if you do:
> >  - don't load the 8021q module (or don't enable kernel support)
> >  - enable promisc
> >  (1)
> >  - load 8021q module
> >  (2)
> >  - add a vlan interface
> >  (3)
> >  - add another vlan interface
> >  (4)
> >
> > What frames would you actually receive on the base interface
> > in (1), (2), (3), (4) and what is the user expectation?
> > I'd say its the same every time. (IIRC there is already some
> > discrepancy due to the VLAN filter hardware offloading)
> 
> The default value is:
> ethtool -k eno0 | grep rx-vlan-filter
> rx-vlan-filter: off
> 
> so we receive all VLAN-tagged packets by default in enetc, unless VLAN
> filtering is turned on.
> 
> > > I chose option 2 because it was way simpler and was just as correct.
> >
> > Fair, but it will also put additional burden to the user to also
> > disable the vlan filtering, right?. Otherwise it would just work. And
> > it will waste CPU cycles for unwanted frames.
> > Although your new patch version contains a new "(yet)" ;)
> 
> True, nobody said it's optimal, but you can't make progress if you
> always try to do things optimally the first time (but at least you
> should do something that's not wrong).
> Adding the dev_uc_add, dev_mc_add and vlan_vid_add calls to
> net/sched/cls_flower.c doesn't seem an impossible task (especially since
> all of them are refcounted, it should be pretty simple to avoid strange
> interactions with other layers such as 8021q), but nonetheless, it just
> wasn't (and still isn't) high enough on my list of priorities.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Vladimir Oltean]

On Mon, Mar 01, 2021 at 05:26:53PM +0100, Markus Blöchl wrote:
> The main problem here could also just be that almost everybody _thinks_
> that promiscuity means receiving all frames and no one is aware of the
> standards definition.
> In fact, I can't blame them, as the standard is hard to come by and not
> enjoyable to read, imho. And all secondary documentation I could find
> on the internet explain promiscuous mode as a "mode of operation" in which
> "the card accepts every Ethernet packet sent on the network" or similar.
> Even libpcap, which I consider the reference on network sniffing, thinks
> that "Promiscuous mode [...] sniffs all traffic on the wire."
> 
> Thus I still think that this issue is also fixable by proper
> documentation of promiscuity.
> At least the meaning and guarantees of IFF_PROMISC in this kernel should
> be clearly defined - in one way or the other - such that users with
> different expectations can be directed there and drivers with different
> behavior can be fixed with that definition as justification.

If Jakub and/or David give us the ACK, I will go ahead and update the
documentation (probably Documentation/networking/netdevices.rst) to
mention what does IFF_PROMISC cover, _separate_ from this series.

Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in IFF_PROMISC mode

[Jakub Kicinski]

On Mon, 1 Mar 2021 19:02:51 +0200 Vladimir Oltean wrote:
> On Mon, Mar 01, 2021 at 05:26:53PM +0100, Markus Blöchl wrote:
> > The main problem here could also just be that almost everybody _thinks_
> > that promiscuity means receiving all frames and no one is aware of the
> > standards definition.
> > In fact, I can't blame them, as the standard is hard to come by and not
> > enjoyable to read, imho. And all secondary documentation I could find
> > on the internet explain promiscuous mode as a "mode of operation" in which
> > "the card accepts every Ethernet packet sent on the network" or similar.
> > Even libpcap, which I consider the reference on network sniffing, thinks
> > that "Promiscuous mode [...] sniffs all traffic on the wire."
> > 
> > Thus I still think that this issue is also fixable by proper
> > documentation of promiscuity.
> > At least the meaning and guarantees of IFF_PROMISC in this kernel should
> > be clearly defined - in one way or the other - such that users with
> > different expectations can be directed there and drivers with different
> > behavior can be fixed with that definition as justification.  
> 
> If Jakub and/or David give us the ACK, I will go ahead and update the
> documentation (probably Documentation/networking/netdevices.rst) to
> mention what does IFF_PROMISC cover, _separate_ from this series.

How do we do this in practical terms?

It'd definitely be very useful to write up the discussions but we 
can't expect that all existing drivers get converted, and incorrect
documentation is worse than none at all.

IIRC Ido also pointed out that the bridge driver follows the "promisc
includes VLAN", so SW drivers would need to be updated as well.

I personally agree with the interpretation that PROMISC == disable DA
filter, but we can't reasonably expect all drivers to follow it.
All I can think of is documenting that the driver _may_ not disable
VLAN filter, and that's our recommendation for new drivers, therefore
users who want "vlan promisc" _must_ disable rx-vlan but the inverse is
not guaranteed (rx-vlan on + PROMISC == only frames for known VLANs).

What's your thinking?

[PATCH v2 net 6/6] net: enetc: force the RGMII speed and duplex instead of operating in inband mode

[Vladimir Oltean]

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

The ENETC port 0 MAC supports in-band status signaling coming from a PHY
when operating in RGMII mode, and this feature is enabled by default.

It has been reported that RGMII is broken in fixed-link, and that is not
surprising considering the fact that no PHY is attached to the MAC in
that case, but a switch.

This brings us to the topic of the patch: the enetc driver should have
not enabled the optional in-band status signaling for RGMII unconditionally,
but should have forced the speed and duplex to what was resolved by
phylink.

Note that phylink does not accept the RGMII modes as valid for in-band
signaling, and these operate a bit differently than 1000base-x and SGMII
(notably there is no clause 37 state machine so no ACK required from the
MAC, instead the PHY sends extra code words on RXD[3:0] whenever it is
not transmitting something else, so it should be safe to leave a PHY
with this option unconditionally enabled even if we ignore it). The spec
talks about this here:
https://e2e.ti.com/cfs-file/__key/communityserver-discussions-components-files/138/RGMIIv1_5F00_3.pdf

Fixes: 71b77a7a27a3 ("enetc: Migrate to PHYLINK and PCS_LYNX")
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
- Don't write to the MAC if speed and duplex did not change.
- Don't update the speed with the MAC enabled.
- Remove the logic for enabling in-band signaling in enetc_mac_config.

 .../net/ethernet/freescale/enetc/enetc_hw.h   | 13 ++++-
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 58 ++++++++++++++++---
 2 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index a62604a1e54e..de0d20b0f489 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -238,10 +238,17 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_PM_IMDIO_BASE	0x8030
 
 #define ENETC_PM0_IF_MODE	0x8300
-#define ENETC_PMO_IFM_RG	BIT(2)
+#define ENETC_PM0_IFM_RG	BIT(2)
 #define ENETC_PM0_IFM_RLP	(BIT(5) | BIT(11))
-#define ENETC_PM0_IFM_RGAUTO	(BIT(15) | ENETC_PMO_IFM_RG | BIT(1))
-#define ENETC_PM0_IFM_XGMII	BIT(12)
+#define ENETC_PM0_IFM_EN_AUTO	BIT(15)
+#define ENETC_PM0_IFM_SSP_MASK	GENMASK(14, 13)
+#define ENETC_PM0_IFM_SSP_1000	(2 << 13)
+#define ENETC_PM0_IFM_SSP_100	(0 << 13)
+#define ENETC_PM0_IFM_SSP_10	(1 << 13)
+#define ENETC_PM0_IFM_FULL_DPX	BIT(12)
+#define ENETC_PM0_IFM_IFMODE_MASK GENMASK(1, 0)
+#define ENETC_PM0_IFM_IFMODE_XGMII 0
+#define ENETC_PM0_IFM_IFMODE_GMII 2
 #define ENETC_PSIDCAPR		0x1b08
 #define ENETC_PSIDCAPR_MSK	GENMASK(15, 0)
 #define ENETC_PSFCAPR		0x1b18
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 49681a0566ed..e0c2e8db4801 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -315,7 +315,7 @@ static void enetc_set_loopback(struct net_device *ndev, bool en)
 	u32 reg;
 
 	reg = enetc_port_rd(hw, ENETC_PM0_IF_MODE);
-	if (reg & ENETC_PMO_IFM_RG) {
+	if (reg & ENETC_PM0_IFM_RG) {
 		/* RGMII mode */
 		reg = (reg & ~ENETC_PM0_IFM_RLP) |
 		      (en ? ENETC_PM0_IFM_RLP : 0);
@@ -492,15 +492,23 @@ static void enetc_configure_port_mac(struct enetc_hw *hw)
 		      ENETC_PM0_CMD_TXP	| ENETC_PM0_PROMISC);
 }
 
-static void enetc_mac_config(struct enetc_hw *hw, phy_interface_t phy_mode)
+static void enetc_mac_config(struct enetc_hw *hw, phy_interface_t phy_mode,
+			     unsigned int mode)
 {
-	/* set auto-speed for RGMII */
-	if (enetc_port_rd(hw, ENETC_PM0_IF_MODE) & ENETC_PMO_IFM_RG ||
-	    phy_interface_mode_is_rgmii(phy_mode))
-		enetc_port_wr(hw, ENETC_PM0_IF_MODE, ENETC_PM0_IFM_RGAUTO);
+	u32 val;
+
+	if (phy_interface_mode_is_rgmii(phy_mode)) {
+		val = enetc_port_rd(hw, ENETC_PM0_IF_MODE);
+		val &= ~ENETC_PM0_IFM_EN_AUTO;
+		val &= ENETC_PM0_IFM_IFMODE_MASK;
+		val |= ENETC_PM0_IFM_IFMODE_GMII | ENETC_PM0_IFM_RG;
+		enetc_port_wr(hw, ENETC_PM0_IF_MODE, val);
+	}
 
-	if (phy_mode == PHY_INTERFACE_MODE_USXGMII)
-		enetc_port_wr(hw, ENETC_PM0_IF_MODE, ENETC_PM0_IFM_XGMII);
+	if (phy_mode == PHY_INTERFACE_MODE_USXGMII) {
+		val = ENETC_PM0_IFM_FULL_DPX | ENETC_PM0_IFM_IFMODE_XGMII;
+		enetc_port_wr(hw, ENETC_PM0_IF_MODE, val);
+	}
 }
 
 static void enetc_mac_enable(struct enetc_hw *hw, bool en)
@@ -925,13 +933,41 @@ static void enetc_pl_mac_config(struct phylink_config *config,
 	struct enetc_pf *pf = phylink_to_enetc_pf(config);
 	struct enetc_ndev_priv *priv;
 
-	enetc_mac_config(&pf->si->hw, state->interface);
+	enetc_mac_config(&pf->si->hw, state->interface, mode);
 
 	priv = netdev_priv(pf->si->ndev);
 	if (pf->pcs)
 		phylink_set_pcs(priv->phylink, &pf->pcs->pcs);
 }
 
+static void enetc_force_rgmii_mac(struct enetc_hw *hw, int speed, int duplex)
+{
+	u32 old_val, val;
+
+	old_val = val = enetc_port_rd(hw, ENETC_PM0_IF_MODE);
+
+	if (speed == SPEED_1000) {
+		val &= ~ENETC_PM0_IFM_SSP_MASK;
+		val |= ENETC_PM0_IFM_SSP_1000;
+	} else if (speed == SPEED_100) {
+		val &= ~ENETC_PM0_IFM_SSP_MASK;
+		val |= ENETC_PM0_IFM_SSP_100;
+	} else if (speed == SPEED_10) {
+		val &= ~ENETC_PM0_IFM_SSP_MASK;
+		val |= ENETC_PM0_IFM_SSP_10;
+	}
+
+	if (duplex == DUPLEX_FULL)
+		val |= ENETC_PM0_IFM_FULL_DPX;
+	else
+		val &= ~ENETC_PM0_IFM_FULL_DPX;
+
+	if (val == old_val)
+		return;
+
+	enetc_port_wr(hw, ENETC_PM0_IF_MODE, val);
+}
+
 static void enetc_pl_mac_link_up(struct phylink_config *config,
 				 struct phy_device *phy, unsigned int mode,
 				 phy_interface_t interface, int speed,
@@ -944,6 +980,10 @@ static void enetc_pl_mac_link_up(struct phylink_config *config,
 	if (priv->active_offloads & ENETC_F_QBV)
 		enetc_sched_speed_set(priv, speed);
 
+	if (!phylink_autoneg_inband(mode) &&
+	    phy_interface_mode_is_rgmii(interface))
+		enetc_force_rgmii_mac(&pf->si->hw, speed, duplex);
+
 	enetc_mac_enable(&pf->si->hw, true);
 }
 
-- 
2.25.1

Re: [PATCH v2 net 6/6] net: enetc: force the RGMII speed and duplex instead of operating in inband mode

[Russell King - ARM Linux admin]

On Thu, Feb 25, 2021 at 02:18:35PM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The ENETC port 0 MAC supports in-band status signaling coming from a PHY
> when operating in RGMII mode, and this feature is enabled by default.
> 
> It has been reported that RGMII is broken in fixed-link, and that is not
> surprising considering the fact that no PHY is attached to the MAC in
> that case, but a switch.
> 
> This brings us to the topic of the patch: the enetc driver should have
> not enabled the optional in-band status signaling for RGMII unconditionally,
> but should have forced the speed and duplex to what was resolved by
> phylink.
> 
> Note that phylink does not accept the RGMII modes as valid for in-band
> signaling, and these operate a bit differently than 1000base-x and SGMII
> (notably there is no clause 37 state machine so no ACK required from the
> MAC, instead the PHY sends extra code words on RXD[3:0] whenever it is
> not transmitting something else, so it should be safe to leave a PHY
> with this option unconditionally enabled even if we ignore it). The spec
> talks about this here:
> https://e2e.ti.com/cfs-file/__key/communityserver-discussions-components-files/138/RGMIIv1_5F00_3.pdf
> 
> Fixes: 71b77a7a27a3 ("enetc: Migrate to PHYLINK and PCS_LYNX")
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>

Looks better, thanks.

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Changes in v2:
> - Don't write to the MAC if speed and duplex did not change.
> - Don't update the speed with the MAC enabled.
> - Remove the logic for enabling in-band signaling in enetc_mac_config.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!