[PATCH 0/2] Fix Ethernet jumbo frames support for Armada 370

[PATCH 0/2] Fix Ethernet jumbo frames support for Armada 370

[Simon Guinot]

Hello,

This patch series fixes the Ethernet jumbo frames support for Armada 370
SoCs. Unlike Armada XP, the Ethernet controller in Armada 370 SoCs don't
support TCP/IP checksumming with frames largest than 1600 Bytes.

This patches should be applied to the -stable kernels 3.8 and onwards.

Regards,

Simon

Simon Guinot (2):
  net: mvneta: introduce tx_csum_limit property
  ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

 .../bindings/net/marvell-armada-370-neta.txt       |  3 +++
 arch/arm/boot/dts/armada-370.dtsi                  |  8 +++++++
 drivers/net/ethernet/marvell/mvneta.c              | 25 +++++++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

--
Cc: <stable@vger.kernel.org>
-- 
2.1.4

[PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Simon Guinot]

This patch introduces the tx_csum_limit DT property. This allows to
configure the maximum frame size for which the Ethernet controller is
able to perform TCP/IP checksumming. If MTU is set to a value greater
than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
are disabled.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: <stable@vger.kernel.org> # v3.8+
---
 .../bindings/net/marvell-armada-370-neta.txt       |  3 +++
 drivers/net/ethernet/marvell/mvneta.c              | 25 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 750d577e8083..db48c83ff0f5 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -8,6 +8,9 @@ Required properties:
 - phy-mode: See ethernet.txt file in the same directory
 - clocks: a pointer to the reference clock for this device.
 
+Optional properties:
+- tx_csum_limit: max tx packet size for hardware checksum.
+
 Example:
 
 ethernet@d0070000 {
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..1a724e0a2a26 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -310,6 +310,7 @@ struct mvneta_port {
 	unsigned int link;
 	unsigned int duplex;
 	unsigned int speed;
+	unsigned int tx_csum_limit;
 	int use_inband_status:1;
 };
 
@@ -2502,8 +2503,10 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu)
 
 	dev->mtu = mtu;
 
-	if (!netif_running(dev))
+	if (!netif_running(dev)) {
+		netdev_update_features(dev);
 		return 0;
+	}
 
 	/* The interface is running, so we have to force a
 	 * reallocation of the queues
@@ -2532,9 +2535,26 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu)
 	mvneta_start_dev(pp);
 	mvneta_port_up(pp);
 
+	netdev_update_features(dev);
+
 	return 0;
 }
 
+static netdev_features_t mvneta_fix_features(struct net_device *dev,
+					     netdev_features_t features)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+
+	if (pp->tx_csum_limit && dev->mtu > pp->tx_csum_limit) {
+		features &= ~(NETIF_F_IP_CSUM | NETIF_F_TSO);
+		netdev_info(dev,
+			    "Disable IP checksum for MTU greater than %dB\n",
+			    pp->tx_csum_limit);
+	}
+
+	return features;
+}
+
 /* Get mac address */
 static void mvneta_get_mac_addr(struct mvneta_port *pp, unsigned char *addr)
 {
@@ -2856,6 +2876,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
 	.ndo_set_rx_mode     = mvneta_set_rx_mode,
 	.ndo_set_mac_address = mvneta_set_mac_addr,
 	.ndo_change_mtu      = mvneta_change_mtu,
+	.ndo_fix_features    = mvneta_fix_features,
 	.ndo_get_stats64     = mvneta_get_stats64,
 	.ndo_do_ioctl        = mvneta_ioctl,
 };
@@ -3101,6 +3122,8 @@ static int mvneta_probe(struct platform_device *pdev)
 		}
 	}
 
+	of_property_read_u32(dn, "tx_csum_limit", &pp->tx_csum_limit);
+
 	pp->tx_ring_size = MVNETA_MAX_TXD;
 	pp->rx_ring_size = MVNETA_MAX_RXD;
 
-- 
2.1.4

Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Russell King - ARM Linux]

On Mon, Jun 15, 2015 at 04:27:22PM +0200, Simon Guinot wrote:
> This patch introduces the tx_csum_limit DT property. This allows to
> configure the maximum frame size for which the Ethernet controller is
> able to perform TCP/IP checksumming. If MTU is set to a value greater
> than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
> are disabled.
> 
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Cc: <stable@vger.kernel.org> # v3.8+

Sorry, I just tripped over this.  This looks like a patch adding a new
_feature_ to this ethernet driver.  It isn't a regression fix, and it
isn't a bug fix.  Why are you wanting to get it into stable kernels,
which are supposed to only have bug and regression fixes applied?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Thomas Petazzoni]

Russell,

On Mon, 15 Jun 2015 15:36:01 +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 15, 2015 at 04:27:22PM +0200, Simon Guinot wrote:
> > This patch introduces the tx_csum_limit DT property. This allows to
> > configure the maximum frame size for which the Ethernet controller is
> > able to perform TCP/IP checksumming. If MTU is set to a value greater
> > than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
> > are disabled.
> > 
> > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> > Cc: <stable@vger.kernel.org> # v3.8+
> 
> Sorry, I just tripped over this.  This looks like a patch adding a new
> _feature_ to this ethernet driver.  It isn't a regression fix, and it
> isn't a bug fix.  Why are you wanting to get it into stable kernels,
> which are supposed to only have bug and regression fixes applied?

Look at the cover letter, it is fixing a bug.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Thomas Petazzoni]

Dear Simon Guinot,

On Mon, 15 Jun 2015 16:27:22 +0200, Simon Guinot wrote:
> This patch introduces the tx_csum_limit DT property. This allows to
> configure the maximum frame size for which the Ethernet controller is
> able to perform TCP/IP checksumming. If MTU is set to a value greater
> than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
> are disabled.
> 
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Cc: <stable@vger.kernel.org> # v3.8+
> ---
>  .../bindings/net/marvell-armada-370-neta.txt       |  3 +++
>  drivers/net/ethernet/marvell/mvneta.c              | 25 +++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
> index 750d577e8083..db48c83ff0f5 100644
> --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
> +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
> @@ -8,6 +8,9 @@ Required properties:
>  - phy-mode: See ethernet.txt file in the same directory
>  - clocks: a pointer to the reference clock for this device.
>  
> +Optional properties:
> +- tx_csum_limit: max tx packet size for hardware checksum.

To be honest, I'd prefer to have a different compatible string to
identify the two different versions of the hardware block.

The current armada-370-neta would limit the HW checksumming features to
packets smaller than 1600 bytes, while a new armada-xp-neta would not
have this limit.

Yet another case where we should have used "armada-<soc>-neta",
"armada-370-neta" in the .dtsi files for each SoC so that such
modification do not require changing the Device Trees.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

On Mon, Jun 15, 2015 at 04:49:52PM +0200, Thomas Petazzoni wrote:
> Dear Simon Guinot,
> 
> On Mon, 15 Jun 2015 16:27:22 +0200, Simon Guinot wrote:
> > This patch introduces the tx_csum_limit DT property. This allows to
> > configure the maximum frame size for which the Ethernet controller is
> > able to perform TCP/IP checksumming. If MTU is set to a value greater
> > than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
> > are disabled.
> > 
> > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> > Cc: <stable@vger.kernel.org> # v3.8+
> > ---
> >  .../bindings/net/marvell-armada-370-neta.txt       |  3 +++
> >  drivers/net/ethernet/marvell/mvneta.c              | 25 +++++++++++++++++++++-
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
> > index 750d577e8083..db48c83ff0f5 100644
> > --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
> > +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
> > @@ -8,6 +8,9 @@ Required properties:
> >  - phy-mode: See ethernet.txt file in the same directory
> >  - clocks: a pointer to the reference clock for this device.
> >  
> > +Optional properties:
> > +- tx_csum_limit: max tx packet size for hardware checksum.
> 
> To be honest, I'd prefer to have a different compatible string to
> identify the two different versions of the hardware block.
> 
> The current armada-370-neta would limit the HW checksumming features to
> packets smaller than 1600 bytes, while a new armada-xp-neta would not
> have this limit.

This was also my first idea. But by doing this, we take the risk of
losing the HW checksumming feature with jumbo frames on some currently
working Armada XP setups. This may happen for example if a user is able
to update the kernel but not the on-board DTB. In order to fix a feature
on a SoC, we are breaking the DTB-kernel compatibility for the very same
feature on an another SoC. I am not sure it is OK.

Are you comfortable with that ?

By adding a new optional property, at least we ensure that the things
will still work the same way with Armada-XP SoCs (for every DTB-Linux
combination).

Please, let me know if you still want to introduce a compatible string
for Armada-XP.

Simon

> 
> Yet another case where we should have used "armada-<soc>-neta",
> "armada-370-neta" in the .dtsi files for each SoC so that such
> modification do not require changing the Device Trees.
> 
> Best regards,
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Thomas Petazzoni]

Hello,

On Mon, 15 Jun 2015 17:54:41 +0200, Simon Guinot wrote:

> > The current armada-370-neta would limit the HW checksumming features to
> > packets smaller than 1600 bytes, while a new armada-xp-neta would not
> > have this limit.
> 
> This was also my first idea. But by doing this, we take the risk of
> losing the HW checksumming feature with jumbo frames on some currently
> working Armada XP setups. This may happen for example if a user is able
> to update the kernel but not the on-board DTB. In order to fix a feature
> on a SoC, we are breaking the DTB-kernel compatibility for the very same
> feature on an another SoC. I am not sure it is OK.
> 
> Are you comfortable with that ?

It's either that, or keep a not-ideal solution for this problem. I'm
not a strong believer in the need of enforcing the DT as a stable ABI,
and I am not aware of anyone using Marvell platforms with a DTB that
isn't changed together with the kernel. So my preference goes to fixing
the problem properly, rather than doing non-ideal solutions.

Though this is just my own opinion, and I am not a maintainer of the
mach-mvebu platform.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property

[Jason Cooper]

Thomas, Simon,

On Tue, Jun 16, 2015 at 12:00:34PM +0200, Thomas Petazzoni wrote:
> On Mon, 15 Jun 2015 17:54:41 +0200, Simon Guinot wrote:
> > > The current armada-370-neta would limit the HW checksumming features to
> > > packets smaller than 1600 bytes, while a new armada-xp-neta would not
> > > have this limit.
> > 
> > This was also my first idea. But by doing this, we take the risk of
> > losing the HW checksumming feature with jumbo frames on some currently
> > working Armada XP setups. This may happen for example if a user is able
> > to update the kernel but not the on-board DTB. In order to fix a feature
> > on a SoC, we are breaking the DTB-kernel compatibility for the very same
> > feature on an another SoC. I am not sure it is OK.

Frankly, this isn't a realistic scenario.  We've said from day one that
if the dtb is provided with the board that it needs to be updateable.
For exactly these kinds of situations.  Also, Thomas' assessment is
correct, everyone we've ever spoken to is keeping the dtb in sync with
the kernel.

As long as a board with the old dtb boots a newer kernel without
crashing, then it's fine.  afaict in this situation, the updated driver
should limit HW checksumming for packets >1600 bytes if the compatible
string is 'armada-370-neta'.  Regardless of actual SoC underneath.
If the driver gets 'armada-xp-neta' then there is no checksum limit.

Users with an Armada XP SoC and an old dtb will need to upgrade the dtb
in order to make use of HW checksumming on jumbo packets with newer
kernels.  Seems sane to me.

thx,

Jason.

[PATCH 2/2] ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

[Simon Guinot]

The Ethernet controller found in Armada 370 SoCs don't support TCP/IP
checksumming with frames largest than 1600 Bytes.

This patch sets accordingly the tx_csum_limit property in Ethernet nodes
for Armada 370.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: <stable@vger.kernel.org> # v3.8+
---
 arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 00b50db57c9c..046489df351d 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -307,6 +307,14 @@
 					dmacap,memset;
 				};
 			};
+
+			ethernet@70000 {
+				tx_csum_limit = <1600>;
+			};
+
+			ethernet@74000 {
+				tx_csum_limit = <1600>;
+			};
 		};
 	};
 };
-- 
2.1.4