[PATCH net] net: mscc: ocelot: properly account for VLAN header length when setting MRU

[PATCH net] net: mscc: ocelot: properly account for VLAN header length when setting MRU

[Vladimir Oltean]

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

What the driver writes into MAC_MAXLEN_CFG does not actually represent
VLAN_ETH_FRAME_LEN but instead ETH_FRAME_LEN + ETH_FCS_LEN. Yes they are
numerically equal, but the difference is important, as the switch treats
VLAN-tagged traffic specially and knows to increase the maximum accepted
frame size automatically. So it is always wrong to account for VLAN in
the MAC_MAXLEN_CFG register.

Unconditionally increase the maximum allowed frame size for
double-tagged traffic. Accounting for the additional length does not
mean that the other VLAN membership checks aren't performed, so there's
no harm done.

Also, stop abusing the MTU name for configuring the MRU. There is no
support for configuring the MRU on an interface at the moment.

Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support")
Fixes: fa914e9c4d94 ("net: mscc: ocelot: create a helper for changing the port MTU")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 28 +++++++++++++++++-----------
 include/soc/mscc/ocelot_dev.h      |  2 +-
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index cd4dd885f038..6e4cca34a04f 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -2188,24 +2188,29 @@ static int ocelot_init_timestamp(struct ocelot *ocelot)
 	return 0;
 }
 
-static void ocelot_port_set_mtu(struct ocelot *ocelot, int port, size_t mtu)
+/* Configure the maximum SDU (L2 payload) on RX to the value specified in @sdu.
+ * The length of VLAN tags is accounted for automatically via DEV_MAC_TAGS_CFG.
+ */
+static void ocelot_port_set_maxlen(struct ocelot *ocelot, int port, size_t sdu)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	int maxlen = sdu + ETH_HLEN + ETH_FCS_LEN;
 	int atop_wm;
 
-	ocelot_port_writel(ocelot_port, mtu, DEV_MAC_MAXLEN_CFG);
+	ocelot_port_writel(ocelot_port, maxlen, DEV_MAC_MAXLEN_CFG);
 
 	/* Set Pause WM hysteresis
-	 * 152 = 6 * mtu / OCELOT_BUFFER_CELL_SZ
-	 * 101 = 4 * mtu / OCELOT_BUFFER_CELL_SZ
+	 * 152 = 6 * maxlen / OCELOT_BUFFER_CELL_SZ
+	 * 101 = 4 * maxlen / OCELOT_BUFFER_CELL_SZ
 	 */
 	ocelot_write_rix(ocelot, SYS_PAUSE_CFG_PAUSE_ENA |
 			 SYS_PAUSE_CFG_PAUSE_STOP(101) |
 			 SYS_PAUSE_CFG_PAUSE_START(152), SYS_PAUSE_CFG, port);
 
 	/* Tail dropping watermark */
-	atop_wm = (ocelot->shared_queue_sz - 9 * mtu) / OCELOT_BUFFER_CELL_SZ;
-	ocelot_write_rix(ocelot, ocelot_wm_enc(9 * mtu),
+	atop_wm = (ocelot->shared_queue_sz - 9 * maxlen) /
+		   OCELOT_BUFFER_CELL_SZ;
+	ocelot_write_rix(ocelot, ocelot_wm_enc(9 * maxlen),
 			 SYS_ATOP, port);
 	ocelot_write(ocelot, ocelot_wm_enc(atop_wm), SYS_ATOP_TOT_CFG);
 }
@@ -2234,9 +2239,10 @@ void ocelot_init_port(struct ocelot *ocelot, int port)
 			   DEV_MAC_HDX_CFG);
 
 	/* Set Max Length and maximum tags allowed */
-	ocelot_port_set_mtu(ocelot, port, VLAN_ETH_FRAME_LEN);
+	ocelot_port_set_maxlen(ocelot, port, ETH_DATA_LEN);
 	ocelot_port_writel(ocelot_port, DEV_MAC_TAGS_CFG_TAG_ID(ETH_P_8021AD) |
 			   DEV_MAC_TAGS_CFG_VLAN_AWR_ENA |
+			   DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA |
 			   DEV_MAC_TAGS_CFG_VLAN_LEN_AWR_ENA,
 			   DEV_MAC_TAGS_CFG);
 
@@ -2329,18 +2335,18 @@ void ocelot_configure_cpu(struct ocelot *ocelot, int npi,
 			 ANA_PORT_PORT_CFG, cpu);
 
 	if (npi >= 0 && npi < ocelot->num_phys_ports) {
-		int mtu = VLAN_ETH_FRAME_LEN + OCELOT_TAG_LEN;
+		int sdu = ETH_DATA_LEN + OCELOT_TAG_LEN;
 
 		ocelot_write(ocelot, QSYS_EXT_CPU_CFG_EXT_CPUQ_MSK_M |
 			     QSYS_EXT_CPU_CFG_EXT_CPU_PORT(npi),
 			     QSYS_EXT_CPU_CFG);
 
 		if (injection == OCELOT_TAG_PREFIX_SHORT)
-			mtu += OCELOT_SHORT_PREFIX_LEN;
+			sdu += OCELOT_SHORT_PREFIX_LEN;
 		else if (injection == OCELOT_TAG_PREFIX_LONG)
-			mtu += OCELOT_LONG_PREFIX_LEN;
+			sdu += OCELOT_LONG_PREFIX_LEN;
 
-		ocelot_port_set_mtu(ocelot, npi, mtu);
+		ocelot_port_set_maxlen(ocelot, npi, sdu);
 
 		/* Enable NPI port */
 		ocelot_write_rix(ocelot,
diff --git a/include/soc/mscc/ocelot_dev.h b/include/soc/mscc/ocelot_dev.h
index 0a50d53bbd3f..7c08437061fc 100644
--- a/include/soc/mscc/ocelot_dev.h
+++ b/include/soc/mscc/ocelot_dev.h
@@ -74,7 +74,7 @@
 #define DEV_MAC_TAGS_CFG_TAG_ID_M                         GENMASK(31, 16)
 #define DEV_MAC_TAGS_CFG_TAG_ID_X(x)                      (((x) & GENMASK(31, 16)) >> 16)
 #define DEV_MAC_TAGS_CFG_VLAN_LEN_AWR_ENA                 BIT(2)
-#define DEV_MAC_TAGS_CFG_PB_ENA                           BIT(1)
+#define DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA                 BIT(1)
 #define DEV_MAC_TAGS_CFG_VLAN_AWR_ENA                     BIT(0)
 
 #define DEV_MAC_ADV_CHK_CFG                               0x2c
-- 
2.17.1

Re: [PATCH net] net: mscc: ocelot: properly account for VLAN header length when setting MRU

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon,  9 Mar 2020 22:16:08 +0200

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> What the driver writes into MAC_MAXLEN_CFG does not actually represent
> VLAN_ETH_FRAME_LEN but instead ETH_FRAME_LEN + ETH_FCS_LEN. Yes they are
> numerically equal, but the difference is important, as the switch treats
> VLAN-tagged traffic specially and knows to increase the maximum accepted
> frame size automatically. So it is always wrong to account for VLAN in
> the MAC_MAXLEN_CFG register.
> 
> Unconditionally increase the maximum allowed frame size for
> double-tagged traffic. Accounting for the additional length does not
> mean that the other VLAN membership checks aren't performed, so there's
> no harm done.
> 
> Also, stop abusing the MTU name for configuring the MRU. There is no
> support for configuring the MRU on an interface at the moment.
> 
> Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support")
> Fixes: fa914e9c4d94 ("net: mscc: ocelot: create a helper for changing the port MTU")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

This doesn't apply cleanly to the current net tree, please respin.

Thank you.

Re: [PATCH net] net: mscc: ocelot: properly account for VLAN header length when setting MRU

[Vladimir Oltean]

On Tue, 10 Mar 2020 at 02:55, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Mon,  9 Mar 2020 22:16:08 +0200
>
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > What the driver writes into MAC_MAXLEN_CFG does not actually represent
> > VLAN_ETH_FRAME_LEN but instead ETH_FRAME_LEN + ETH_FCS_LEN. Yes they are
> > numerically equal, but the difference is important, as the switch treats
> > VLAN-tagged traffic specially and knows to increase the maximum accepted
> > frame size automatically. So it is always wrong to account for VLAN in
> > the MAC_MAXLEN_CFG register.
> >
> > Unconditionally increase the maximum allowed frame size for
> > double-tagged traffic. Accounting for the additional length does not
> > mean that the other VLAN membership checks aren't performed, so there's
> > no harm done.
> >
> > Also, stop abusing the MTU name for configuring the MRU. There is no
> > support for configuring the MRU on an interface at the moment.
> >
> > Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support")
> > Fixes: fa914e9c4d94 ("net: mscc: ocelot: create a helper for changing the port MTU")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> This doesn't apply cleanly to the current net tree, please respin.
>
> Thank you.

Ok. Sent new version here:
https://patchwork.ozlabs.org/patch/1251902/
When you merge net into net-next you can use this patch as reference
for the conflict resolution, since it is going to conflict with
69df578c5f4b ("net: mscc: ocelot: eliminate confusion between CPU and
NPI port") - both rename some variables.

Thanks,
-Vladimir

Re: [PATCH net] net: mscc: ocelot: properly account for VLAN header length when setting MRU

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Tue, 10 Mar 2020 03:30:26 +0200

> When you merge net into net-next you can use this patch as reference
> for the conflict resolution, since it is going to conflict with
> 69df578c5f4b ("net: mscc: ocelot: eliminate confusion between CPU and
> NPI port") - both rename some variables.

Ok, thanks for letting me know.