[PATCHv3 net 0/5] nfp: MTU fixes for net

Hi Dave!

This is the first part of MTU reconfiguration fixes.  These
are the patches which I would like to get into -net.  The
requested overhaul of the way MTU configuration is done is
posted as a separate series targeted at net-next.

Thanks!


Jakub Kicinski (5):
  nfp: return error if MTU change fails
  nfp: free buffers before changing MTU
  nfp: correct RX buffer length calculation
  nfp: fix RX buffer length validation
  nfp: don't trust netif_running() in debug code

 .../net/ethernet/netronome/nfp/nfp_net_common.c    | 42 ++++++++++------------
 .../net/ethernet/netronome/nfp/nfp_net_debugfs.c   |  4 +--
 2 files changed, 20 insertions(+), 26 deletions(-)

-- 
1.9.1

When reopening device fails after MTU change, let the userspace
know.  MTU remains changed even though error is returned, this
is what all ethernet devices are doing.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Rolf Neugebauer <redacted>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 43c618bafdb6..006d9600240f 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c

@@ -1911,6 +1911,7 @@ static void nfp_net_set_rx_mode(struct net_device *netdev)
 static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct nfp_net *nn = netdev_priv(netdev);
+	int ret = 0;
 	u32 tmp;
 
 	nn_dbg(nn, "New MTU = %d\n", new_mtu);

@@ -1929,10 +1930,10 @@ static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu)
 	/* restart if running */
 	if (netif_running(netdev)) {
 		nfp_net_netdev_close(netdev);
-		nfp_net_netdev_open(netdev);
+		ret = nfp_net_netdev_open(netdev);
 	}
 
-	return 0;
+	return ret;
 }
 
 static struct rtnl_link_stats64 *nfp_net_stat64(struct net_device *netdev,

-- 
1.9.1

For freeing DMA buffers we depend on nfp_net.fl_bufsz having the same
value as during allocation therefore in .ndo_change_mtu we must first
free the buffers and then change the setting.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Rolf Neugebauer <redacted>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 006d9600240f..b381682de3d6 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c

@@ -1921,17 +1921,17 @@ static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu)
 		return -EINVAL;
 	}
 
+	if (netif_running(netdev))
+		nfp_net_netdev_close(netdev);
+
 	netdev->mtu = new_mtu;
 
 	/* Freelist buffer size rounded up to the nearest 1K */
 	tmp = new_mtu + ETH_HLEN + VLAN_HLEN + NFP_NET_MAX_PREPEND;
 	nn->fl_bufsz = roundup(tmp, 1024);
 
-	/* restart if running */
-	if (netif_running(netdev)) {
-		nfp_net_netdev_close(netdev);
+	if (netif_running(netdev))
 		ret = nfp_net_netdev_open(netdev);
-	}
 
 	return ret;
 }

-- 
1.9.1

Meaning of data_len and meta_len RX WB descriptor fields depend
slightly on whether rx_offset is dynamic or not.  For dynamic
offsets data_len includes meta_len.  This makes the code harder
to follow, in fact our RX buffer length check is incorrect -
we are comparing allocation length to data_len while we should
also account for meta_len.

Let's adjust the values of data_len and meta_len to their natural
meaning and simplify the logic.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Rolf Neugebauer <redacted>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 553ae64e2f7f..070645f9bc21 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c

@@ -1259,22 +1259,19 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
 
 		meta_len = rxd->rxd.meta_len_dd & PCIE_DESC_RX_META_LEN_MASK;
 		data_len = le16_to_cpu(rxd->rxd.data_len);
+		/* For dynamic offset data_len includes meta_len, adjust */
+		if (nn->rx_offset == NFP_NET_CFG_RX_OFFSET_DYNAMIC)
+			data_len -= meta_len;
+		else
+			meta_len = nn->rx_offset;
 
-		if (WARN_ON_ONCE(data_len > nn->fl_bufsz)) {
+		if (WARN_ON_ONCE(meta_len + data_len > nn->fl_bufsz)) {
 			dev_kfree_skb_any(skb);
 			continue;
 		}
 
-		if (nn->rx_offset == NFP_NET_CFG_RX_OFFSET_DYNAMIC) {
-			/* The packet data starts after the metadata */
-			skb_reserve(skb, meta_len);
-		} else {
-			/* The packet data starts at a fixed offset */
-			skb_reserve(skb, nn->rx_offset);
-		}
-
-		/* Adjust the SKB for the dynamic meta data pre-pended */
-		skb_put(skb, data_len - meta_len);
+		skb_reserve(skb, meta_len);
+		skb_put(skb, data_len);
 
 		nfp_net_set_hash(nn->netdev, skb, rxd);
 

-- 
1.9.1

When calculating the RX buffer length we need to account for
up to 2 VLAN tags and up to 8 MPLS labels.  Rounding up to 1k
is an relic of a distant past and can be removed.  While at
it also remove trivial print statement.

Signed-off-by: Jakub Kicinski <redacted>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index b381682de3d6..553ae64e2f7f 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c

@@ -61,6 +61,7 @@
 
 #include <linux/ktime.h>
 
+#include <net/mpls.h>
 #include <net/vxlan.h>
 
 #include "nfp_net_ctrl.h"

@@ -1912,9 +1913,6 @@ static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct nfp_net *nn = netdev_priv(netdev);
 	int ret = 0;
-	u32 tmp;
-
-	nn_dbg(nn, "New MTU = %d\n", new_mtu);
 
 	if (new_mtu < 68 || new_mtu > nn->max_mtu) {
 		nn_err(nn, "New MTU (%d) is not valid\n", new_mtu);

@@ -1925,10 +1923,8 @@ static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu)
 		nfp_net_netdev_close(netdev);
 
 	netdev->mtu = new_mtu;
-
-	/* Freelist buffer size rounded up to the nearest 1K */
-	tmp = new_mtu + ETH_HLEN + VLAN_HLEN + NFP_NET_MAX_PREPEND;
-	nn->fl_bufsz = roundup(tmp, 1024);
+	nn->fl_bufsz = NFP_NET_MAX_PREPEND + ETH_HLEN + VLAN_HLEN * 2 +
+		MPLS_HLEN * 8 + new_mtu;
 
 	if (netif_running(netdev))
 		ret = nfp_net_netdev_open(netdev);

-- 
1.9.1

Since change_mtu() can fail and leave us with netif_running()
returning true even though all rings were freed - we should
look at NFP_NET_CFG_CTRL_ENABLE flag to determine if device
is really opened.

Signed-off-by: Jakub Kicinski <redacted>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_debugfs.c b/drivers/net/ethernet/netronome/nfp/nfp_net_debugfs.c
index 4c97c713121c..7af404d492cc 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_debugfs.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_debugfs.c

@@ -52,7 +52,7 @@ static int nfp_net_debugfs_rx_q_read(struct seq_file *file, void *data)
 	if (!rx_ring->r_vec || !rx_ring->r_vec->nfp_net)
 		goto out;
 	nn = rx_ring->r_vec->nfp_net;
-	if (!netif_running(nn->netdev))
+	if (!(nn->ctrl & NFP_NET_CFG_CTRL_ENABLE))
 		goto out;
 
 	rxd_cnt = rx_ring->cnt;

@@ -127,7 +127,7 @@ static int nfp_net_debugfs_tx_q_read(struct seq_file *file, void *data)
 	if (!tx_ring->r_vec || !tx_ring->r_vec->nfp_net)
 		goto out;
 	nn = tx_ring->r_vec->nfp_net;
-	if (!netif_running(nn->netdev))
+	if (!(nn->ctrl & NFP_NET_CFG_CTRL_ENABLE))
 		goto out;
 
 	txd_cnt = tx_ring->cnt;

-- 
1.9.1

From: Jakub Kicinski <redacted>
Date: Thu, 18 Feb 2016 20:38:13 +0000

Since change_mtu() can fail and leave us with netif_running()
returning true even though all rings were freed - we should
look at NFP_NET_CFG_CTRL_ENABLE flag to determine if device
is really opened.

Signed-off-by: Jakub Kicinski <redacted>

This is exactly why I don't like how you are doing your MTU change at
all.

You must not make the device inoperative if you simply cannot perform
the MTU change.  I'm pretty sure I've told you this already, this
whole ->close(), MTU change, ->open() OOPS THAT FAILED sequence is a
non-starter.  You can't do this.

You are leaving the netdev object in an illegal state when this
happens.

You must return from ->change_mtu() with the device in the UP
and running state if you cannot make the MTU change.

I don't care how invasive it is, you must fix this properly if
you want to fix this because as-is this patch series is a cure
worse than the disease.

I'm not applying any of your currently submitted changes, both for net
and net-next, sorry.

On 2/20/16, David Miller [off-list ref] wrote:

From: Jakub Kicinski <redacted>
Date: Thu, 18 Feb 2016 20:38:13 +0000

quoted

Since change_mtu() can fail and leave us with netif_running()
returning true even though all rings were freed - we should
look at NFP_NET_CFG_CTRL_ENABLE flag to determine if device
is really opened.

Signed-off-by: Jakub Kicinski <redacted>

This is exactly why I don't like how you are doing your MTU change at
all.

You must not make the device inoperative if you simply cannot perform
the MTU change.  I'm pretty sure I've told you this already, this
whole ->close(), MTU change, ->open() OOPS THAT FAILED sequence is a
non-starter.  You can't do this.

OK, I just wanted as little changes as possbile
here since we are at rc5 already. I should've
really caught that before upstreaming the driver :/

You are leaving the netdev object in an illegal state when this
happens.

After the net-next series this could only happen
if FW crashed and stopped responding to
commands. Since this is VF driver I dont see
anything I can do with crashed FW :(  Should I
close the device from the driver side?

Could you please look at the net-next series and
tell me if its a step in the right direction? I feel a
bit puzzled, I thought the next series does
exactly what you wanted.

Thank you for your patience...