[PATCH net 0/3] nfp: port enumeration change and FW ABI adjustment

[PATCH net 0/3] nfp: port enumeration change and FW ABI adjustment

[Jakub Kicinski]

Hi!

This set changes the way ports are numbered internally to avoid MAC address
changes and invalid link information when breakout is configured.  Second
patch gets rid of old way of looking up MAC addresses in device information
which caused all this confusion.

Patch 3 is a small adjustment to the new FW ABI version we introduced in
this release cycle.


To clarify these are intended for 4.13, naturally.

Jakub Kicinski (3):
  nfp: improve order of interfaces in breakout mode
  nfp: remove legacy MAC address lookup
  nfp: default to chained metadata prepend format

 drivers/net/ethernet/netronome/nfp/flower/main.c   |  3 +-
 drivers/net/ethernet/netronome/nfp/nfp_app_nic.c   |  2 +-
 drivers/net/ethernet/netronome/nfp/nfp_main.h      |  5 +--
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |  9 ++++-
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c  | 44 +++++-----------------
 drivers/net/ethernet/netronome/nfp/nfp_port.c      | 14 +++----
 6 files changed, 28 insertions(+), 49 deletions(-)

-- 
2.11.0

[PATCH net 1/3] nfp: improve order of interfaces in breakout mode

[Jakub Kicinski]

For historical reasons we enumerate the vNICs in order.  This means
that if user configures breakout on a multiport card, the first
interface of the second port will have its MAC address changed.

What's worse, when moved from static information (HWInfo) to using
management FW (NSP), more features started depending on the port ids.
Right now in case of breakout first subport of the second port and
second subport of the first port will have their link info swapped.

Revise the ordering scheme so that first subport maintains its address.
Side effect of this change is that we will use base lane ids in
devlink (i.e. 40G ports will be 4 ids apart), e.g.:

pci/0000:04:00.0/0: type eth netdev p6p1
pci/0000:04:00.0/4: type eth netdev p6p2

Note that behaviour of phys_port_id is not changed since there is
a separate id number for the subport there.

Fixes: ec8b1fbe682d ("nfp: support port splitting via devlink")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_main.h     |  2 --
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c | 10 +++++-----
 drivers/net/ethernet/netronome/nfp/nfp_port.c     | 14 +++++++-------
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
index a08cfba7e68e..365252ab4660 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
@@ -149,8 +149,6 @@ void nfp_net_pci_remove(struct nfp_pf *pf);
 int nfp_hwmon_register(struct nfp_pf *pf);
 void nfp_hwmon_unregister(struct nfp_pf *pf);
 
-struct nfp_eth_table_port *
-nfp_net_find_port(struct nfp_eth_table *eth_tbl, unsigned int id);
 void
 nfp_net_get_mac_addr(struct nfp_pf *pf, struct nfp_port *port, unsigned int id);
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index c85a2f18c4df..bfcdada29cc0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -126,13 +126,13 @@ nfp_net_get_mac_addr(struct nfp_pf *pf, struct nfp_port *port, unsigned int id)
 	ether_addr_copy(port->netdev->perm_addr, mac_addr);
 }
 
-struct nfp_eth_table_port *
-nfp_net_find_port(struct nfp_eth_table *eth_tbl, unsigned int id)
+static struct nfp_eth_table_port *
+nfp_net_find_port(struct nfp_eth_table *eth_tbl, unsigned int index)
 {
 	int i;
 
 	for (i = 0; eth_tbl && i < eth_tbl->count; i++)
-		if (eth_tbl->ports[i].eth_index == id)
+		if (eth_tbl->ports[i].index == index)
 			return &eth_tbl->ports[i];
 
 	return NULL;
@@ -202,7 +202,7 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
 static struct nfp_net *
 nfp_net_pf_alloc_vnic(struct nfp_pf *pf, bool needs_netdev,
 		      void __iomem *ctrl_bar, void __iomem *qc_bar,
-		      int stride, unsigned int eth_id)
+		      int stride, unsigned int id)
 {
 	u32 tx_base, rx_base, n_tx_rings, n_rx_rings;
 	struct nfp_net *nn;
@@ -228,7 +228,7 @@ nfp_net_pf_alloc_vnic(struct nfp_pf *pf, bool needs_netdev,
 	nn->stride_tx = stride;
 
 	if (needs_netdev) {
-		err = nfp_app_vnic_init(pf->app, nn, eth_id);
+		err = nfp_app_vnic_init(pf->app, nn, id);
 		if (err) {
 			nfp_net_free(nn);
 			return ERR_PTR(err);
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c
index 776e54dd5dd0..e42644dbb865 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
@@ -184,24 +184,24 @@ nfp_port_get_phys_port_name(struct net_device *netdev, char *name, size_t len)
 int nfp_port_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
 			   struct nfp_port *port, unsigned int id)
 {
-	port->eth_id = id;
-	port->eth_port = nfp_net_find_port(pf->eth_tbl, id);
-
 	/* Check if vNIC has external port associated and cfg is OK */
-	if (!port->eth_port) {
+	if (!pf->eth_tbl || id >= pf->eth_tbl->count) {
 		nfp_err(app->cpp,
-			"NSP port entries don't match vNICs (no entry for port #%d)\n",
+			"NSP port entries don't match vNICs (no entry %d)\n",
 			id);
 		return -EINVAL;
 	}
-	if (port->eth_port->override_changed) {
+	if (pf->eth_tbl->ports[id].override_changed) {
 		nfp_warn(app->cpp,
 			 "Config changed for port #%d, reboot required before port will be operational\n",
-			 id);
+			 pf->eth_tbl->ports[id].index);
 		port->type = NFP_PORT_INVALID;
 		return 0;
 	}
 
+	port->eth_port = &pf->eth_tbl->ports[id];
+	port->eth_id = pf->eth_tbl->ports[id].index;
+
 	return 0;
 }
 
-- 
2.11.0

[PATCH net 2/3] nfp: remove legacy MAC address lookup

[Jakub Kicinski]

The legacy MAC address lookup doesn't work well with breakout
cables.  We are probably better off picking random addresses
than the wrong ones in the theoretical scenario where management
FW didn't tell us what the port config is.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/main.c  |  3 +-
 drivers/net/ethernet/netronome/nfp/nfp_app_nic.c  |  2 +-
 drivers/net/ethernet/netronome/nfp/nfp_main.h     |  3 +-
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c | 34 ++++-------------------
 4 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index 5fe6d3582597..fc10f27e0a0c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -245,8 +245,7 @@ nfp_flower_spawn_phy_reprs(struct nfp_app *app, struct nfp_flower_priv *priv)
 		}
 
 		SET_NETDEV_DEV(reprs->reprs[phys_port], &priv->nn->pdev->dev);
-		nfp_net_get_mac_addr(app->pf, port,
-				     eth_tbl->ports[i].eth_index);
+		nfp_net_get_mac_addr(app->pf, port);
 
 		cmsg_port_id = nfp_flower_cmsg_phys_port(phys_port);
 		err = nfp_repr_init(app, reprs->reprs[phys_port],
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app_nic.c b/drivers/net/ethernet/netronome/nfp/nfp_app_nic.c
index c11a6c34e217..4e37c81f9eaf 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_app_nic.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_app_nic.c
@@ -69,7 +69,7 @@ int nfp_app_nic_vnic_init(struct nfp_app *app, struct nfp_net *nn,
 	if (err)
 		return err < 0 ? err : 0;
 
-	nfp_net_get_mac_addr(app->pf, nn->port, id);
+	nfp_net_get_mac_addr(app->pf, nn->port);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
index 365252ab4660..6922410806db 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
@@ -149,8 +149,7 @@ void nfp_net_pci_remove(struct nfp_pf *pf);
 int nfp_hwmon_register(struct nfp_pf *pf);
 void nfp_hwmon_unregister(struct nfp_pf *pf);
 
-void
-nfp_net_get_mac_addr(struct nfp_pf *pf, struct nfp_port *port, unsigned int id);
+void nfp_net_get_mac_addr(struct nfp_pf *pf, struct nfp_port *port);
 
 bool nfp_ctrl_tx(struct nfp_net *nn, struct sk_buff *skb);
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index bfcdada29cc0..5797dbf2b507 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -84,46 +84,22 @@ static int nfp_is_ready(struct nfp_pf *pf)
  * nfp_net_get_mac_addr() - Get the MAC address.
  * @pf:       NFP PF handle
  * @port:     NFP port structure
- * @id:	      NFP port id
  *
  * First try to get the MAC address from NSP ETH table. If that
- * fails try HWInfo.  As a last resort generate a random address.
+ * fails generate a random address.
  */
-void
-nfp_net_get_mac_addr(struct nfp_pf *pf, struct nfp_port *port, unsigned int id)
+void nfp_net_get_mac_addr(struct nfp_pf *pf, struct nfp_port *port)
 {
 	struct nfp_eth_table_port *eth_port;
-	u8 mac_addr[ETH_ALEN];
-	const char *mac_str;
-	char name[32];
 
 	eth_port = __nfp_port_get_eth_port(port);
-	if (eth_port) {
-		ether_addr_copy(port->netdev->dev_addr, eth_port->mac_addr);
-		ether_addr_copy(port->netdev->perm_addr, eth_port->mac_addr);
-		return;
-	}
-
-	snprintf(name, sizeof(name), "eth%d.mac", id);
-
-	mac_str = nfp_hwinfo_lookup(pf->hwinfo, name);
-	if (!mac_str) {
-		nfp_warn(pf->cpp, "Can't lookup MAC address. Generate\n");
-		eth_hw_addr_random(port->netdev);
-		return;
-	}
-
-	if (sscanf(mac_str, "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx",
-		   &mac_addr[0], &mac_addr[1], &mac_addr[2],
-		   &mac_addr[3], &mac_addr[4], &mac_addr[5]) != 6) {
-		nfp_warn(pf->cpp, "Can't parse MAC address (%s). Generate.\n",
-			 mac_str);
+	if (!eth_port) {
 		eth_hw_addr_random(port->netdev);
 		return;
 	}
 
-	ether_addr_copy(port->netdev->dev_addr, mac_addr);
-	ether_addr_copy(port->netdev->perm_addr, mac_addr);
+	ether_addr_copy(port->netdev->dev_addr, eth_port->mac_addr);
+	ether_addr_copy(port->netdev->perm_addr, eth_port->mac_addr);
 }
 
 static struct nfp_eth_table_port *
-- 
2.11.0

[PATCH net 3/3] nfp: default to chained metadata prepend format

[Jakub Kicinski]

ABI 4.x introduced the chained metadata format and made it the
only one possible.  There are cases, however, where the old
format is preferred - mostly to make interoperation with VFs
using ABI 3.x easier for the datapath.  In ABI 5.x we allowed
for more flexibility by selecting the metadata format based
on capabilities.  The default was left to non-chained.

In case of fallback traffic, there is no capability telling the
driver there may be chained metadata.  With a very stripped-
-down FW the default old metadata format would be selected
making the driver drop all fallback traffic.

This patch changes the default selection in the driver. It
should not hurt with old firmwares, because if they don't
advertise RSS they will not produce metadata anyway.  New
firmwares advertising ABI 5.x, however, can depend on the
driver defaulting to chained format.

Fixes: f9380629fafc ("nfp: advertise support for NFD ABI 0.5")
Suggested-by: Michael Rapson <michael.rapson@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 30f82b41d400..18750ff0ede6 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3719,10 +3719,17 @@ int nfp_net_init(struct nfp_net *nn)
 	nn->cap = nn_readl(nn, NFP_NET_CFG_CAP);
 	nn->max_mtu = nn_readl(nn, NFP_NET_CFG_MAX_MTU);
 
-	/* Chained metadata is signalled by capabilities except in version 4 */
+	/* ABI 4.x and ctrl vNIC always use chained metadata, in other cases
+	 * we allow use of non-chained metadata if RSS(v1) is the only
+	 * advertised capability requiring metadata.
+	 */
 	nn->dp.chained_metadata_format = nn->fw_ver.major == 4 ||
 					 !nn->dp.netdev ||
+					 !(nn->cap & NFP_NET_CFG_CTRL_RSS) ||
 					 nn->cap & NFP_NET_CFG_CTRL_CHAIN_META;
+	/* RSS(v1) uses non-chained metadata format, except in ABI 4.x where
+	 * it has the same meaning as RSSv2.
+	 */
 	if (nn->dp.chained_metadata_format && nn->fw_ver.major != 4)
 		nn->cap &= ~NFP_NET_CFG_CTRL_RSS;
 
-- 
2.11.0

Re: [PATCH net 0/3] nfp: port enumeration change and FW ABI adjustment

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue,  4 Jul 2017 02:27:18 -0700

> This set changes the way ports are numbered internally to avoid MAC address
> changes and invalid link information when breakout is configured.  Second
> patch gets rid of old way of looking up MAC addresses in device information
> which caused all this confusion.
> 
> Patch 3 is a small adjustment to the new FW ABI version we introduced in
> this release cycle.
> 
> 
> To clarify these are intended for 4.13, naturally.

net-next is closed, and it looks like these are not really bug fixes
but a behavior adjustment and therefore more like a new feature.

Thanks.

Re: [PATCH net 0/3] nfp: port enumeration change and FW ABI adjustment

[Jakub Kicinski]

On Tue, 04 Jul 2017 03:29:49 -0700 (PDT), David Miller wrote:
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Tue,  4 Jul 2017 02:27:18 -0700
> 
> > This set changes the way ports are numbered internally to avoid MAC address
> > changes and invalid link information when breakout is configured.  Second
> > patch gets rid of old way of looking up MAC addresses in device information
> > which caused all this confusion.
> > 
> > Patch 3 is a small adjustment to the new FW ABI version we introduced in
> > this release cycle.
> > 
> > 
> > To clarify these are intended for 4.13, naturally.  
> 
> net-next is closed, and it looks like these are not really bug fixes
> but a behavior adjustment and therefore more like a new feature.

Ah, silly me, I had these ready on Friday but didn't want to participate
in the just-before-the-merge-window rush :(

Could you be pursued to take patch 3?  Without it our current Flower
firmware does not work with the 5.x ABI, if we get a kernel released in
this state we will have to bump the ABI again, effectively making the
4.13 Flower offload unusable :(

Patch 1 fixes ethtool dumping and configuring wrong ports.  I had a
report from someone using a 2x40 card where one port was broken out to
4x10 and ethtool was swapping info on the 40G port and one of the 10G
ports.  I didn't notice this in testing, because I was testing with two
NFPs back to back, so the port swap sort of cancelled itself out.  I
should've made it clearer in the commit message that the patch
effectively fixes the association between management FW idea of ports
and the netdevs.  This is lower priority than patch 3, though.

Patch 2 can be dropped, I tossed it in unnecessarily.

Re: [PATCH net 0/3] nfp: port enumeration change and FW ABI adjustment

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 4 Jul 2017 13:52:07 -0700

> On Tue, 04 Jul 2017 03:29:49 -0700 (PDT), David Miller wrote:
>> From: Jakub Kicinski <jakub.kicinski@netronome.com>
>> Date: Tue,  4 Jul 2017 02:27:18 -0700
>> 
>> > This set changes the way ports are numbered internally to avoid MAC address
>> > changes and invalid link information when breakout is configured.  Second
>> > patch gets rid of old way of looking up MAC addresses in device information
>> > which caused all this confusion.
>> > 
>> > Patch 3 is a small adjustment to the new FW ABI version we introduced in
>> > this release cycle.
>> > 
>> > 
>> > To clarify these are intended for 4.13, naturally.  
>> 
>> net-next is closed, and it looks like these are not really bug fixes
>> but a behavior adjustment and therefore more like a new feature.
> 
> Ah, silly me, I had these ready on Friday but didn't want to participate
> in the just-before-the-merge-window rush :(

I've applied this series, thanks for explaining the situation more
clearly.