[PATCH net-next v2 0/9] introduce flower offload capabilities

[PATCH net-next v2 0/9] introduce flower offload capabilities

[Simon Horman]

Hi,

this series adds flower offload to the NFP driver. It builds on recent
work to add representor and a skeleton flower app - now the app does what
its name says.

In general the approach taken is to allow some flows within
the universe of possible flower matches and tc actions to be offloaded.
It is planned that this support will grow over time but the support
offered by this patch-set seems to be a reasonable starting point.

Key Changes since RFC:
* Revised locking scheme for flows
* Make generalise tc_setup NDO
* Addressed other review of RFC
* Dropped RFC designation

Pieter Jansen van Vuuren (7):
  nfp: provide infrastructure for offloading flower based TC filters
  nfp: extend flower add flow offload
  nfp: extend flower matching capabilities
  nfp: add basic action capabilities to flower offloads
  nfp: add metadata to each flow offload
  nfp: add a stats handler for flower offloads
  nfp: add control message passing capabilities to flower offloads

Simon Horman (2):
  net: switchdev: add SET_SWITCHDEV_OPS helper
  nfp: add phys_switch_id support

 drivers/net/ethernet/netronome/nfp/Makefile        |   6 +-
 drivers/net/ethernet/netronome/nfp/flower/action.c | 211 +++++++++
 drivers/net/ethernet/netronome/nfp/flower/cmsg.c   |  11 +-
 drivers/net/ethernet/netronome/nfp/flower/cmsg.h   | 202 +++++++++
 drivers/net/ethernet/netronome/nfp/flower/main.c   |  32 +-
 drivers/net/ethernet/netronome/nfp/flower/main.h   | 159 +++++++
 drivers/net/ethernet/netronome/nfp/flower/match.c  | 292 ++++++++++++
 .../net/ethernet/netronome/nfp/flower/metadata.c   | 497 +++++++++++++++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    | 397 ++++++++++++++++
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |  17 +-
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c  |   8 +
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.h  |   9 +
 drivers/net/ethernet/netronome/nfp/nfp_port.c      |  43 ++
 drivers/net/ethernet/netronome/nfp/nfp_port.h      |   8 +
 include/net/switchdev.h                            |   4 +
 15 files changed, 1869 insertions(+), 27 deletions(-)
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/action.c
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/main.h
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/match.c
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/metadata.c
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/offload.c

-- 
2.1.4

[PATCH net-next v2 1/9] net: switchdev: add SET_SWITCHDEV_OPS helper

[Simon Horman]

Add a helper to allow switchdev ops to be set if NET_SWITCHDEV is configured
and do nothing otherwise. This allows for slightly cleaner code which
uses switchdev but does not select NET_SWITCHDEV.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 include/net/switchdev.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index c784a6ac6ef1..8ae9e3b6392e 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -217,6 +217,8 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
 
 bool switchdev_port_same_parent_id(struct net_device *a,
 				   struct net_device *b);
+
+#define SWITCHDEV_SET_OPS(netdev, ops) ((netdev)->switchdev_ops = (ops))
 #else
 
 static inline void switchdev_deferred_process(void)
@@ -322,6 +324,8 @@ static inline bool switchdev_port_same_parent_id(struct net_device *a,
 	return false;
 }
 
+#define SWITCHDEV_SET_OPS(netdev, ops) do {} while (0)
+
 #endif
 
 #endif /* _LINUX_SWITCHDEV_H_ */
-- 
2.1.4

[PATCH net-next v2 2/9] nfp: add phys_switch_id support

[Simon Horman]

Add phys_switch_id support by allowing lookup of
SWITCHDEV_ATTR_ID_PORT_PARENT_ID via the nfp_repr_port_attr_get
switchdev operation.

This is visible to user-space in the phys_switch_id attribute
of a netdev.

e.g.
cd /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0
find . -name phys_switch_id | xargs grep .
./net/eth3/phys_switch_id:00154d1300bd
./net/eth4/phys_switch_id:00154d1300bd
./net/eth2/phys_switch_id:00154d1300bd
grep: ./net/eth5/phys_switch_id: Operation not supported

In the above eth2 and eth3 and representor netdevs for the first and second
physical port. eth4 is the representor for the PF. And eth5 is the PF netdev.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |  3 +++
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c  |  2 ++
 drivers/net/ethernet/netronome/nfp/nfp_port.c      | 28 ++++++++++++++++++++++
 drivers/net/ethernet/netronome/nfp/nfp_port.h      |  3 +++
 4 files changed, 36 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 2e728543e840..b5834525c5f0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -64,6 +64,7 @@
 #include <linux/vmalloc.h>
 #include <linux/ktime.h>
 
+#include <net/switchdev.h>
 #include <net/vxlan.h>
 
 #include "nfpcore/nfp_nsp.h"
@@ -3703,6 +3704,8 @@ static void nfp_net_netdev_init(struct nfp_net *nn)
 	netdev->netdev_ops = &nfp_net_netdev_ops;
 	netdev->watchdog_timeo = msecs_to_jiffies(5 * 1000);
 
+	SWITCHDEV_SET_OPS(netdev, &nfp_port_switchdev_ops);
+
 	/* MTU range: 68 - hw-specific max */
 	netdev->min_mtu = ETH_MIN_MTU;
 	netdev->max_mtu = nn->max_mtu;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index 046b89eb4cf2..bc9108071e5b 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -35,6 +35,7 @@
 #include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/lockdep.h>
 #include <net/dst_metadata.h>
+#include <net/switchdev.h>
 
 #include "nfpcore/nfp_cpp.h"
 #include "nfpcore/nfp_nsp.h"
@@ -299,6 +300,7 @@ int nfp_repr_init(struct nfp_app *app, struct net_device *netdev,
 	repr->dst->u.port_info.lower_dev = pf_netdev;
 
 	netdev->netdev_ops = &nfp_repr_netdev_ops;
+	SWITCHDEV_SET_OPS(netdev, &nfp_port_switchdev_ops);
 
 	err = register_netdev(netdev);
 	if (err)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c
index 0b44952945d8..c95215eb87c2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
@@ -59,6 +59,34 @@ struct nfp_port *nfp_port_from_netdev(struct net_device *netdev)
 	return NULL;
 }
 
+static int
+nfp_port_attr_get(struct net_device *netdev, struct switchdev_attr *attr)
+{
+	struct nfp_port *port;
+
+	port = nfp_port_from_netdev(netdev);
+	if (!port)
+		return -EOPNOTSUPP;
+
+	switch (attr->id) {
+	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID: {
+		const u8 *serial;
+		/* N.B: attr->u.ppid.id is binary data */
+		attr->u.ppid.id_len = nfp_cpp_serial(port->app->cpp, &serial);
+		memcpy(&attr->u.ppid.id, serial, attr->u.ppid.id_len);
+		break;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+const struct switchdev_ops nfp_port_switchdev_ops = {
+	.switchdev_port_attr_get	= nfp_port_attr_get,
+};
+
 struct nfp_port *
 nfp_port_from_id(struct nfp_pf *pf, enum nfp_port_type type, unsigned int id)
 {
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.h b/drivers/net/ethernet/netronome/nfp/nfp_port.h
index 57d852a4ca59..de60cacd3362 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.h
@@ -35,6 +35,7 @@
 #define _NFP_PORT_H_
 
 #include <net/devlink.h>
+#include <net/switchdev.h>
 
 struct net_device;
 struct nfp_app;
@@ -106,6 +107,8 @@ struct nfp_port {
 	struct list_head port_list;
 };
 
+extern const struct switchdev_ops nfp_port_switchdev_ops;
+
 struct nfp_port *nfp_port_from_netdev(struct net_device *netdev);
 struct nfp_port *
 nfp_port_from_id(struct nfp_pf *pf, enum nfp_port_type type, unsigned int id);
-- 
2.1.4

[PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Adds a flower based TC offload handler for representor devices, this
is in addition to the bpf based offload handler. The changes in this
patch will be used in a follow-up patch to add tc flower offload to
the NFP.

The flower app enables tc offloads on representors by default.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/Makefile        |   3 +-
 drivers/net/ethernet/netronome/nfp/flower/main.c   |  20 ++++
 drivers/net/ethernet/netronome/nfp/flower/main.h   |  45 ++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    | 127 +++++++++++++++++++++
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |  14 +--
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c  |   6 +
 drivers/net/ethernet/netronome/nfp/nfp_port.c      |  15 +++
 drivers/net/ethernet/netronome/nfp/nfp_port.h      |   5 +
 8 files changed, 221 insertions(+), 14 deletions(-)
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/main.h
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/offload.c

diff --git a/drivers/net/ethernet/netronome/nfp/Makefile b/drivers/net/ethernet/netronome/nfp/Makefile
index 43bdbc228969..d7afd2b410fe 100644
--- a/drivers/net/ethernet/netronome/nfp/Makefile
+++ b/drivers/net/ethernet/netronome/nfp/Makefile
@@ -32,7 +32,8 @@ nfp-objs := \
 ifeq ($(CONFIG_NFP_APP_FLOWER),y)
 nfp-objs += \
 	    flower/cmsg.o \
-	    flower/main.o
+	    flower/main.o \
+	    flower/offload.o
 endif
 
 ifeq ($(CONFIG_BPF_SYSCALL),y)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index ab68a8f58862..19f20f819e2f 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -37,7 +37,9 @@
 #include <net/devlink.h>
 #include <net/dst_metadata.h>
 
+#include "main.h"
 #include "../nfpcore/nfp_cpp.h"
+#include "../nfpcore/nfp_nffw.h"
 #include "../nfpcore/nfp_nsp.h"
 #include "../nfp_app.h"
 #include "../nfp_main.h"
@@ -46,6 +48,8 @@
 #include "../nfp_port.h"
 #include "./cmsg.h"
 
+#define NFP_FLOWER_ALLOWED_VER 0x0001000000010000UL
+
 /**
  * struct nfp_flower_priv - Flower APP per-vNIC priv data
  * @nn:		     Pointer to vNIC
@@ -313,6 +317,8 @@ static int nfp_flower_vnic_init(struct nfp_app *app, struct nfp_net *nn,
 static int nfp_flower_init(struct nfp_app *app)
 {
 	const struct nfp_pf *pf = app->pf;
+	u64 version;
+	int err;
 
 	if (!pf->eth_tbl) {
 		nfp_warn(app->cpp, "FlowerNIC requires eth table\n");
@@ -329,6 +335,18 @@ static int nfp_flower_init(struct nfp_app *app)
 		return -EINVAL;
 	}
 
+	version = nfp_rtsym_read_le(app->pf->rtbl, "hw_flower_version", &err);
+	if (err) {
+		nfp_warn(app->cpp, "FlowerNIC requires hw_flower_version memory symbol\n");
+		return err;
+	}
+
+	/* We need to ensure hardware has enough flower capabilities. */
+	if (version != NFP_FLOWER_ALLOWED_VER) {
+		nfp_warn(app->cpp, "FlowerNIC: unspported firmware version\n");
+		return -EINVAL;
+	}
+
 	app->priv = kzalloc(sizeof(struct nfp_flower_priv), GFP_KERNEL);
 	if (!app->priv)
 		return -ENOMEM;
@@ -367,4 +385,6 @@ const struct nfp_app_type app_flower = {
 
 	.eswitch_mode_get  = eswitch_mode_get,
 	.repr_get	= nfp_flower_repr_get,
+
+	.setup_tc	= nfp_flower_setup_tc,
 };
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
new file mode 100644
index 000000000000..c7a19527875e
--- /dev/null
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2017 Netronome Systems, Inc.
+ *
+ * This software is dual licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree or the BSD 2-Clause License provided below.  You have the
+ * option to license this software under the complete terms of either license.
+ *
+ * The BSD 2-Clause License:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      1. Redistributions of source code must retain the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer.
+ *
+ *      2. Redistributions in binary form must reproduce the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer in the documentation and/or other materials
+ *         provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef __NFP_FLOWER_H__
+#define __NFP_FLOWER_H__ 1
+
+#include <linux/types.h>
+
+struct tc_to_netdev;
+struct net_device;
+struct nfp_app;
+
+int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
+			u32 handle, __be16 proto, struct tc_to_netdev *tc);
+#endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
new file mode 100644
index 000000000000..794e1cbf3f7f
--- /dev/null
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -0,0 +1,127 @@
+/*
+ * Copyright (C) 2017 Netronome Systems, Inc.
+ *
+ * This software is dual licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree or the BSD 2-Clause License provided below.  You have the
+ * option to license this software under the complete terms of either license.
+ *
+ * The BSD 2-Clause License:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      1. Redistributions of source code must retain the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer.
+ *
+ *      2. Redistributions in binary form must reproduce the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer in the documentation and/or other materials
+ *         provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/skbuff.h>
+#include <net/devlink.h>
+#include <net/pkt_cls.h>
+
+#include "main.h"
+#include "cmsg.h"
+#include "../nfpcore/nfp_cpp.h"
+#include "../nfpcore/nfp_nsp.h"
+#include "../nfp_app.h"
+#include "../nfp_main.h"
+#include "../nfp_net.h"
+#include "../nfp_port.h"
+
+/**
+ * nfp_flower_add_offload() - Adds a new flow to hardware.
+ * @app:	Pointer to the APP handle
+ * @netdev:	netdev structure.
+ * @flow:	TC flower classifier offload structure.
+ *
+ * Adds a new flow to the repeated hash structure and action payload.
+ *
+ * Return: negative value on error, 0 if configured successfully.
+ */
+static int
+nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
+		       struct tc_cls_flower_offload *flow)
+{
+	return -EOPNOTSUPP;
+}
+
+/**
+ * nfp_flower_del_offload() - Removes a flow from hardware.
+ * @app:	Pointer to the APP handle
+ * @netdev:	netdev structure.
+ * @flow:       TC flower classifier offload structure
+ *
+ * Removes a flow from the repeated hash structure and clears the
+ * action payload.
+ *
+ * Return: negative value on error, 0 if removed successfully.
+ */
+static int
+nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
+		       struct tc_cls_flower_offload *flow)
+{
+	return -EOPNOTSUPP;
+}
+
+/**
+ * nfp_flower_get_stats() - Populates flow stats obatain from hardware.
+ * @app:	Pointer to the APP handle
+ * @flow:       TC flower classifier offload structure
+ *
+ * Populates a flow statistics structure which which corresponds to a
+ * specific flow.
+ *
+ * Return: negative value on error, 0 if stats populated successfully.
+ */
+static int
+nfp_flower_get_stats(struct nfp_app *app, struct tc_cls_flower_offload *flow)
+{
+	return -EOPNOTSUPP;
+}
+
+static int
+nfp_flower_repr_offload(struct nfp_app *app, struct net_device *netdev,
+			struct tc_cls_flower_offload *flower)
+{
+	switch (flower->command) {
+	case TC_CLSFLOWER_REPLACE:
+		return nfp_flower_add_offload(app, netdev, flower);
+	case TC_CLSFLOWER_DESTROY:
+		return nfp_flower_del_offload(app, netdev, flower);
+	case TC_CLSFLOWER_STATS:
+		return nfp_flower_get_stats(app, flower);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
+			u32 handle, __be16 proto, struct tc_to_netdev *tc)
+{
+	if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
+		return -EOPNOTSUPP;
+
+	if (!eth_proto_is_802_3(proto))
+		return -EOPNOTSUPP;
+
+	if (tc->type != TC_SETUP_CLSFLOWER)
+		return -EINVAL;
+
+	return nfp_flower_repr_offload(app, netdev, tc->cls_flower);
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index b5834525c5f0..30f82b41d400 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3097,18 +3097,6 @@ static void nfp_net_stat64(struct net_device *netdev,
 	}
 }
 
-static int
-nfp_net_setup_tc(struct net_device *netdev, u32 handle, u32 chain_index,
-		 __be16 proto, struct tc_to_netdev *tc)
-{
-	struct nfp_net *nn = netdev_priv(netdev);
-
-	if (chain_index)
-		return -EOPNOTSUPP;
-
-	return nfp_app_setup_tc(nn->app, netdev, handle, proto, tc);
-}
-
 static int nfp_net_set_features(struct net_device *netdev,
 				netdev_features_t features)
 {
@@ -3424,7 +3412,7 @@ const struct net_device_ops nfp_net_netdev_ops = {
 	.ndo_get_stats64	= nfp_net_stat64,
 	.ndo_vlan_rx_add_vid	= nfp_net_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= nfp_net_vlan_rx_kill_vid,
-	.ndo_setup_tc		= nfp_net_setup_tc,
+	.ndo_setup_tc		= nfp_port_setup_tc,
 	.ndo_tx_timeout		= nfp_net_tx_timeout,
 	.ndo_set_rx_mode	= nfp_net_set_rx_mode,
 	.ndo_change_mtu		= nfp_net_change_mtu,
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index bc9108071e5b..8ec5474f4b18 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -258,6 +258,7 @@ const struct net_device_ops nfp_repr_netdev_ops = {
 	.ndo_has_offload_stats	= nfp_repr_has_offload_stats,
 	.ndo_get_offload_stats	= nfp_repr_get_offload_stats,
 	.ndo_get_phys_port_name	= nfp_port_get_phys_port_name,
+	.ndo_setup_tc		= nfp_port_setup_tc,
 };
 
 static void nfp_repr_clean(struct nfp_repr *repr)
@@ -302,6 +303,11 @@ int nfp_repr_init(struct nfp_app *app, struct net_device *netdev,
 	netdev->netdev_ops = &nfp_repr_netdev_ops;
 	SWITCHDEV_SET_OPS(netdev, &nfp_port_switchdev_ops);
 
+	if (nfp_app_has_tc(app)) {
+		netdev->features |= NETIF_F_HW_TC;
+		netdev->hw_features |= NETIF_F_HW_TC;
+	}
+
 	err = register_netdev(netdev);
 	if (err)
 		goto err_clean;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c
index c95215eb87c2..720a263a0a36 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
@@ -87,6 +87,21 @@ const struct switchdev_ops nfp_port_switchdev_ops = {
 	.switchdev_port_attr_get	= nfp_port_attr_get,
 };
 
+int nfp_port_setup_tc(struct net_device *netdev, u32 handle, u32 chain_index,
+		      __be16 proto, struct tc_to_netdev *tc)
+{
+	struct nfp_port *port;
+
+	if (chain_index)
+		return -EOPNOTSUPP;
+
+	port = nfp_port_from_netdev(netdev);
+	if (!port)
+		return -EOPNOTSUPP;
+
+	return nfp_app_setup_tc(port->app, netdev, handle, proto, tc);
+}
+
 struct nfp_port *
 nfp_port_from_id(struct nfp_pf *pf, enum nfp_port_type type, unsigned int id)
 {
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.h b/drivers/net/ethernet/netronome/nfp/nfp_port.h
index de60cacd3362..f3552da3c277 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.h
@@ -37,6 +37,7 @@
 #include <net/devlink.h>
 #include <net/switchdev.h>
 
+struct tc_to_netdev;
 struct net_device;
 struct nfp_app;
 struct nfp_pf;
@@ -109,6 +110,10 @@ struct nfp_port {
 
 extern const struct switchdev_ops nfp_port_switchdev_ops;
 
+int
+nfp_port_setup_tc(struct net_device *netdev, u32 handle, u32 chain_index,
+		  __be16 proto, struct tc_to_netdev *tc);
+
 struct nfp_port *nfp_port_from_netdev(struct net_device *netdev);
 struct nfp_port *
 nfp_port_from_id(struct nfp_pf *pf, enum nfp_port_type type, unsigned int id);
-- 
2.1.4

Re: [PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Jakub Kicinski]

On Wed, 28 Jun 2017 22:29:56 +0200, Simon Horman wrote:
> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> 
> Adds a flower based TC offload handler for representor devices, this
> is in addition to the bpf based offload handler. The changes in this
> patch will be used in a follow-up patch to add tc flower offload to
> the NFP.
> 
> The flower app enables tc offloads on representors by default.
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

Thanks, two nits, since it seems like there will have to be another
respin.

> @@ -313,6 +317,8 @@ static int nfp_flower_vnic_init(struct nfp_app *app, struct nfp_net *nn,
>  static int nfp_flower_init(struct nfp_app *app)
>  {
>  	const struct nfp_pf *pf = app->pf;
> +	u64 version;
> +	int err;
>  
>  	if (!pf->eth_tbl) {
>  		nfp_warn(app->cpp, "FlowerNIC requires eth table\n");
> @@ -329,6 +335,18 @@ static int nfp_flower_init(struct nfp_app *app)
>  		return -EINVAL;
>  	}
>  
> +	version = nfp_rtsym_read_le(app->pf->rtbl, "hw_flower_version", &err);
> +	if (err) {
> +		nfp_warn(app->cpp, "FlowerNIC requires hw_flower_version memory symbol\n");
> +		return err;
> +	}
> +
> +	/* We need to ensure hardware has enough flower capabilities. */
> +	if (version != NFP_FLOWER_ALLOWED_VER) {
> +		nfp_warn(app->cpp, "FlowerNIC: unspported firmware version\n");

s/unspported/unsupported/

> +		return -EINVAL;
> +	}
> +
>  	app->priv = kzalloc(sizeof(struct nfp_flower_priv), GFP_KERNEL);
>  	if (!app->priv)
>  		return -ENOMEM;

> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.h b/drivers/net/ethernet/netronome/nfp/nfp_port.h
> index de60cacd3362..f3552da3c277 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_port.h
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_port.h
> @@ -37,6 +37,7 @@
>  #include <net/devlink.h>
>  #include <net/switchdev.h>
>  
> +struct tc_to_netdev;
>  struct net_device;
>  struct nfp_app;
>  struct nfp_pf;
> @@ -109,6 +110,10 @@ struct nfp_port {
>  
>  extern const struct switchdev_ops nfp_port_switchdev_ops;
>  
> +int
> +nfp_port_setup_tc(struct net_device *netdev, u32 handle, u32 chain_index,

int can be on the same line

Re: [PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Simon Horman]

On Wed, Jun 28, 2017 at 06:35:07PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2017 22:29:56 +0200, Simon Horman wrote:
> > From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > 
> > Adds a flower based TC offload handler for representor devices, this
> > is in addition to the bpf based offload handler. The changes in this
> > patch will be used in a follow-up patch to add tc flower offload to
> > the NFP.
> > 
> > The flower app enables tc offloads on representors by default.
> > 
> > Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> Thanks, two nits, since it seems like there will have to be another
> respin.

Thanks, I will fix those in v3.

Re: [PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Jakub Kicinski]

On Wed, 28 Jun 2017 22:29:56 +0200, Simon Horman wrote:
> +/**
> + * nfp_flower_del_offload() - Removes a flow from hardware.
> + * @app:	Pointer to the APP handle
> + * @netdev:	netdev structure.
> + * @flow:       TC flower classifier offload structure

Nit: there are spaces and tabs mixed here, same for
nfp_flower_get_stats().

> + *
> + * Removes a flow from the repeated hash structure and clears the
> + * action payload.
> + *
> + * Return: negative value on error, 0 if removed successfully.
> + */
> +static int
> +nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
> +		       struct tc_cls_flower_offload *flow)
> +{
> +	return -EOPNOTSUPP;
> +}

Re: [PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Simon Horman]

On Wed, Jun 28, 2017 at 06:53:40PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2017 22:29:56 +0200, Simon Horman wrote:
> > +/**
> > + * nfp_flower_del_offload() - Removes a flow from hardware.
> > + * @app:	Pointer to the APP handle
> > + * @netdev:	netdev structure.
> > + * @flow:       TC flower classifier offload structure
> 
> Nit: there are spaces and tabs mixed here, same for
> nfp_flower_get_stats().

Thanks for noticing that, we'll fix it.

> > + *
> > + * Removes a flow from the repeated hash structure and clears the
> > + * action payload.
> > + *
> > + * Return: negative value on error, 0 if removed successfully.
> > + */
> > +static int
> > +nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
> > +		       struct tc_cls_flower_offload *flow)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 

Re: [PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:29 PM, Simon Horman
<simon.horman@netronome.com> wrote:

> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c

> + * nfp_flower_del_offload() - Removes a flow from hardware.
> + * @app:       Pointer to the APP handle
> + * @netdev:    netdev structure.
> + * @flow:       TC flower classifier offload structure

The copy from patchworks has some damage on this line

 * @flow: ֲ| ֲ| ֲ| ֲ| ֲ| ֲ| TC flower classifier offload structure

and the same line for get stats, take a look

> + * nfp_flower_get_stats() - Populates flow stats obatain from hardware.
> + * @app:       Pointer to the APP handle
> + * @flow:       TC flower classifier offload structure <-- here too

Re: [PATCH net-next v2 3/9] nfp: provide infrastructure for offloading flower based TC filters

[Simon Horman]

On Thu, Jun 29, 2017 at 04:56:44PM +0300, Or Gerlitz wrote:
> On Wed, Jun 28, 2017 at 11:29 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> 
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> 
> > + * nfp_flower_del_offload() - Removes a flow from hardware.
> > + * @app:       Pointer to the APP handle
> > + * @netdev:    netdev structure.
> > + * @flow:       TC flower classifier offload structure
> 
> The copy from patchworks has some damage on this line
> 
>  * @flow: ֲ| ֲ| ֲ| ֲ| ֲ| ֲ| TC flower classifier offload structure
> 
> and the same line for get stats, take a look

Thanks, I will fix that whitespace damage.

> 
> > + * nfp_flower_get_stats() - Populates flow stats obatain from hardware.
> > + * @app:       Pointer to the APP handle
> > + * @flow:       TC flower classifier offload structure <-- here too

[PATCH net-next v2 4/9] nfp: extend flower add flow offload

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Extends the flower flow add function by calculating which match
fields are present in the flower offload structure and allocating
the appropriate space to describe these.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/cmsg.h   | 141 +++++++++++++++++
 drivers/net/ethernet/netronome/nfp/flower/main.h   |  24 +++
 .../net/ethernet/netronome/nfp/flower/offload.c    | 166 ++++++++++++++++++++-
 3 files changed, 330 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index c10ae7631941..1b1888e8dc14 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -40,6 +40,147 @@
 
 #include "../nfp_app.h"
 
+#define NFP_FLOWER_LAYER_META		BIT(0)
+#define NFP_FLOWER_LAYER_PORT		BIT(1)
+#define NFP_FLOWER_LAYER_MAC		BIT(2)
+#define NFP_FLOWER_LAYER_TP		BIT(3)
+#define NFP_FLOWER_LAYER_IPV4		BIT(4)
+#define NFP_FLOWER_LAYER_IPV6		BIT(5)
+#define NFP_FLOWER_LAYER_CT		BIT(6)
+#define NFP_FLOWER_LAYER_VXLAN		BIT(7)
+
+#define NFP_FLOWER_LAYER_ETHER		BIT(3)
+#define NFP_FLOWER_LAYER_ARP		BIT(4)
+
+/* Metadata without L2 (1W/4B)
+ * ----------------------------------------------------------------
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |  key_layers   |    mask_id    |           reserved            |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_meta_one {
+	u8 nfp_flow_key_layer;
+	u8 mask_id;
+	u16 reserved;
+};
+
+/* Metadata with L2 (1W/4B)
+ * ----------------------------------------------------------------
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |    key_type   |    mask_id    | PCP |p|   vlan outermost VID  |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *                                 ^                               ^
+ *                           NOTE: |             TCI               |
+ *                                 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_meta_two {
+	u8 nfp_flow_key_layer;
+	u8 mask_id;
+	__be16 tci;
+};
+
+/* Port details (1W/4B)
+ * ----------------------------------------------------------------
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                         port_ingress                          |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_in_port {
+	__be32 in_port;
+};
+
+/* L2 details (4W/16B)
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                     mac_addr_dst, 31 - 0                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |      mac_addr_dst, 47 - 32    |     mac_addr_src, 15 - 0      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                     mac_addr_src, 47 - 16                     |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |       mpls outermost label            |  TC |B|   reserved  |q|
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_mac_mpls {
+	u8 mac_dst[6];
+	u8 mac_src[6];
+	__be32 mpls_lse;
+};
+
+/* L4 ports (for UDP, TCP, SCTP) (1W/4B)
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |            port_src           |           port_dst            |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_tp_ports {
+	__be16 port_src;
+	__be16 port_dst;
+};
+
+/* L3 IPv4 details (3W/12B)
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |    DSCP   |ECN|   protocol    |           reserved            |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                        ipv4_addr_src                          |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                        ipv4_addr_dst                          |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_ipv4 {
+	u8 tos;
+	u8 proto;
+	u8 ttl;
+	u8 reserved;
+	__be32 ipv4_src;
+	__be32 ipv4_dst;
+};
+
+/* L3 IPv6 details (10W/40B)
+ *    3                   2                   1
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |    DSCP   |ECN|   protocol    |          reserved             |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |   ipv6_exthdr   | res |            ipv6_flow_label            |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_src,   31 - 0                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_src,  63 - 32                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_src,  95 - 64                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_src, 127 - 96                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_dst,   31 - 0                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_dst,  63 - 32                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_dst,  95 - 64                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                  ipv6_addr_dst, 127 - 96                      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct nfp_flower_ipv6 {
+	u8 tos;
+	u8 proto;
+	u8 ttl;
+	u8 reserved;
+	__be32 ipv6_flow_label_exthdr;
+	struct in6_addr ipv6_src;
+	struct in6_addr ipv6_dst;
+};
+
 /* The base header for a control message packet.
  * Defines an 8-bit version, and an 8-bit type, padded
  * to a 32-bit word. Rest of the packet is type-specific.
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index c7a19527875e..ba3e14c3ec26 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -40,6 +40,30 @@ struct tc_to_netdev;
 struct net_device;
 struct nfp_app;
 
+struct nfp_fl_key_ls {
+	u32 key_layer_two;
+	u8 key_layer;
+	int key_size;
+};
+
+struct nfp_fl_rule_metadata {
+	u8 key_len;
+	u8 mask_len;
+	u8 act_len;
+	u8 flags;
+	__be32 host_ctx_id;
+	__be64 host_cookie __packed;
+	__be64 flow_version __packed;
+	__be32 shortcut;
+};
+
+struct nfp_fl_payload {
+	struct nfp_fl_rule_metadata meta;
+	char *unmasked_data;
+	char *mask_data;
+	char *action_data;
+};
+
 int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
 			u32 handle, __be16 proto, struct tc_to_netdev *tc);
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 794e1cbf3f7f..c15d606d2e54 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -44,6 +44,145 @@
 #include "../nfp_net.h"
 #include "../nfp_port.h"
 
+static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
+{
+	return dissector_uses_key(f->dissector,
+				  FLOW_DISSECTOR_KEY_IPV4_ADDRS) ||
+		dissector_uses_key(f->dissector,
+				   FLOW_DISSECTOR_KEY_IPV6_ADDRS) ||
+		dissector_uses_key(f->dissector,
+				   FLOW_DISSECTOR_KEY_PORTS) ||
+		dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ICMP);
+}
+
+static int
+nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
+				struct tc_cls_flower_offload *flow)
+{
+	struct flow_dissector_key_control *mask_enc_ctl;
+	struct flow_dissector_key_basic *mask_basic;
+	struct flow_dissector_key_basic *key_basic;
+	u32 key_layer_two;
+	u8 key_layer;
+	int key_size;
+
+	mask_enc_ctl = skb_flow_dissector_target(flow->dissector,
+						 FLOW_DISSECTOR_KEY_ENC_CONTROL,
+						 flow->mask);
+
+	mask_basic = skb_flow_dissector_target(flow->dissector,
+					       FLOW_DISSECTOR_KEY_BASIC,
+					       flow->mask);
+
+	key_basic = skb_flow_dissector_target(flow->dissector,
+					      FLOW_DISSECTOR_KEY_BASIC,
+					      flow->key);
+	key_layer_two = 0;
+	key_layer = NFP_FLOWER_LAYER_PORT | NFP_FLOWER_LAYER_MAC;
+	key_size = sizeof(struct nfp_flower_meta_one) +
+		   sizeof(struct nfp_flower_in_port) +
+		   sizeof(struct nfp_flower_mac_mpls);
+
+	/* We are expecting a tunnel. For now we ignore offloading. */
+	if (mask_enc_ctl->addr_type)
+		return -EOPNOTSUPP;
+
+	if (mask_basic->n_proto) {
+		/* Ethernet type is present in the key. */
+		switch (key_basic->n_proto) {
+		case cpu_to_be16(ETH_P_IP):
+			key_layer |= NFP_FLOWER_LAYER_IPV4;
+			key_size += sizeof(struct nfp_flower_ipv4);
+			break;
+
+		case cpu_to_be16(ETH_P_IPV6):
+			key_layer |= NFP_FLOWER_LAYER_IPV6;
+			key_size += sizeof(struct nfp_flower_ipv6);
+			break;
+
+		/* Currently we do not offload ARP
+		 * because we rely on it to get to the host.
+		 */
+		case cpu_to_be16(ETH_P_ARP):
+			return -EOPNOTSUPP;
+
+		/* Will be included in layer 2. */
+		case cpu_to_be16(ETH_P_8021Q):
+			break;
+
+		default:
+			/* Other ethtype - we need check the masks for the
+			 * remainer of the key to ensure we can offload.
+			 */
+			if (nfp_flower_check_higher_than_mac(flow))
+				return -EOPNOTSUPP;
+			break;
+		}
+	}
+
+	if (mask_basic->ip_proto) {
+		/* Ethernet type is present in the key. */
+		switch (key_basic->ip_proto) {
+		case IPPROTO_TCP:
+		case IPPROTO_UDP:
+		case IPPROTO_SCTP:
+		case IPPROTO_ICMP:
+		case IPPROTO_ICMPV6:
+			key_layer |= NFP_FLOWER_LAYER_TP;
+			key_size += sizeof(struct nfp_flower_tp_ports);
+			break;
+		default:
+			/* Other ip proto - we need check the masks for the
+			 * remainer of the key to ensure we can offload.
+			 */
+			return -EOPNOTSUPP;
+		}
+	}
+
+	ret_key_ls->key_layer = key_layer;
+	ret_key_ls->key_layer_two = key_layer_two;
+	ret_key_ls->key_size = key_size;
+
+	return 0;
+}
+
+static struct nfp_fl_payload *
+nfp_flower_allocate_new(struct nfp_fl_key_ls *key_layer)
+{
+	struct nfp_fl_payload *flow_pay;
+
+	flow_pay = kmalloc(sizeof(*flow_pay), GFP_KERNEL);
+	if (!flow_pay)
+		return NULL;
+
+	flow_pay->meta.key_len = key_layer->key_size;
+	flow_pay->unmasked_data = kmalloc(key_layer->key_size, GFP_KERNEL);
+	if (!flow_pay->unmasked_data)
+		goto err_free_flow;
+
+	flow_pay->meta.mask_len = key_layer->key_size;
+	flow_pay->mask_data = kmalloc(key_layer->key_size, GFP_KERNEL);
+	if (!flow_pay->mask_data)
+		goto err_free_unmasked;
+
+	flow_pay->meta.flags = 0;
+
+	return flow_pay;
+
+err_free_unmasked:
+	kfree(flow_pay->unmasked_data);
+err_free_flow:
+	kfree(flow_pay);
+	return NULL;
+}
+
+static void nfp_flower_deallocate_nfp(struct nfp_fl_payload *flow_pay)
+{
+	kfree(flow_pay->mask_data);
+	kfree(flow_pay->unmasked_data);
+	kfree(flow_pay);
+}
+
 /**
  * nfp_flower_add_offload() - Adds a new flow to hardware.
  * @app:	Pointer to the APP handle
@@ -58,7 +197,32 @@ static int
 nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 		       struct tc_cls_flower_offload *flow)
 {
-	return -EOPNOTSUPP;
+	struct nfp_fl_payload *flow_pay;
+	struct nfp_fl_key_ls *key_layer;
+	int err;
+
+	key_layer = kmalloc(sizeof(*key_layer), GFP_KERNEL);
+	if (!key_layer)
+		return -ENOMEM;
+
+	err = nfp_flower_calculate_key_layers(key_layer, flow);
+	if (err)
+		goto err_free_key_ls;
+
+	flow_pay = nfp_flower_allocate_new(key_layer);
+	if (!flow_pay) {
+		err = -ENOMEM;
+		goto err_free_key_ls;
+	}
+
+	/* TODO: Complete flower_add_offload. */
+	err = -EOPNOTSUPP;
+
+	nfp_flower_deallocate_nfp(flow_pay);
+
+err_free_key_ls:
+	kfree(key_layer);
+	return err;
 }
 
 /**
-- 
2.1.4

Re: [PATCH net-next v2 4/9] nfp: extend flower add flow offload

[Yunsheng Lin]

> +
> +	if (mask_basic->n_proto) {
cpu_to_be16（mask_basic->n_proto）
remove cpu_to_be16 in case.
> +		/* Ethernet type is present in the key. */
> +		switch (key_basic->n_proto) {
> +		case cpu_to_be16(ETH_P_IP):
> +			key_layer |= NFP_FLOWER_LAYER_IPV4;
> +			key_size += sizeof(struct nfp_flower_ipv4);
> +			break;
> +
> +		case cpu_to_be16(ETH_P_IPV6):
> +			key_layer |= NFP_FLOWER_LAYER_IPV6;
> +			key_size += sizeof(struct nfp_flower_ipv6);
> +			break;
> +
> +		/* Currently we do not offload ARP
> +		 * because we rely on it to get to the host.
> +		 */
> +		case cpu_to_be16(ETH_P_ARP):
> +			return -EOPNOTSUPP;
> +
> +		/* Will be included in layer 2. */
> +		case cpu_to_be16(ETH_P_8021Q):
> +			break;
> +
> +		default:
> +			/* Other ethtype - we need check the masks for the
> +			 * remainer of the key to ensure we can offload.
> +			 */
> +			if (nfp_flower_check_higher_than_mac(flow))
> +				return -EOPNOTSUPP;
> +			break;
> +		}
> +	}
> +
> +	if (mask_basic->ip_proto) {
> +		/* Ethernet type is present in the key. */
> +		switch (key_basic->ip_proto) {
> +		case IPPROTO_TCP:
> +		case IPPROTO_UDP:
> +		case IPPROTO_SCTP:
> +		case IPPROTO_ICMP:
> +		case IPPROTO_ICMPV6:
> +			key_layer |= NFP_FLOWER_LAYER_TP;
> +			key_size += sizeof(struct nfp_flower_tp_ports);
> +			break;
> +		default:
> +			/* Other ip proto - we need check the masks for the
> +			 * remainer of the key to ensure we can offload.
> +			 */
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	ret_key_ls->key_layer = key_layer;
> +	ret_key_ls->key_layer_two = key_layer_two;
> +	ret_key_ls->key_size = key_size;
> +
> +	return 0;
> +}
> +
> +static struct nfp_fl_payload *
> +nfp_flower_allocate_new(struct nfp_fl_key_ls *key_layer)
> +{
> +	struct nfp_fl_payload *flow_pay;
> +
> +	flow_pay = kmalloc(sizeof(*flow_pay), GFP_KERNEL);
> +	if (!flow_pay)
> +		return NULL;
> +
> +	flow_pay->meta.key_len = key_layer->key_size;
> +	flow_pay->unmasked_data = kmalloc(key_layer->key_size, GFP_KERNEL);
> +	if (!flow_pay->unmasked_data)
> +		goto err_free_flow;
> +
> +	flow_pay->meta.mask_len = key_layer->key_size;
> +	flow_pay->mask_data = kmalloc(key_layer->key_size, GFP_KERNEL);
> +	if (!flow_pay->mask_data)
> +		goto err_free_unmasked;
> +
> +	flow_pay->meta.flags = 0;
> +
> +	return flow_pay;
> +
> +err_free_unmasked:
> +	kfree(flow_pay->unmasked_data);
> +err_free_flow:
> +	kfree(flow_pay);
> +	return NULL;
> +}
> +
> +static void nfp_flower_deallocate_nfp(struct nfp_fl_payload *flow_pay)
> +{
> +	kfree(flow_pay->mask_data);
> +	kfree(flow_pay->unmasked_data);
> +	kfree(flow_pay);
> +}
> +
>  /**
>   * nfp_flower_add_offload() - Adds a new flow to hardware.
>   * @app:	Pointer to the APP handle
> @@ -58,7 +197,32 @@ static int
>  nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
>  		       struct tc_cls_flower_offload *flow)
>  {
> -	return -EOPNOTSUPP;
> +	struct nfp_fl_payload *flow_pay;
> +	struct nfp_fl_key_ls *key_layer;
> +	int err;
> +
> +	key_layer = kmalloc(sizeof(*key_layer), GFP_KERNEL);
> +	if (!key_layer)
> +		return -ENOMEM;
> +
> +	err = nfp_flower_calculate_key_layers(key_layer, flow);
> +	if (err)
> +		goto err_free_key_ls;
> +
> +	flow_pay = nfp_flower_allocate_new(key_layer);
> +	if (!flow_pay) {
> +		err = -ENOMEM;
> +		goto err_free_key_ls;
> +	}
> +
> +	/* TODO: Complete flower_add_offload. */
> +	err = -EOPNOTSUPP;
> +
> +	nfp_flower_deallocate_nfp(flow_pay);
> +
> +err_free_key_ls:
> +	kfree(key_layer);
> +	return err;
>  }
>  
>  /**
> 

Re: [PATCH net-next v2 4/9] nfp: extend flower add flow offload

[Jakub Kicinski]

On Thu, 29 Jun 2017 14:18:07 +0800, Yunsheng Lin wrote:
> > +	if (mask_basic->n_proto) {  
> cpu_to_be16（mask_basic->n_proto）
> remove cpu_to_be16 in case.

Thanks, but this is incorrect.  Byte swapping constants is done at
compilation time - therefore it's preferred.

RE: [PATCH net-next v2 4/9] nfp: extend flower add flow offload

[David Laight]

From: Jakub Kicinski
> Sent: 29 June 2017 07:48
> On Thu, 29 Jun 2017 14:18:07 +0800, Yunsheng Lin wrote:
> > > +	if (mask_basic->n_proto) {
> > cpu_to_be16(mask_basic->n_proto)

Should be be16_to_cpu()

> > remove cpu_to_be16 in case.
> 
> Thanks, but this is incorrect.  Byte swapping constants is done at
> compilation time - therefore it's preferred.

Except that the 'cpu' values are likely to be dense so the compiler
is likely to generate a jump table for the switch statement instead
of sequence of conditionals.

OTOH the jump table is almost certainly a data cache miss, whereas
the conditionals might be predicted correctly.

The best code might come from an explicitly ordered sequence of
conditionals.

	David

[PATCH net-next v2 5/9] nfp: extend flower matching capabilities

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Extends matching capabilities for flower offloads to include vlan,
layer 2, layer 3 and layer 4 type matches. This includes both exact
and wildcard matching.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/Makefile        |   1 +
 drivers/net/ethernet/netronome/nfp/flower/cmsg.h   |   4 +
 drivers/net/ethernet/netronome/nfp/flower/main.h   |   5 +
 drivers/net/ethernet/netronome/nfp/flower/match.c  | 292 +++++++++++++++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    |   5 +
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.h  |   9 +
 6 files changed, 316 insertions(+)
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/match.c

diff --git a/drivers/net/ethernet/netronome/nfp/Makefile b/drivers/net/ethernet/netronome/nfp/Makefile
index d7afd2b410fe..018cef3fa10a 100644
--- a/drivers/net/ethernet/netronome/nfp/Makefile
+++ b/drivers/net/ethernet/netronome/nfp/Makefile
@@ -33,6 +33,7 @@ ifeq ($(CONFIG_NFP_APP_FLOWER),y)
 nfp-objs += \
 	    flower/cmsg.o \
 	    flower/main.o \
+	    flower/match.o \
 	    flower/offload.o
 endif
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 1b1888e8dc14..1956c1acf39f 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -52,6 +52,10 @@
 #define NFP_FLOWER_LAYER_ETHER		BIT(3)
 #define NFP_FLOWER_LAYER_ARP		BIT(4)
 
+#define NFP_FLOWER_MASK_VLAN_PRIO	GENMASK(15, 13)
+#define NFP_FLOWER_MASK_VLAN_CFI	BIT(12)
+#define NFP_FLOWER_MASK_VLAN_VID	GENMASK(11, 0)
+
 /* Metadata without L2 (1W/4B)
  * ----------------------------------------------------------------
  *    3                   2                   1
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index ba3e14c3ec26..5ba7e5194708 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -66,4 +66,9 @@ struct nfp_fl_payload {
 
 int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
 			u32 handle, __be16 proto, struct tc_to_netdev *tc);
+int nfp_flower_compile_flow_match(struct tc_cls_flower_offload *flow,
+				  struct nfp_fl_key_ls *key_ls,
+				  struct net_device *netdev,
+				  struct nfp_fl_payload *nfp_flow);
+
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c b/drivers/net/ethernet/netronome/nfp/flower/match.c
new file mode 100644
index 000000000000..b700daf300e0
--- /dev/null
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -0,0 +1,292 @@
+/*
+ * Copyright (C) 2017 Netronome Systems, Inc.
+ *
+ * This software is dual licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree or the BSD 2-Clause License provided below.  You have the
+ * option to license this software under the complete terms of either license.
+ *
+ * The BSD 2-Clause License:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      1. Redistributions of source code must retain the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer.
+ *
+ *      2. Redistributions in binary form must reproduce the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer in the documentation and/or other materials
+ *         provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/bitfield.h>
+#include <net/pkt_cls.h>
+
+#include "cmsg.h"
+#include "main.h"
+
+static void
+nfp_flower_compile_meta_tci(struct nfp_flower_meta_two *frame,
+			    struct tc_cls_flower_offload *flow, u8 key_type,
+			    bool mask_version)
+{
+	struct flow_dissector_key_vlan *flow_vlan;
+	u16 tmp_tci;
+
+	/* Populate the metadata frame. */
+	frame->nfp_flow_key_layer = key_type;
+	frame->mask_id = ~0;
+
+	if (mask_version) {
+		frame->tci = cpu_to_be16(~0);
+		return;
+	}
+
+	flow_vlan = skb_flow_dissector_target(flow->dissector,
+					      FLOW_DISSECTOR_KEY_VLAN,
+					      flow->key);
+
+	/* Populate the tci field. */
+	if (!flow_vlan->vlan_id) {
+		tmp_tci = 0;
+	} else {
+		tmp_tci = FIELD_PREP(NFP_FLOWER_MASK_VLAN_PRIO,
+				     flow_vlan->vlan_priority) |
+			  FIELD_PREP(NFP_FLOWER_MASK_VLAN_VID,
+				     flow_vlan->vlan_id) |
+			  NFP_FLOWER_MASK_VLAN_CFI;
+	}
+	frame->tci = cpu_to_be16(tmp_tci);
+}
+
+static void
+nfp_flower_compile_meta(struct nfp_flower_meta_one *frame, u8 key_type)
+{
+	frame->nfp_flow_key_layer = key_type;
+	frame->mask_id = 0;
+	frame->reserved = 0;
+}
+
+static int
+nfp_flower_compile_port(struct nfp_flower_in_port *frame, u32 cmsg_port,
+			bool mask_version)
+{
+	if (mask_version) {
+		frame->in_port = cpu_to_be32(~0);
+		return 0;
+	}
+
+	frame->in_port = cpu_to_be32(cmsg_port);
+
+	return 0;
+}
+
+static void
+nfp_flower_compile_mac(struct nfp_flower_mac_mpls *frame,
+		       struct tc_cls_flower_offload *flow,
+		       bool mask_version)
+{
+	struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
+	struct flow_dissector_key_eth_addrs *flow_mac;
+
+	flow_mac = skb_flow_dissector_target(flow->dissector,
+					     FLOW_DISSECTOR_KEY_ETH_ADDRS,
+					     target);
+
+	memset(frame, 0, sizeof(struct nfp_flower_mac_mpls));
+
+	/* Populate mac frame. */
+	ether_addr_copy(frame->mac_dst, &flow_mac->dst[0]);
+	ether_addr_copy(frame->mac_src, &flow_mac->src[0]);
+
+	if (mask_version)
+		frame->mpls_lse = cpu_to_be32(~0);
+}
+
+static void
+nfp_flower_compile_tport(struct nfp_flower_tp_ports *frame,
+			 struct tc_cls_flower_offload *flow,
+			 bool mask_version)
+{
+	struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
+	struct flow_dissector_key_ports *flow_tp;
+
+	flow_tp = skb_flow_dissector_target(flow->dissector,
+					    FLOW_DISSECTOR_KEY_PORTS,
+					    target);
+
+	frame->port_src = flow_tp->src;
+	frame->port_dst = flow_tp->dst;
+}
+
+static void
+nfp_flower_compile_ipv4(struct nfp_flower_ipv4 *frame,
+			struct tc_cls_flower_offload *flow,
+			bool mask_version)
+{
+	struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
+	struct flow_dissector_key_ipv4_addrs *flow_ipv4;
+	struct flow_dissector_key_basic *flow_basic;
+
+	flow_ipv4 = skb_flow_dissector_target(flow->dissector,
+					      FLOW_DISSECTOR_KEY_IPV4_ADDRS,
+					      target);
+
+	flow_basic = skb_flow_dissector_target(flow->dissector,
+					       FLOW_DISSECTOR_KEY_BASIC,
+					       target);
+
+	/* Populate IPv4 frame. */
+	frame->reserved = 0;
+	frame->ipv4_src = flow_ipv4->src;
+	frame->ipv4_dst = flow_ipv4->dst;
+	frame->proto = flow_basic->ip_proto;
+	/* Wildcard TOS/TTL as TC can't match them yet. */
+	frame->tos = 0;
+	frame->ttl = 0;
+}
+
+static void
+nfp_flower_compile_ipv6(struct nfp_flower_ipv6 *frame,
+			struct tc_cls_flower_offload *flow,
+			bool mask_version)
+{
+	struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
+	struct flow_dissector_key_ipv6_addrs *flow_ipv6;
+	struct flow_dissector_key_basic *flow_basic;
+
+	flow_ipv6 = skb_flow_dissector_target(flow->dissector,
+					      FLOW_DISSECTOR_KEY_IPV6_ADDRS,
+					      target);
+
+	flow_basic = skb_flow_dissector_target(flow->dissector,
+					       FLOW_DISSECTOR_KEY_BASIC,
+					       target);
+
+	/* Populate IPv6 frame. */
+	frame->reserved = 0;
+	frame->ipv6_src = flow_ipv6->src;
+	frame->ipv6_dst = flow_ipv6->dst;
+	frame->proto = flow_basic->ip_proto;
+	/* Wildcard LABEL/TOS/TTL as TC can't match them yet. */
+	frame->ipv6_flow_label_exthdr = 0;
+	frame->tos = 0;
+	frame->ttl = 0;
+}
+
+int nfp_flower_compile_flow_match(struct tc_cls_flower_offload *flow,
+				  struct nfp_fl_key_ls *key_ls,
+				  struct net_device *netdev,
+				  struct nfp_fl_payload *nfp_flow)
+{
+	int err;
+	u8 *ext;
+	u8 *msk;
+
+	memset(nfp_flow->unmasked_data, 0, key_ls->key_size);
+	memset(nfp_flow->mask_data, 0, key_ls->key_size);
+
+	ext = nfp_flow->unmasked_data;
+	msk = nfp_flow->mask_data;
+	if (NFP_FLOWER_LAYER_PORT & key_ls->key_layer) {
+		/* Populate Exact Metadata. */
+		nfp_flower_compile_meta_tci((struct nfp_flower_meta_two *)ext,
+					    flow, key_ls->key_layer, false);
+		/* Populate Mask Metadata. */
+		nfp_flower_compile_meta_tci((struct nfp_flower_meta_two *)msk,
+					    flow, key_ls->key_layer, true);
+		ext += sizeof(struct nfp_flower_meta_two);
+		msk += sizeof(struct nfp_flower_meta_two);
+
+		/* Populate Exact Port data. */
+		err = nfp_flower_compile_port((struct nfp_flower_in_port *)ext,
+					      nfp_repr_get_port_id(netdev),
+					      false);
+		if (err)
+			return err;
+
+		/* Populate Mask Port Data. */
+		err = nfp_flower_compile_port((struct nfp_flower_in_port *)msk,
+					      nfp_repr_get_port_id(netdev),
+					      true);
+		if (err)
+			return err;
+
+		ext += sizeof(struct nfp_flower_in_port);
+		msk += sizeof(struct nfp_flower_in_port);
+	} else {
+		/* Populate Exact Metadata. */
+		nfp_flower_compile_meta((struct nfp_flower_meta_one *)ext,
+					key_ls->key_layer);
+		/* Populate Mask Metadata. */
+		nfp_flower_compile_meta((struct nfp_flower_meta_one *)msk,
+					key_ls->key_layer);
+		ext += sizeof(struct nfp_flower_meta_one);
+		msk += sizeof(struct nfp_flower_meta_one);
+	}
+
+	if (NFP_FLOWER_LAYER_META & key_ls->key_layer) {
+		/* Additional Metadata Fields.
+		 * Currently unsupported.
+		 */
+		return -EOPNOTSUPP;
+	}
+
+	if (NFP_FLOWER_LAYER_MAC & key_ls->key_layer) {
+		/* Populate Exact MAC Data. */
+		nfp_flower_compile_mac((struct nfp_flower_mac_mpls *)ext,
+				       flow, false);
+		/* Populate Mask MAC Data. */
+		nfp_flower_compile_mac((struct nfp_flower_mac_mpls *)msk,
+				       flow, true);
+		ext += sizeof(struct nfp_flower_mac_mpls);
+		msk += sizeof(struct nfp_flower_mac_mpls);
+	}
+
+	if (NFP_FLOWER_LAYER_TP & key_ls->key_layer) {
+		/* Populate Exact TP Data. */
+		nfp_flower_compile_tport((struct nfp_flower_tp_ports *)ext,
+					 flow, false);
+		/* Populate Mask TP Data. */
+		nfp_flower_compile_tport((struct nfp_flower_tp_ports *)msk,
+					 flow, true);
+		ext += sizeof(struct nfp_flower_tp_ports);
+		msk += sizeof(struct nfp_flower_tp_ports);
+	}
+
+	if (NFP_FLOWER_LAYER_IPV4 & key_ls->key_layer) {
+		/* Populate Exact IPv4 Data. */
+		nfp_flower_compile_ipv4((struct nfp_flower_ipv4 *)ext,
+					flow, false);
+		/* Populate Mask IPv4 Data. */
+		nfp_flower_compile_ipv4((struct nfp_flower_ipv4 *)msk,
+					flow, true);
+		ext += sizeof(struct nfp_flower_ipv4);
+		msk += sizeof(struct nfp_flower_ipv4);
+	}
+
+	if (NFP_FLOWER_LAYER_IPV6 & key_ls->key_layer) {
+		/* Populate Exact IPv4 Data. */
+		nfp_flower_compile_ipv6((struct nfp_flower_ipv6 *)ext,
+					flow, false);
+		/* Populate Mask IPv4 Data. */
+		nfp_flower_compile_ipv6((struct nfp_flower_ipv6 *)msk,
+					flow, true);
+		ext += sizeof(struct nfp_flower_ipv6);
+		msk += sizeof(struct nfp_flower_ipv6);
+	}
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index c15d606d2e54..0d0ac2241b56 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -215,9 +215,14 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 		goto err_free_key_ls;
 	}
 
+	err = nfp_flower_compile_flow_match(flow, key_layer, netdev, flow_pay);
+	if (err)
+		goto err_destroy_flow;
+
 	/* TODO: Complete flower_add_offload. */
 	err = -EOPNOTSUPP;
 
+err_destroy_flow:
 	nfp_flower_deallocate_nfp(flow_pay);
 
 err_free_key_ls:
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.h b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.h
index 6a6727816010..32179cad062a 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.h
@@ -38,6 +38,8 @@ struct metadata_dst;
 struct nfp_net;
 struct nfp_port;
 
+#include <net/dst_metadata.h>
+
 /**
  * struct nfp_reprs - container for representor netdevs
  * @num_reprs:	Number of elements in reprs array
@@ -104,6 +106,13 @@ static inline bool nfp_netdev_is_nfp_repr(struct net_device *netdev)
 	return netdev->netdev_ops == &nfp_repr_netdev_ops;
 }
 
+static inline int nfp_repr_get_port_id(struct net_device *netdev)
+{
+	struct nfp_repr *priv = netdev_priv(netdev);
+
+	return priv->dst->u.port_info.port_id;
+}
+
 void nfp_repr_inc_rx_stats(struct net_device *netdev, unsigned int len);
 int nfp_repr_init(struct nfp_app *app, struct net_device *netdev,
 		  u32 cmsg_port_id, struct nfp_port *port,
-- 
2.1.4

Re: [PATCH net-next v2 5/9] nfp: extend flower matching capabilities

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:29 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> +       /* Populate IPv4 frame. */
> +       frame->reserved = 0;
> +       frame->ipv4_src = flow_ipv4->src;
> +       frame->ipv4_dst = flow_ipv4->dst;
> +       frame->proto = flow_basic->ip_proto;
> +       /* Wildcard TOS/TTL as TC can't match them yet. */

FWIW this is wrong... matching on both was added recently

> +       frame->tos = 0;
> +       frame->ttl = 0;

note that the same flow dissector matching is used for ipv4 ttl/tos
and ipv6 hoplimit/traffic class

Re: [PATCH net-next v2 5/9] nfp: extend flower matching capabilities

[Simon Horman]

On Thu, Jun 29, 2017 at 05:31:22PM +0300, Or Gerlitz wrote:
> On Wed, Jun 28, 2017 at 11:29 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > +       /* Populate IPv4 frame. */
> > +       frame->reserved = 0;
> > +       frame->ipv4_src = flow_ipv4->src;
> > +       frame->ipv4_dst = flow_ipv4->dst;
> > +       frame->proto = flow_basic->ip_proto;
> > +       /* Wildcard TOS/TTL as TC can't match them yet. */
> 
> FWIW this is wrong... matching on both was added recently
> 
> > +       frame->tos = 0;
> > +       frame->ttl = 0;
> 
> note that the same flow dissector matching is used for ipv4 ttl/tos
> and ipv6 hoplimit/traffic class

Thanks. I think we'll update the comment for now and make use
of the nice new feature you added in a follow-up patchset.

Re: [PATCH net-next v2 5/9] nfp: extend flower matching capabilities

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:29 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> +nfp_flower_compile_meta_tci(struct nfp_flower_meta_two *frame,
> +                           struct tc_cls_flower_offload *flow, u8 key_type,
> +                           bool mask_version)

what's the role of the mask_version flag here and elsewhere in the
sister functions?

> +{
> +       struct flow_dissector_key_vlan *flow_vlan;
> +       u16 tmp_tci;
> +
> +       /* Populate the metadata frame. */
> +       frame->nfp_flow_key_layer = key_type;
> +       frame->mask_id = ~0;
> +
> +       if (mask_version) {
> +               frame->tci = cpu_to_be16(~0);
> +               return;
> +       }
> +

Re: [PATCH net-next v2 5/9] nfp: extend flower matching capabilities

[Simon Horman]

On Thu, Jun 29, 2017 at 05:33:32PM +0300, Or Gerlitz wrote:
> On Wed, Jun 28, 2017 at 11:29 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > +nfp_flower_compile_meta_tci(struct nfp_flower_meta_two *frame,
> > +                           struct tc_cls_flower_offload *flow, u8 key_type,
> > +                           bool mask_version)
> 
> what's the role of the mask_version flag here and elsewhere in the
> sister functions?

Hi Or,

I chatted with Pieter about this and my understanding is that metadata
comes in pairs; masked and umasked. The mask_version parameter controls
which of the pair is compiled.

> 
> > +{
> > +       struct flow_dissector_key_vlan *flow_vlan;
> > +       u16 tmp_tci;
> > +
> > +       /* Populate the metadata frame. */
> > +       frame->nfp_flow_key_layer = key_type;
> > +       frame->mask_id = ~0;
> > +
> > +       if (mask_version) {
> > +               frame->tci = cpu_to_be16(~0);
> > +               return;
> > +       }
> > +

[PATCH net-next v2 6/9] nfp: add basic action capabilities to flower offloads

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Adds push vlan, pop vlan, output and drop action capabilities
to flower offloads.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/Makefile        |   1 +
 drivers/net/ethernet/netronome/nfp/flower/action.c | 211 +++++++++++++++++++++
 drivers/net/ethernet/netronome/nfp/flower/cmsg.h   |  45 +++++
 drivers/net/ethernet/netronome/nfp/flower/main.h   |   3 +
 .../net/ethernet/netronome/nfp/flower/offload.c    |  11 ++
 5 files changed, 271 insertions(+)
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/action.c

diff --git a/drivers/net/ethernet/netronome/nfp/Makefile b/drivers/net/ethernet/netronome/nfp/Makefile
index 018cef3fa10a..1ba0ea78adc3 100644
--- a/drivers/net/ethernet/netronome/nfp/Makefile
+++ b/drivers/net/ethernet/netronome/nfp/Makefile
@@ -31,6 +31,7 @@ nfp-objs := \
 
 ifeq ($(CONFIG_NFP_APP_FLOWER),y)
 nfp-objs += \
+	    flower/action.o \
 	    flower/cmsg.o \
 	    flower/main.o \
 	    flower/match.o \
diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
new file mode 100644
index 000000000000..291b9af0e5d4
--- /dev/null
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -0,0 +1,211 @@
+/*
+ * Copyright (C) 2017 Netronome Systems, Inc.
+ *
+ * This software is dual licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree or the BSD 2-Clause License provided below.  You have the
+ * option to license this software under the complete terms of either license.
+ *
+ * The BSD 2-Clause License:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      1. Redistributions of source code must retain the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer.
+ *
+ *      2. Redistributions in binary form must reproduce the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer in the documentation and/or other materials
+ *         provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/bitfield.h>
+#include <net/pkt_cls.h>
+#include <net/switchdev.h>
+#include <net/tc_act/tc_gact.h>
+#include <net/tc_act/tc_mirred.h>
+#include <net/tc_act/tc_vlan.h>
+
+#include "cmsg.h"
+#include "main.h"
+#include "../nfp_net_repr.h"
+
+static void nfp_fl_pop_vlan(struct nfp_fl_pop_vlan *pop_vlan)
+{
+	size_t act_size = sizeof(struct nfp_fl_pop_vlan);
+	u16 tmp_pop_vlan_op;
+
+	tmp_pop_vlan_op =
+		FIELD_PREP(NFP_FL_ACT_LEN_LW, act_size / NFP_FL_LW_SIZ) |
+		FIELD_PREP(NFP_FL_ACT_JMP_ID, NFP_FL_ACTION_OPCODE_POP_VLAN);
+
+	pop_vlan->a_op = cpu_to_be16(tmp_pop_vlan_op);
+	pop_vlan->reserved = 0;
+}
+
+static void
+nfp_fl_push_vlan(struct nfp_fl_push_vlan *push_vlan,
+		 const struct tc_action *action)
+{
+	size_t act_size = sizeof(struct nfp_fl_push_vlan);
+	struct tcf_vlan *vlan = to_vlan(action);
+	u16 tmp_push_vlan_tci;
+	u16 tmp_push_vlan_op;
+
+	tmp_push_vlan_op =
+		FIELD_PREP(NFP_FL_ACT_LEN_LW, act_size / NFP_FL_LW_SIZ) |
+		FIELD_PREP(NFP_FL_ACT_JMP_ID, NFP_FL_ACTION_OPCODE_PUSH_VLAN);
+
+	push_vlan->a_op = cpu_to_be16(tmp_push_vlan_op);
+	/* Set action push vlan parameters. */
+	push_vlan->reserved = 0;
+	push_vlan->vlan_tpid = tcf_vlan_push_proto(action);
+
+	tmp_push_vlan_tci =
+		FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
+		FIELD_PREP(NFP_FL_PUSH_VLAN_VID, vlan->tcfv_push_vid) |
+		NFP_FL_PUSH_VLAN_CFI;
+	push_vlan->vlan_tci = cpu_to_be16(tmp_push_vlan_tci);
+}
+
+static int
+nfp_fl_output(struct nfp_fl_output *output, const struct tc_action *action,
+	      struct nfp_fl_payload *nfp_flow, bool last,
+	      struct net_device *in_dev)
+{
+	size_t act_size = sizeof(struct nfp_fl_output);
+	struct net_device *out_dev;
+	u16 tmp_output_op;
+	int ifindex;
+
+	/* Set action opcode to output action. */
+	tmp_output_op =
+		FIELD_PREP(NFP_FL_ACT_LEN_LW, act_size / NFP_FL_LW_SIZ) |
+		FIELD_PREP(NFP_FL_ACT_JMP_ID, NFP_FL_ACTION_OPCODE_OUTPUT);
+
+	output->a_op = cpu_to_be16(tmp_output_op);
+
+	/* Set action output parameters. */
+	output->flags = cpu_to_be16(last ? NFP_FL_OUT_FLAGS_LAST : 0);
+
+	ifindex = tcf_mirred_ifindex(action);
+	out_dev = __dev_get_by_index(dev_net(in_dev), ifindex);
+	if (!out_dev)
+		return -EOPNOTSUPP;
+
+	/* Only offload egress ports are on the same device as the ingress
+	 * port.
+	 */
+	if (!switchdev_port_same_parent_id(in_dev, out_dev))
+		return -EOPNOTSUPP;
+
+	output->port = cpu_to_be32(nfp_repr_get_port_id(out_dev));
+	if (!output->port)
+		return -EOPNOTSUPP;
+
+	nfp_flow->meta.shortcut = output->port;
+
+	return 0;
+}
+
+static int
+nfp_flower_loop_action(const struct tc_action *a,
+		       struct nfp_fl_payload *nfp_fl, int *a_len,
+		       struct net_device *netdev)
+{
+	struct nfp_fl_push_vlan *psh_v;
+	struct nfp_fl_pop_vlan *pop_v;
+	struct nfp_fl_output *output;
+	int err;
+
+	if (is_tcf_gact_shot(a)) {
+		nfp_fl->meta.shortcut = cpu_to_be32(NFP_FL_SC_ACT_DROP);
+	} else if (is_tcf_mirred_egress_redirect(a)) {
+		if (*a_len + sizeof(struct nfp_fl_output) > NFP_FL_MAX_A_SIZ)
+			return -EOPNOTSUPP;
+
+		output = (struct nfp_fl_output *)&nfp_fl->action_data[*a_len];
+		err = nfp_fl_output(output, a, nfp_fl, true, netdev);
+		if (err)
+			return err;
+
+		*a_len += sizeof(struct nfp_fl_output);
+	} else if (is_tcf_mirred_egress_mirror(a)) {
+		if (*a_len + sizeof(struct nfp_fl_output) > NFP_FL_MAX_A_SIZ)
+			return -EOPNOTSUPP;
+
+		output = (struct nfp_fl_output *)&nfp_fl->action_data[*a_len];
+		err = nfp_fl_output(output, a, nfp_fl, false, netdev);
+		if (err)
+			return err;
+
+		*a_len += sizeof(struct nfp_fl_output);
+	} else if (is_tcf_vlan(a) && tcf_vlan_action(a) == TCA_VLAN_ACT_POP) {
+		if (*a_len + sizeof(struct nfp_fl_pop_vlan) > NFP_FL_MAX_A_SIZ)
+			return -EOPNOTSUPP;
+
+		pop_v = (struct nfp_fl_pop_vlan *)&nfp_fl->action_data[*a_len];
+		nfp_fl->meta.shortcut = cpu_to_be32(NFP_FL_SC_ACT_POPV);
+
+		nfp_fl_pop_vlan(pop_v);
+		*a_len += sizeof(struct nfp_fl_pop_vlan);
+	} else if (is_tcf_vlan(a) && tcf_vlan_action(a) == TCA_VLAN_ACT_PUSH) {
+		if (*a_len + sizeof(struct nfp_fl_push_vlan) > NFP_FL_MAX_A_SIZ)
+			return -EOPNOTSUPP;
+
+		psh_v = (struct nfp_fl_push_vlan *)&nfp_fl->action_data[*a_len];
+		nfp_fl->meta.shortcut = cpu_to_be32(NFP_FL_SC_ACT_NULL);
+
+		nfp_fl_push_vlan(psh_v, a);
+		*a_len += sizeof(struct nfp_fl_push_vlan);
+	} else {
+		/* Currently we do not handle any other actions. */
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+int nfp_flower_compile_action(struct tc_cls_flower_offload *flow,
+			      struct net_device *netdev,
+			      struct nfp_fl_payload *nfp_flow)
+{
+	int act_len, act_cnt, err;
+	const struct tc_action *a;
+	LIST_HEAD(actions);
+
+	memset(nfp_flow->action_data, 0, NFP_FL_MAX_A_SIZ);
+	nfp_flow->meta.act_len = 0;
+	act_len = 0;
+	act_cnt = 0;
+
+	tcf_exts_to_list(flow->exts, &actions);
+	list_for_each_entry(a, &actions, list) {
+		err = nfp_flower_loop_action(a, nfp_flow, &act_len, netdev);
+		if (err)
+			return err;
+		act_cnt++;
+	}
+
+	/* We optimise when the action list is small, this can unfortunately
+	 * not happen once we have more than one action in the action list.
+	 */
+	if (act_cnt > 1)
+		nfp_flow->meta.shortcut = cpu_to_be32(NFP_FL_SC_ACT_NULL);
+
+	nfp_flow->meta.act_len = act_len;
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 1956c1acf39f..4c72e537af32 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -56,6 +56,51 @@
 #define NFP_FLOWER_MASK_VLAN_CFI	BIT(12)
 #define NFP_FLOWER_MASK_VLAN_VID	GENMASK(11, 0)
 
+#define NFP_FL_SC_ACT_DROP		0x80000000
+#define NFP_FL_SC_ACT_USER		0x7D000000
+#define NFP_FL_SC_ACT_POPV		0x6A000000
+#define NFP_FL_SC_ACT_NULL		0x00000000
+
+/* The maximum action list size (in bytes) supported by the NFP.
+ */
+#define NFP_FL_MAX_A_SIZ		1216
+#define NFP_FL_LW_SIZ			4
+
+/* Action opcodes */
+#define NFP_FL_ACTION_OPCODE_OUTPUT	0
+#define NFP_FL_ACTION_OPCODE_PUSH_VLAN	1
+#define NFP_FL_ACTION_OPCODE_POP_VLAN	2
+#define NFP_FL_ACTION_OPCODE_NUM	32
+
+#define NFP_FL_ACT_JMP_ID		GENMASK(15, 8)
+#define NFP_FL_ACT_LEN_LW		GENMASK(7, 0)
+
+#define NFP_FL_OUT_FLAGS_LAST		BIT(15)
+#define NFP_FL_OUT_FLAGS_USE_TUN	BIT(4)
+#define NFP_FL_OUT_FLAGS_TYPE_IDX	GENMASK(2, 0)
+
+#define NFP_FL_PUSH_VLAN_PRIO		GENMASK(15, 13)
+#define NFP_FL_PUSH_VLAN_CFI		BIT(12)
+#define NFP_FL_PUSH_VLAN_VID		GENMASK(11, 0)
+
+struct nfp_fl_output {
+	__be16 a_op;
+	__be16 flags;
+	__be32 port;
+};
+
+struct nfp_fl_push_vlan {
+	__be16 a_op;
+	__be16 reserved;
+	__be16 vlan_tpid;
+	__be16 vlan_tci;
+};
+
+struct nfp_fl_pop_vlan {
+	__be16 a_op;
+	__be16 reserved;
+};
+
 /* Metadata without L2 (1W/4B)
  * ----------------------------------------------------------------
  *    3                   2                   1
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 5ba7e5194708..7c9530504752 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -70,5 +70,8 @@ int nfp_flower_compile_flow_match(struct tc_cls_flower_offload *flow,
 				  struct nfp_fl_key_ls *key_ls,
 				  struct net_device *netdev,
 				  struct nfp_fl_payload *nfp_flow);
+int nfp_flower_compile_action(struct tc_cls_flower_offload *flow,
+			      struct net_device *netdev,
+			      struct nfp_fl_payload *nfp_flow);
 
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 0d0ac2241b56..f1cb0bd54137 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -165,10 +165,16 @@ nfp_flower_allocate_new(struct nfp_fl_key_ls *key_layer)
 	if (!flow_pay->mask_data)
 		goto err_free_unmasked;
 
+	flow_pay->action_data = kmalloc(NFP_FL_MAX_A_SIZ, GFP_KERNEL);
+	if (!flow_pay->action_data)
+		goto err_free_mask;
+
 	flow_pay->meta.flags = 0;
 
 	return flow_pay;
 
+err_free_mask:
+	kfree(flow_pay->mask_data);
 err_free_unmasked:
 	kfree(flow_pay->unmasked_data);
 err_free_flow:
@@ -178,6 +184,7 @@ nfp_flower_allocate_new(struct nfp_fl_key_ls *key_layer)
 
 static void nfp_flower_deallocate_nfp(struct nfp_fl_payload *flow_pay)
 {
+	kfree(flow_pay->action_data);
 	kfree(flow_pay->mask_data);
 	kfree(flow_pay->unmasked_data);
 	kfree(flow_pay);
@@ -219,6 +226,10 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 	if (err)
 		goto err_destroy_flow;
 
+	err = nfp_flower_compile_action(flow, netdev, flow_pay);
+	if (err)
+		goto err_destroy_flow;
+
 	/* TODO: Complete flower_add_offload. */
 	err = -EOPNOTSUPP;
 
-- 
2.1.4

[PATCH net-next v2 7/9] nfp: add metadata to each flow offload

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Adds metadata describing the mask id of each flow and keeps track of
flows installed in hardware. Previously a flow could not be removed
from hardware as there was no way of knowing if that a specific flow
was installed. This is solved by storing the offloaded flows in a
hash table.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/Makefile        |   1 +
 drivers/net/ethernet/netronome/nfp/flower/main.c   |  14 +-
 drivers/net/ethernet/netronome/nfp/flower/main.h   |  53 +++
 .../net/ethernet/netronome/nfp/flower/metadata.c   | 377 +++++++++++++++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    |  22 +-
 5 files changed, 456 insertions(+), 11 deletions(-)
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/metadata.c

diff --git a/drivers/net/ethernet/netronome/nfp/Makefile b/drivers/net/ethernet/netronome/nfp/Makefile
index 1ba0ea78adc3..b8e1358868bd 100644
--- a/drivers/net/ethernet/netronome/nfp/Makefile
+++ b/drivers/net/ethernet/netronome/nfp/Makefile
@@ -35,6 +35,7 @@ nfp-objs += \
 	    flower/cmsg.o \
 	    flower/main.o \
 	    flower/match.o \
+	    flower/metadata.o \
 	    flower/offload.o
 endif
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index 19f20f819e2f..1103d23a8ec7 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -50,14 +50,6 @@
 
 #define NFP_FLOWER_ALLOWED_VER 0x0001000000010000UL
 
-/**
- * struct nfp_flower_priv - Flower APP per-vNIC priv data
- * @nn:		     Pointer to vNIC
- */
-struct nfp_flower_priv {
-	struct nfp_net *nn;
-};
-
 static const char *nfp_flower_extra_cap(struct nfp_app *app, struct nfp_net *nn)
 {
 	return "FLOWER";
@@ -351,6 +343,12 @@ static int nfp_flower_init(struct nfp_app *app)
 	if (!app->priv)
 		return -ENOMEM;
 
+	err = nfp_flower_metadata_init(app);
+	if (nfp_flower_metadata_init(app)) {
+		kfree(app->priv);
+		return err;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 7c9530504752..ae54b8052043 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -34,12 +34,51 @@
 #ifndef __NFP_FLOWER_H__
 #define __NFP_FLOWER_H__ 1
 
+#include <linux/circ_buf.h>
+#include <linux/hashtable.h>
+#include <linux/time64.h>
 #include <linux/types.h>
 
 struct tc_to_netdev;
 struct net_device;
 struct nfp_app;
 
+#define NFP_FLOWER_HASH_BITS		10
+#define NFP_FLOWER_HASH_SEED		129004
+
+#define NFP_FLOWER_MASK_ENTRY_RS	256
+#define NFP_FLOWER_MASK_ELEMENT_RS	1
+#define NFP_FLOWER_MASK_HASH_BITS	10
+#define NFP_FLOWER_MASK_HASH_SEED	9198806
+
+#define NFP_FL_META_FLAG_NEW_MASK	128
+#define NFP_FL_META_FLAG_LAST_MASK	1
+
+#define NFP_FL_MASK_REUSE_TIME_NS	40000
+#define NFP_FL_MASK_ID_LOCATION		1
+
+struct nfp_fl_mask_id {
+	struct circ_buf mask_id_free_list;
+	struct timespec64 *last_used;
+	u8 init_unallocated;
+};
+
+/**
+ * struct nfp_flower_priv - Flower APP per-vNIC priv data
+ * @nn:			Pointer to vNIC
+ * @flower_version:	HW version of flower
+ * @mask_ids:		List of free mask ids
+ * @mask_table:		Hash table used to store masks
+ * @flow_table:		Hash table used to store flower rules
+ */
+struct nfp_flower_priv {
+	struct nfp_net *nn;
+	u64 flower_version;
+	struct nfp_fl_mask_id mask_ids;
+	DECLARE_HASHTABLE(mask_table, NFP_FLOWER_MASK_HASH_BITS);
+	DECLARE_HASHTABLE(flow_table, NFP_FLOWER_HASH_BITS);
+};
+
 struct nfp_fl_key_ls {
 	u32 key_layer_two;
 	u8 key_layer;
@@ -64,6 +103,9 @@ struct nfp_fl_payload {
 	char *action_data;
 };
 
+int nfp_flower_metadata_init(struct nfp_app *app);
+void nfp_flower_metadata_cleanup(struct nfp_app *app);
+
 int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
 			u32 handle, __be16 proto, struct tc_to_netdev *tc);
 int nfp_flower_compile_flow_match(struct tc_cls_flower_offload *flow,
@@ -73,5 +115,16 @@ int nfp_flower_compile_flow_match(struct tc_cls_flower_offload *flow,
 int nfp_flower_compile_action(struct tc_cls_flower_offload *flow,
 			      struct net_device *netdev,
 			      struct nfp_fl_payload *nfp_flow);
+int nfp_compile_flow_metadata(struct nfp_app *app,
+			      struct tc_cls_flower_offload *flow,
+			      struct nfp_fl_payload *nfp_flow);
+int nfp_modify_flow_metadata(struct nfp_app *app,
+			     struct nfp_fl_payload *nfp_flow);
+
+struct nfp_fl_payload *
+nfp_flower_find_in_fl_table(struct nfp_app *app,
+			    unsigned long tc_flower_cookie);
+struct nfp_fl_payload *
+nfp_flower_remove_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie);
 
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/metadata.c b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
new file mode 100644
index 000000000000..51de9ab85951
--- /dev/null
+++ b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
@@ -0,0 +1,377 @@
+/*
+ * Copyright (C) 2017 Netronome Systems, Inc.
+ *
+ * This software is dual licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree or the BSD 2-Clause License provided below.  You have the
+ * option to license this software under the complete terms of either license.
+ *
+ * The BSD 2-Clause License:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      1. Redistributions of source code must retain the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer.
+ *
+ *      2. Redistributions in binary form must reproduce the above
+ *         copyright notice, this list of conditions and the following
+ *         disclaimer in the documentation and/or other materials
+ *         provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/hash.h>
+#include <linux/hashtable.h>
+#include <linux/jhash.h>
+#include <linux/vmalloc.h>
+#include <net/pkt_cls.h>
+
+#include "cmsg.h"
+#include "main.h"
+#include "../nfp_app.h"
+
+struct nfp_mask_id_table {
+	struct hlist_node link;
+	u32 hash_key;
+	u32 ref_cnt;
+	u8 mask_id;
+};
+
+struct nfp_flower_table {
+	unsigned long tc_flower_cookie;
+	struct nfp_fl_payload *nfp_flow;
+	struct hlist_node link;
+	struct rcu_head rcu;
+};
+
+static struct nfp_flower_table *
+nfp_flower_search_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct nfp_flower_table *flower_entry;
+
+	hash_for_each_possible_rcu(priv->flow_table, flower_entry, link,
+				   tc_flower_cookie)
+		if (flower_entry->tc_flower_cookie == tc_flower_cookie)
+			return flower_entry;
+
+	return NULL;
+}
+
+static int
+nfp_flower_add_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie,
+			struct nfp_fl_payload *nfp_flow)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct nfp_flower_table *flower_entry;
+
+	flower_entry = nfp_flower_search_fl_table(app, tc_flower_cookie);
+	if (flower_entry)
+		return -EEXIST;
+
+	flower_entry = kmalloc(sizeof(*flower_entry), GFP_KERNEL);
+	if (!flower_entry)
+		return -ENOMEM;
+
+	INIT_HLIST_NODE(&flower_entry->link);
+	flower_entry->nfp_flow = nfp_flow;
+	flower_entry->tc_flower_cookie = tc_flower_cookie;
+	hash_add_rcu(priv->flow_table, &flower_entry->link, tc_flower_cookie);
+
+	return 0;
+}
+
+struct nfp_fl_payload *
+nfp_flower_find_in_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie)
+{
+	struct nfp_flower_table *flow_entry;
+
+	flow_entry = nfp_flower_search_fl_table(app, tc_flower_cookie);
+	if (!flow_entry)
+		return NULL;
+
+	return flow_entry->nfp_flow;
+}
+
+struct nfp_fl_payload *
+nfp_flower_remove_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie)
+{
+	struct nfp_flower_table *flow_entry;
+	struct nfp_fl_payload *nfp_flow;
+
+	flow_entry = nfp_flower_search_fl_table(app, tc_flower_cookie);
+	if (!flow_entry)
+		return NULL;
+
+	nfp_flow = flow_entry->nfp_flow;
+	hash_del_rcu(&flow_entry->link);
+	/* Allow caller to rely on there being no other references for
+	 * further cleanup of nfp_flow. Otherwise kfree_rcu() could be
+	 * used.
+	 */
+	synchronize_rcu();
+	kfree(flow_entry);
+
+	return nfp_flow;
+}
+
+static int nfp_release_mask_id(struct nfp_app *app, u8 mask_id)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct circ_buf *ring;
+	struct timespec64 now;
+
+	ring = &priv->mask_ids.mask_id_free_list;
+	/* Checking if buffer is full. */
+	if (CIRC_SPACE(ring->head, ring->tail, NFP_FLOWER_MASK_ENTRY_RS) == 0)
+		return -ENOBUFS;
+
+	memcpy(&ring->buf[ring->head], &mask_id, NFP_FLOWER_MASK_ELEMENT_RS);
+	ring->head = (ring->head + NFP_FLOWER_MASK_ELEMENT_RS) %
+		     (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
+
+	getnstimeofday64(&now);
+	priv->mask_ids.last_used[mask_id] = now;
+
+	return 0;
+}
+
+static int nfp_mask_alloc(struct nfp_app *app, u8 *mask_id)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct timespec64 delta, now;
+	struct circ_buf *ring;
+	u8 temp_id, freed_id;
+
+	ring = &priv->mask_ids.mask_id_free_list;
+	freed_id = NFP_FLOWER_MASK_ENTRY_RS - 1;
+	/* Checking for unallocated entries first. */
+	if (priv->mask_ids.init_unallocated > 0) {
+		*mask_id = priv->mask_ids.init_unallocated;
+		priv->mask_ids.init_unallocated--;
+		return 0;
+	}
+
+	/* Checking if buffer is empty. */
+	if (ring->head == ring->tail) {
+		*mask_id = freed_id;
+		return -ENOENT;
+	}
+
+	memcpy(&temp_id, &ring->buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
+	*mask_id = temp_id;
+	memcpy(&ring->buf[ring->tail], &freed_id, NFP_FLOWER_MASK_ELEMENT_RS);
+	ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
+		     (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
+
+	getnstimeofday64(&now);
+	delta = timespec64_sub(now, priv->mask_ids.last_used[*mask_id]);
+
+	if (timespec64_to_ns(&delta) < NFP_FL_MASK_REUSE_TIME_NS) {
+		nfp_release_mask_id(app, *mask_id);
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+static int
+nfp_add_mask_table(struct nfp_app *app, char *mask_data, u32 mask_len)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct nfp_mask_id_table *mask_entry;
+	unsigned long hash_key;
+	u8 mask_id;
+
+	if (nfp_mask_alloc(app, &mask_id))
+		return -ENOENT;
+
+	mask_entry = kmalloc(sizeof(*mask_entry), GFP_KERNEL);
+	if (!mask_entry) {
+		nfp_release_mask_id(app, mask_id);
+		return -ENOMEM;
+	}
+
+	INIT_HLIST_NODE(&mask_entry->link);
+	mask_entry->mask_id = mask_id;
+	hash_key = jhash(mask_data, mask_len, NFP_FLOWER_MASK_HASH_SEED);
+	mask_entry->hash_key = hash_key;
+	mask_entry->ref_cnt = 1;
+	hash_add(priv->mask_table, &mask_entry->link, hash_key);
+
+	return mask_id;
+}
+
+static struct nfp_mask_id_table *
+nfp_search_mask_table(struct nfp_app *app, char *mask_data, u32 mask_len)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct nfp_mask_id_table *mask_entry;
+	unsigned long hash_key;
+
+	hash_key = jhash(mask_data, mask_len, NFP_FLOWER_MASK_HASH_SEED);
+
+	hash_for_each_possible(priv->mask_table, mask_entry, link, hash_key)
+		if (mask_entry->hash_key == hash_key)
+			return mask_entry;
+
+	return NULL;
+}
+
+static int
+nfp_find_in_mask_table(struct nfp_app *app, char *mask_data, u32 mask_len)
+{
+	struct nfp_mask_id_table *mask_entry;
+
+	mask_entry = nfp_search_mask_table(app, mask_data, mask_len);
+	if (!mask_entry)
+		return -ENOENT;
+
+	mask_entry->ref_cnt++;
+
+	/* Casting u8 to int for later use. */
+	return mask_entry->mask_id;
+}
+
+static bool
+nfp_check_mask_add(struct nfp_app *app, char *mask_data, u32 mask_len,
+		   u8 *meta_flags, u8 *mask_id)
+{
+	int id;
+
+	id = nfp_find_in_mask_table(app, mask_data, mask_len);
+	if (id < 0) {
+		id = nfp_add_mask_table(app, mask_data, mask_len);
+		if (id < 0)
+			return false;
+		*meta_flags |= NFP_FL_META_FLAG_NEW_MASK;
+	}
+	*mask_id = id;
+
+	return true;
+}
+
+static bool
+nfp_check_mask_remove(struct nfp_app *app, char *mask_data, u32 mask_len,
+		      u8 *meta_flags, u8 *mask_id)
+{
+	struct nfp_mask_id_table *mask_entry;
+
+	mask_entry = nfp_search_mask_table(app, mask_data, mask_len);
+	if (!mask_entry)
+		return false;
+
+	*mask_id = mask_entry->mask_id;
+	mask_entry->ref_cnt--;
+	if (!mask_entry->ref_cnt) {
+		hash_del(&mask_entry->link);
+		nfp_release_mask_id(app, *mask_id);
+		kfree(mask_entry);
+		if (meta_flags)
+			*meta_flags |= NFP_FL_META_FLAG_LAST_MASK;
+	}
+
+	return true;
+}
+
+int nfp_compile_flow_metadata(struct nfp_app *app,
+			      struct tc_cls_flower_offload *flow,
+			      struct nfp_fl_payload *nfp_flow)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	u8 new_mask_id;
+	int err;
+
+	new_mask_id = 0;
+	if (!nfp_check_mask_add(app, nfp_flow->mask_data,
+				nfp_flow->meta.mask_len,
+				&nfp_flow->meta.flags, &new_mask_id))
+		return -ENOENT;
+
+	nfp_flow->meta.flow_version = cpu_to_be64(priv->flower_version);
+	priv->flower_version++;
+
+	/* Update flow payload with mask ids. */
+	nfp_flow->unmasked_data[NFP_FL_MASK_ID_LOCATION] = new_mask_id;
+
+	err = nfp_flower_add_fl_table(app, flow->cookie, nfp_flow);
+	if (err < 0) {
+		if (!nfp_check_mask_remove(app, nfp_flow->mask_data,
+					   nfp_flow->meta.mask_len,
+					   NULL, &new_mask_id))
+			return -EINVAL;
+
+		return err;
+	}
+
+	return 0;
+}
+
+int nfp_modify_flow_metadata(struct nfp_app *app,
+			     struct nfp_fl_payload *nfp_flow)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	u8 new_mask_id = 0;
+
+	nfp_check_mask_remove(app, nfp_flow->mask_data,
+			      nfp_flow->meta.mask_len, &nfp_flow->meta.flags,
+			      &new_mask_id);
+
+	nfp_flow->meta.flow_version = cpu_to_be64(priv->flower_version);
+	priv->flower_version++;
+
+	/* Update flow payload with mask ids. */
+	nfp_flow->unmasked_data[NFP_FL_MASK_ID_LOCATION] = new_mask_id;
+
+	return 0;
+}
+
+int nfp_flower_metadata_init(struct nfp_app *app)
+{
+	struct nfp_flower_priv *priv = app->priv;
+
+	hash_init(priv->mask_table);
+	hash_init(priv->flow_table);
+
+	/* Init ring buffer and unallocated mask_ids. */
+	priv->mask_ids.mask_id_free_list.buf =
+		kmalloc(NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS,
+			GFP_KERNEL);
+	if (!priv->mask_ids.mask_id_free_list.buf)
+		return -ENOMEM;
+
+	priv->mask_ids.init_unallocated = NFP_FLOWER_MASK_ENTRY_RS - 1;
+
+	/* Init timestamps for mask id*/
+	priv->mask_ids.last_used =
+		kmalloc_array(NFP_FLOWER_MASK_ENTRY_RS,
+			      sizeof(*priv->mask_ids.last_used), GFP_KERNEL);
+	if (!priv->mask_ids.last_used) {
+		kfree(priv->mask_ids.mask_id_free_list.buf);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void nfp_flower_metadata_cleanup(struct nfp_app *app)
+{
+	struct nfp_flower_priv *priv = app->priv;
+
+	if (!priv)
+		return;
+
+	kfree(priv->mask_ids.mask_id_free_list.buf);
+	kfree(priv->mask_ids.last_used);
+}
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index f1cb0bd54137..d0c0350efef1 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -230,8 +230,14 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 	if (err)
 		goto err_destroy_flow;
 
-	/* TODO: Complete flower_add_offload. */
-	err = -EOPNOTSUPP;
+	err = nfp_compile_flow_metadata(app, flow, flow_pay);
+	if (err)
+		goto err_destroy_flow;
+
+	/* Deallocate flow payload when flower rule has been destroyed. */
+	kfree(key_layer);
+
+	return 0;
 
 err_destroy_flow:
 	nfp_flower_deallocate_nfp(flow_pay);
@@ -256,7 +262,17 @@ static int
 nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
 		       struct tc_cls_flower_offload *flow)
 {
-	return -EOPNOTSUPP;
+	struct nfp_fl_payload *nfp_flow;
+	int err;
+
+	nfp_flow = nfp_flower_remove_fl_table(app, flow->cookie);
+	if (!nfp_flow)
+		return -ENOENT;
+
+	err = nfp_modify_flow_metadata(app, nfp_flow);
+
+	nfp_flower_deallocate_nfp(nfp_flow);
+	return err;
 }
 
 /**
-- 
2.1.4

Re: [PATCH net-next v2 7/9] nfp: add metadata to each flow offload

[Jakub Kicinski]

On Wed, 28 Jun 2017 22:30:00 +0200, Simon Horman wrote:
> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> 
> Adds metadata describing the mask id of each flow and keeps track of
> flows installed in hardware. Previously a flow could not be removed
> from hardware as there was no way of knowing if that a specific flow
> was installed. This is solved by storing the offloaded flows in a
> hash table.
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

> diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
> index 19f20f819e2f..1103d23a8ec7 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/main.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
> @@ -50,14 +50,6 @@
>  
>  #define NFP_FLOWER_ALLOWED_VER 0x0001000000010000UL
>  
> -/**
> - * struct nfp_flower_priv - Flower APP per-vNIC priv data
> - * @nn:		     Pointer to vNIC
> - */
> -struct nfp_flower_priv {
> -	struct nfp_net *nn;
> -};
> -
>  static const char *nfp_flower_extra_cap(struct nfp_app *app, struct nfp_net *nn)
>  {
>  	return "FLOWER";
> @@ -351,6 +343,12 @@ static int nfp_flower_init(struct nfp_app *app)
>  	if (!app->priv)
>  		return -ENOMEM;
>  
> +	err = nfp_flower_metadata_init(app);
> +	if (nfp_flower_metadata_init(app)) {

You're calling init twice here.  Also please do the error path with
goto, as explained in review of patch 8 having a proper unwind makes
later patches easier to review.

> +		kfree(app->priv);
> +		return err;
> +	}
> +
>  	return 0;
>  }
>  

> +static int nfp_mask_alloc(struct nfp_app *app, u8 *mask_id)
> +{
> +	struct nfp_flower_priv *priv = app->priv;
> +	struct timespec64 delta, now;
> +	struct circ_buf *ring;
> +	u8 temp_id, freed_id;
> +
> +	ring = &priv->mask_ids.mask_id_free_list;
> +	freed_id = NFP_FLOWER_MASK_ENTRY_RS - 1;
> +	/* Checking for unallocated entries first. */
> +	if (priv->mask_ids.init_unallocated > 0) {
> +		*mask_id = priv->mask_ids.init_unallocated;
> +		priv->mask_ids.init_unallocated--;
> +		return 0;
> +	}
> +
> +	/* Checking if buffer is empty. */
> +	if (ring->head == ring->tail) {
> +		*mask_id = freed_id;
> +		return -ENOENT;
> +	}
> +
> +	memcpy(&temp_id, &ring->buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
> +	*mask_id = temp_id;
> +	memcpy(&ring->buf[ring->tail], &freed_id, NFP_FLOWER_MASK_ELEMENT_RS);
> +	ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
> +		     (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
> +
> +	getnstimeofday64(&now);
> +	delta = timespec64_sub(now, priv->mask_ids.last_used[*mask_id]);
> +
> +	if (timespec64_to_ns(&delta) < NFP_FL_MASK_REUSE_TIME_NS) {
> +		nfp_release_mask_id(app, *mask_id);

nfp_release_mask_id() will reset the time stamp and put the mask at the
end of the queue.  Is that OK?

> +		return -ENOENT;
> +	}
> +
> +	return 0;
> +}
> +

> +int nfp_flower_metadata_init(struct nfp_app *app)
> +{
> +	struct nfp_flower_priv *priv = app->priv;
> +
> +	hash_init(priv->mask_table);
> +	hash_init(priv->flow_table);
> +
> +	/* Init ring buffer and unallocated mask_ids. */
> +	priv->mask_ids.mask_id_free_list.buf =
> +		kmalloc(NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS,
> +			GFP_KERNEL);

kmalloc_array, perhaps?  

> +	if (!priv->mask_ids.mask_id_free_list.buf)
> +		return -ENOMEM;
> +
> +	priv->mask_ids.init_unallocated = NFP_FLOWER_MASK_ENTRY_RS - 1;
> +
> +	/* Init timestamps for mask id*/
> +	priv->mask_ids.last_used =
> +		kmalloc_array(NFP_FLOWER_MASK_ENTRY_RS,
> +			      sizeof(*priv->mask_ids.last_used), GFP_KERNEL);
> +	if (!priv->mask_ids.last_used) {
> +		kfree(priv->mask_ids.mask_id_free_list.buf);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}

Re: [oss-drivers] Re: [PATCH net-next v2 7/9] nfp: add metadata to each flow offload

[Simon Horman]

On Wed, Jun 28, 2017 at 07:33:02PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2017 22:30:00 +0200, Simon Horman wrote:
> > From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > 
> > Adds metadata describing the mask id of each flow and keeps track of
> > flows installed in hardware. Previously a flow could not be removed
> > from hardware as there was no way of knowing if that a specific flow
> > was installed. This is solved by storing the offloaded flows in a
> > hash table.
> > 
> > Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> > diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
> > index 19f20f819e2f..1103d23a8ec7 100644
> > --- a/drivers/net/ethernet/netronome/nfp/flower/main.c
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
> > @@ -50,14 +50,6 @@
> >  
> >  #define NFP_FLOWER_ALLOWED_VER 0x0001000000010000UL
> >  
> > -/**
> > - * struct nfp_flower_priv - Flower APP per-vNIC priv data
> > - * @nn:		     Pointer to vNIC
> > - */
> > -struct nfp_flower_priv {
> > -	struct nfp_net *nn;
> > -};
> > -
> >  static const char *nfp_flower_extra_cap(struct nfp_app *app, struct nfp_net *nn)
> >  {
> >  	return "FLOWER";
> > @@ -351,6 +343,12 @@ static int nfp_flower_init(struct nfp_app *app)
> >  	if (!app->priv)
> >  		return -ENOMEM;
> >  
> > +	err = nfp_flower_metadata_init(app);
> > +	if (nfp_flower_metadata_init(app)) {
> 
> You're calling init twice here.  Also please do the error path with
> goto, as explained in review of patch 8 having a proper unwind makes
> later patches easier to review.

Sorry, that was a brain-o on my part. Will fix that and add proper unwind
as you suggest.

> 
> > +		kfree(app->priv);
> > +		return err;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> 
> > +static int nfp_mask_alloc(struct nfp_app *app, u8 *mask_id)
> > +{
> > +	struct nfp_flower_priv *priv = app->priv;
> > +	struct timespec64 delta, now;
> > +	struct circ_buf *ring;
> > +	u8 temp_id, freed_id;
> > +
> > +	ring = &priv->mask_ids.mask_id_free_list;
> > +	freed_id = NFP_FLOWER_MASK_ENTRY_RS - 1;
> > +	/* Checking for unallocated entries first. */
> > +	if (priv->mask_ids.init_unallocated > 0) {
> > +		*mask_id = priv->mask_ids.init_unallocated;
> > +		priv->mask_ids.init_unallocated--;
> > +		return 0;
> > +	}
> > +
> > +	/* Checking if buffer is empty. */
> > +	if (ring->head == ring->tail) {
> > +		*mask_id = freed_id;
> > +		return -ENOENT;
> > +	}
> > +
> > +	memcpy(&temp_id, &ring->buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
> > +	*mask_id = temp_id;
> > +	memcpy(&ring->buf[ring->tail], &freed_id, NFP_FLOWER_MASK_ELEMENT_RS);
> > +	ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
> > +		     (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
> > +
> > +	getnstimeofday64(&now);
> > +	delta = timespec64_sub(now, priv->mask_ids.last_used[*mask_id]);
> > +
> > +	if (timespec64_to_ns(&delta) < NFP_FL_MASK_REUSE_TIME_NS) {
> > +		nfp_release_mask_id(app, *mask_id);
> 
> nfp_release_mask_id() will reset the time stamp and put the mask at the
> end of the queue.  Is that OK?

I discussed this with Pieter. He believes that it is ok as it would be too
early to use the entry and its better put it to the back of the list and
skip to the next one.

> > +		return -ENOENT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> > +int nfp_flower_metadata_init(struct nfp_app *app)
> > +{
> > +	struct nfp_flower_priv *priv = app->priv;
> > +
> > +	hash_init(priv->mask_table);
> > +	hash_init(priv->flow_table);
> > +
> > +	/* Init ring buffer and unallocated mask_ids. */
> > +	priv->mask_ids.mask_id_free_list.buf =
> > +		kmalloc(NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS,
> > +			GFP_KERNEL);
> 
> kmalloc_array, perhaps?  

Yes, indeed. Will do.

> > +	if (!priv->mask_ids.mask_id_free_list.buf)
> > +		return -ENOMEM;
> > +
> > +	priv->mask_ids.init_unallocated = NFP_FLOWER_MASK_ENTRY_RS - 1;
> > +
> > +	/* Init timestamps for mask id*/
> > +	priv->mask_ids.last_used =
> > +		kmalloc_array(NFP_FLOWER_MASK_ENTRY_RS,
> > +			      sizeof(*priv->mask_ids.last_used), GFP_KERNEL);
> > +	if (!priv->mask_ids.last_used) {
> > +		kfree(priv->mask_ids.mask_id_free_list.buf);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}

Re: [oss-drivers] Re: [PATCH net-next v2 7/9] nfp: add metadata to each flow offload

[Jakub Kicinski]

On Thu, 29 Jun 2017 10:14:29 +0200, Simon Horman wrote:
> > > +	/* Checking if buffer is empty. */
> > > +	if (ring->head == ring->tail) {
> > > +		*mask_id = freed_id;
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	memcpy(&temp_id, &ring->buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
> > > +	*mask_id = temp_id;
> > > +	memcpy(&ring->buf[ring->tail], &freed_id, NFP_FLOWER_MASK_ELEMENT_RS);
> > > +	ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
> > > +		     (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
> > > +
> > > +	getnstimeofday64(&now);
> > > +	delta = timespec64_sub(now, priv->mask_ids.last_used[*mask_id]);
> > > +
> > > +	if (timespec64_to_ns(&delta) < NFP_FL_MASK_REUSE_TIME_NS) {
> > > +		nfp_release_mask_id(app, *mask_id);  
> > 
> > nfp_release_mask_id() will reset the time stamp and put the mask at the
> > end of the queue.  Is that OK?  
> 
> I discussed this with Pieter. He believes that it is ok as it would be too
> early to use the entry and its better put it to the back of the list and
> skip to the next one.

But we shouldn't update the "last use" time if the grace period haven't
elapsed otherwise we can live lock (I know, unlikely).  Could we simply
move the time check right after the:

	*mask_id = temp_id;

line?  I.e. move the check before we actually pick the mask off the
queue?

Re: [oss-drivers] Re: [PATCH net-next v2 7/9] nfp: add metadata to each flow offload

[Simon Horman]

On Thu, Jun 29, 2017 at 01:42:50AM -0700, Jakub Kicinski wrote:
> On Thu, 29 Jun 2017 10:14:29 +0200, Simon Horman wrote:
> > > > +	/* Checking if buffer is empty. */
> > > > +	if (ring->head == ring->tail) {
> > > > +		*mask_id = freed_id;
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	memcpy(&temp_id, &ring->buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
> > > > +	*mask_id = temp_id;
> > > > +	memcpy(&ring->buf[ring->tail], &freed_id, NFP_FLOWER_MASK_ELEMENT_RS);
> > > > +	ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
> > > > +		     (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
> > > > +
> > > > +	getnstimeofday64(&now);
> > > > +	delta = timespec64_sub(now, priv->mask_ids.last_used[*mask_id]);
> > > > +
> > > > +	if (timespec64_to_ns(&delta) < NFP_FL_MASK_REUSE_TIME_NS) {
> > > > +		nfp_release_mask_id(app, *mask_id);  
> > > 
> > > nfp_release_mask_id() will reset the time stamp and put the mask at the
> > > end of the queue.  Is that OK?  
> > 
> > I discussed this with Pieter. He believes that it is ok as it would be too
> > early to use the entry and its better put it to the back of the list and
> > skip to the next one.
> 
> But we shouldn't update the "last use" time if the grace period haven't
> elapsed otherwise we can live lock (I know, unlikely).  Could we simply
> move the time check right after the:
> 
> 	*mask_id = temp_id;
> 
> line?  I.e. move the check before we actually pick the mask off the
> queue?

Thanks, we now have a solution along those lines working.

[PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Previously there was no way of updating flow rule stats after they
have been offloaded to hardware. This is solved by keeping track of
stats received from hardware and providing this to the TC handler
on request.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/cmsg.c   |   5 -
 drivers/net/ethernet/netronome/nfp/flower/cmsg.h   |   5 +
 drivers/net/ethernet/netronome/nfp/flower/main.h   |  29 +++++
 .../net/ethernet/netronome/nfp/flower/metadata.c   | 124 ++++++++++++++++++++-
 .../net/ethernet/netronome/nfp/flower/offload.c    |  20 +++-
 5 files changed, 175 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.c b/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
index 916a6196d2ba..0f5410aa66d6 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
@@ -52,11 +52,6 @@ nfp_flower_cmsg_get_hdr(struct sk_buff *skb)
 	return (struct nfp_flower_cmsg_hdr *)skb->data;
 }
 
-static void *nfp_flower_cmsg_get_data(struct sk_buff *skb)
-{
-	return (unsigned char *)skb->data + NFP_FLOWER_CMSG_HLEN;
-}
-
 static struct sk_buff *
 nfp_flower_cmsg_alloc(struct nfp_app *app, unsigned int size,
 		      enum nfp_flower_cmsg_type_port type)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 4c72e537af32..736c4848f073 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -300,6 +300,11 @@ nfp_flower_cmsg_pcie_port(u8 nfp_pcie, enum nfp_flower_cmsg_port_vnic_type type,
 			   NFP_FLOWER_CMSG_PORT_TYPE_PCIE_PORT);
 }
 
+static inline void *nfp_flower_cmsg_get_data(struct sk_buff *skb)
+{
+	return (unsigned char *)skb->data + NFP_FLOWER_CMSG_HLEN;
+}
+
 int nfp_flower_cmsg_portmod(struct nfp_repr *repr, bool carrier_ok);
 void nfp_flower_cmsg_rx(struct nfp_app *app, struct sk_buff *skb);
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index ae54b8052043..cfa54540aff0 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -43,9 +43,13 @@ struct tc_to_netdev;
 struct net_device;
 struct nfp_app;
 
+#define NFP_FL_REPEATED_HASH_MAX	BIT(17)
 #define NFP_FLOWER_HASH_BITS		10
 #define NFP_FLOWER_HASH_SEED		129004
 
+#define NFP_FL_STATS_ENTRY_RS		BIT(20)
+#define NFP_FL_STATS_ELEM_RS		4
+
 #define NFP_FLOWER_MASK_ENTRY_RS	256
 #define NFP_FLOWER_MASK_ELEMENT_RS	1
 #define NFP_FLOWER_MASK_HASH_BITS	10
@@ -63,10 +67,17 @@ struct nfp_fl_mask_id {
 	u8 init_unallocated;
 };
 
+struct nfp_fl_stats_id {
+	struct circ_buf free_list;
+	u32 init_unalloc;
+	u8 repeated_em_count;
+};
+
 /**
  * struct nfp_flower_priv - Flower APP per-vNIC priv data
  * @nn:			Pointer to vNIC
  * @flower_version:	HW version of flower
+ * @stats_ids:		List of free stats ids
  * @mask_ids:		List of free mask ids
  * @mask_table:		Hash table used to store masks
  * @flow_table:		Hash table used to store flower rules
@@ -74,6 +85,7 @@ struct nfp_fl_mask_id {
 struct nfp_flower_priv {
 	struct nfp_net *nn;
 	u64 flower_version;
+	struct nfp_fl_stats_id stats_ids;
 	struct nfp_fl_mask_id mask_ids;
 	DECLARE_HASHTABLE(mask_table, NFP_FLOWER_MASK_HASH_BITS);
 	DECLARE_HASHTABLE(flow_table, NFP_FLOWER_HASH_BITS);
@@ -96,13 +108,28 @@ struct nfp_fl_rule_metadata {
 	__be32 shortcut;
 };
 
+struct nfp_fl_stats {
+	u64 pkts;
+	u64 bytes;
+	u64 used;
+};
+
 struct nfp_fl_payload {
 	struct nfp_fl_rule_metadata meta;
+	spinlock_t lock; /* lock stats */
+	struct nfp_fl_stats stats;
 	char *unmasked_data;
 	char *mask_data;
 	char *action_data;
 };
 
+struct nfp_fl_stats_frame {
+	__be32 stats_con_id;
+	__be32 pkt_count;
+	__be64 byte_count;
+	__be64 stats_cookie;
+};
+
 int nfp_flower_metadata_init(struct nfp_app *app);
 void nfp_flower_metadata_cleanup(struct nfp_app *app);
 
@@ -127,4 +154,6 @@ nfp_flower_find_in_fl_table(struct nfp_app *app,
 struct nfp_fl_payload *
 nfp_flower_remove_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie);
 
+void nfp_flower_rx_flow_stats(struct nfp_app *app, struct sk_buff *skb);
+
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/metadata.c b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
index 51de9ab85951..f03e45ab15af 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/metadata.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
@@ -55,6 +55,55 @@ struct nfp_flower_table {
 	struct rcu_head rcu;
 };
 
+static int nfp_release_stats_entry(struct nfp_app *app, u32 stats_context_id)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	struct circ_buf *ring;
+
+	ring = &priv->stats_ids.free_list;
+	/* Check if buffer is full. */
+	if (!CIRC_SPACE(ring->head, ring->tail, NFP_FL_STATS_ENTRY_RS *
+			NFP_FL_STATS_ELEM_RS -
+			NFP_FL_STATS_ELEM_RS + 1))
+		return -ENOBUFS;
+
+	memcpy(&ring->buf[ring->head], &stats_context_id, NFP_FL_STATS_ELEM_RS);
+	ring->head = (ring->head + NFP_FL_STATS_ELEM_RS) %
+		     (NFP_FL_STATS_ENTRY_RS * NFP_FL_STATS_ELEM_RS);
+
+	return 0;
+}
+
+static int nfp_get_stats_entry(struct nfp_app *app, u32 *stats_context_id)
+{
+	struct nfp_flower_priv *priv = app->priv;
+	u32 freed_stats_id, temp_stats_id;
+	struct circ_buf *ring;
+
+	ring = &priv->stats_ids.free_list;
+	freed_stats_id = NFP_FL_STATS_ENTRY_RS;
+	/* Check for unallocated entries first. */
+	if (priv->stats_ids.init_unalloc > 0) {
+		*stats_context_id = priv->stats_ids.init_unalloc - 1;
+		priv->stats_ids.init_unalloc--;
+		return 0;
+	}
+
+	/* Check if buffer is empty. */
+	if (ring->head == ring->tail) {
+		*stats_context_id = freed_stats_id;
+		return -ENOENT;
+	}
+
+	memcpy(&temp_stats_id, &ring->buf[ring->tail], NFP_FL_STATS_ELEM_RS);
+	*stats_context_id = temp_stats_id;
+	memcpy(&ring->buf[ring->tail], &freed_stats_id, NFP_FL_STATS_ELEM_RS);
+	ring->tail = (ring->tail + NFP_FL_STATS_ELEM_RS) %
+		     (NFP_FL_STATS_ENTRY_RS * NFP_FL_STATS_ELEM_RS);
+
+	return 0;
+}
+
 static struct nfp_flower_table *
 nfp_flower_search_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie)
 {
@@ -69,6 +118,46 @@ nfp_flower_search_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie)
 	return NULL;
 }
 
+static void
+nfp_flower_update_stats(struct nfp_app *app, struct nfp_fl_stats_frame *stats)
+{
+	struct nfp_fl_payload *nfp_flow;
+	unsigned long flower_cookie;
+
+	flower_cookie = be64_to_cpu(stats->stats_cookie);
+
+	rcu_read_lock();
+	nfp_flow = nfp_flower_find_in_fl_table(app, flower_cookie);
+	if (!nfp_flow)
+		goto exit_rcu_unlock;
+
+	if (nfp_flow->meta.host_ctx_id != stats->stats_con_id)
+		goto exit_rcu_unlock;
+
+	spin_lock(&nfp_flow->lock);
+	nfp_flow->stats.pkts += be32_to_cpu(stats->pkt_count);
+	nfp_flow->stats.bytes += be64_to_cpu(stats->byte_count);
+	nfp_flow->stats.used = jiffies;
+	spin_unlock(&nfp_flow->lock);
+
+exit_rcu_unlock:
+	rcu_read_unlock();
+}
+
+void nfp_flower_rx_flow_stats(struct nfp_app *app, struct sk_buff *skb)
+{
+	unsigned int msg_len = skb->len - NFP_FLOWER_CMSG_HLEN;
+	struct nfp_fl_stats_frame *stats_frame;
+	unsigned char *msg;
+	int i;
+
+	msg = nfp_flower_cmsg_get_data(skb);
+
+	stats_frame = (struct nfp_fl_stats_frame *)msg;
+	for (i = 0; i < msg_len / sizeof(*stats_frame); i++)
+		nfp_flower_update_stats(app, stats_frame + i);
+}
+
 static int
 nfp_flower_add_fl_table(struct nfp_app *app, unsigned long tc_flower_cookie,
 			struct nfp_fl_payload *nfp_flow)
@@ -291,22 +380,39 @@ int nfp_compile_flow_metadata(struct nfp_app *app,
 {
 	struct nfp_flower_priv *priv = app->priv;
 	u8 new_mask_id;
+	u32 stats_cxt;
 	int err;
 
+	err = nfp_get_stats_entry(app, &stats_cxt);
+	if (err < 0)
+		return err;
+
+	nfp_flow->meta.host_ctx_id = cpu_to_be32(stats_cxt);
+	nfp_flow->meta.host_cookie = cpu_to_be64(flow->cookie);
+
 	new_mask_id = 0;
 	if (!nfp_check_mask_add(app, nfp_flow->mask_data,
 				nfp_flow->meta.mask_len,
-				&nfp_flow->meta.flags, &new_mask_id))
+				&nfp_flow->meta.flags, &new_mask_id)) {
+		if (nfp_release_stats_entry(app, stats_cxt))
+			return -EINVAL;
 		return -ENOENT;
+	}
 
 	nfp_flow->meta.flow_version = cpu_to_be64(priv->flower_version);
 	priv->flower_version++;
 
 	/* Update flow payload with mask ids. */
 	nfp_flow->unmasked_data[NFP_FL_MASK_ID_LOCATION] = new_mask_id;
+	nfp_flow->stats.pkts = 0;
+	nfp_flow->stats.bytes = 0;
+	nfp_flow->stats.used = jiffies;
 
 	err = nfp_flower_add_fl_table(app, flow->cookie, nfp_flow);
 	if (err < 0) {
+		if (nfp_release_stats_entry(app, stats_cxt))
+			return -EINVAL;
+
 		if (!nfp_check_mask_remove(app, nfp_flow->mask_data,
 					   nfp_flow->meta.mask_len,
 					   NULL, &new_mask_id))
@@ -323,6 +429,7 @@ int nfp_modify_flow_metadata(struct nfp_app *app,
 {
 	struct nfp_flower_priv *priv = app->priv;
 	u8 new_mask_id = 0;
+	u32 temp_ctx_id;
 
 	nfp_check_mask_remove(app, nfp_flow->mask_data,
 			      nfp_flow->meta.mask_len, &nfp_flow->meta.flags,
@@ -334,7 +441,10 @@ int nfp_modify_flow_metadata(struct nfp_app *app,
 	/* Update flow payload with mask ids. */
 	nfp_flow->unmasked_data[NFP_FL_MASK_ID_LOCATION] = new_mask_id;
 
-	return 0;
+	/* Release the stats ctx id. */
+	temp_ctx_id = be32_to_cpu(nfp_flow->meta.host_ctx_id);
+
+	return nfp_release_stats_entry(app, temp_ctx_id);
 }
 
 int nfp_flower_metadata_init(struct nfp_app *app)
@@ -362,6 +472,15 @@ int nfp_flower_metadata_init(struct nfp_app *app)
 		return -ENOMEM;
 	}
 
+	/* Init ring buffer and unallocated stats_ids. */
+	priv->stats_ids.free_list.buf =
+		vmalloc(NFP_FL_STATS_ENTRY_RS * NFP_FL_STATS_ELEM_RS);
+	if (!priv->stats_ids.free_list.buf) {
+		vfree(priv->mask_ids.mask_id_free_list.buf);
+		return -ENOMEM;
+	}
+	priv->stats_ids.init_unalloc = NFP_FL_REPEATED_HASH_MAX;
+
 	return 0;
 }
 
@@ -374,4 +493,5 @@ void nfp_flower_metadata_cleanup(struct nfp_app *app)
 
 	kfree(priv->mask_ids.mask_id_free_list.buf);
 	kfree(priv->mask_ids.last_used);
+	vfree(priv->stats_ids.free_list.buf);
 }
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index d0c0350efef1..79df9d74f89e 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -170,6 +170,7 @@ nfp_flower_allocate_new(struct nfp_fl_key_ls *key_layer)
 		goto err_free_mask;
 
 	flow_pay->meta.flags = 0;
+	spin_lock_init(&flow_pay->lock);
 
 	return flow_pay;
 
@@ -270,7 +271,10 @@ nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
 		return -ENOENT;
 
 	err = nfp_modify_flow_metadata(app, nfp_flow);
+	if (err)
+		goto err_free_flow;
 
+err_free_flow:
 	nfp_flower_deallocate_nfp(nfp_flow);
 	return err;
 }
@@ -288,7 +292,21 @@ nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
 static int
 nfp_flower_get_stats(struct nfp_app *app, struct tc_cls_flower_offload *flow)
 {
-	return -EOPNOTSUPP;
+	struct nfp_fl_payload *nfp_flow;
+
+	nfp_flow = nfp_flower_find_in_fl_table(app, flow->cookie);
+	if (!nfp_flow)
+		return -EINVAL;
+
+	spin_lock(&nfp_flow->lock);
+	tcf_exts_stats_update(flow->exts, nfp_flow->stats.bytes,
+			      nfp_flow->stats.pkts, nfp_flow->stats.used);
+
+	nfp_flow->stats.pkts = 0;
+	nfp_flow->stats.bytes = 0;
+	spin_unlock(&nfp_flow->lock);
+
+	return 0;
 }
 
 static int
-- 
2.1.4

Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Jakub Kicinski]

On Wed, 28 Jun 2017 22:30:01 +0200, Simon Horman wrote:
> @@ -288,7 +292,21 @@ nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
>  static int
>  nfp_flower_get_stats(struct nfp_app *app, struct tc_cls_flower_offload *flow)
>  {
> -	return -EOPNOTSUPP;
> +	struct nfp_fl_payload *nfp_flow;
> +
> +	nfp_flow = nfp_flower_find_in_fl_table(app, flow->cookie);
> +	if (!nfp_flow)
> +		return -EINVAL;
> +
> +	spin_lock(&nfp_flow->lock);
> +	tcf_exts_stats_update(flow->exts, nfp_flow->stats.bytes,
> +			      nfp_flow->stats.pkts, nfp_flow->stats.used);
> +
> +	nfp_flow->stats.pkts = 0;
> +	nfp_flow->stats.bytes = 0;
> +	spin_unlock(&nfp_flow->lock);
> +
> +	return 0;
>  }

This needs to take spin_lock_bh() to lock out the RX path safely :(

Re: [oss-drivers] Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Simon Horman]

On Wed, Jun 28, 2017 at 06:28:17PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2017 22:30:01 +0200, Simon Horman wrote:
> > @@ -288,7 +292,21 @@ nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
> >  static int
> >  nfp_flower_get_stats(struct nfp_app *app, struct tc_cls_flower_offload *flow)
> >  {
> > -	return -EOPNOTSUPP;
> > +	struct nfp_fl_payload *nfp_flow;
> > +
> > +	nfp_flow = nfp_flower_find_in_fl_table(app, flow->cookie);
> > +	if (!nfp_flow)
> > +		return -EINVAL;
> > +
> > +	spin_lock(&nfp_flow->lock);
> > +	tcf_exts_stats_update(flow->exts, nfp_flow->stats.bytes,
> > +			      nfp_flow->stats.pkts, nfp_flow->stats.used);
> > +
> > +	nfp_flow->stats.pkts = 0;
> > +	nfp_flow->stats.bytes = 0;
> > +	spin_unlock(&nfp_flow->lock);
> > +
> > +	return 0;
> >  }
> 
> This needs to take spin_lock_bh() to lock out the RX path safely :(

Sorry about that, will fix.

Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Jakub Kicinski]

On Wed, 28 Jun 2017 22:30:01 +0200, Simon Horman wrote:
> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> 
> Previously there was no way of updating flow rule stats after they
> have been offloaded to hardware. This is solved by keeping track of
> stats received from hardware and providing this to the TC handler
> on request.
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

> @@ -334,7 +441,10 @@ int nfp_modify_flow_metadata(struct nfp_app *app,
>  	/* Update flow payload with mask ids. */
>  	nfp_flow->unmasked_data[NFP_FL_MASK_ID_LOCATION] = new_mask_id;
>  
> -	return 0;
> +	/* Release the stats ctx id. */
> +	temp_ctx_id = be32_to_cpu(nfp_flow->meta.host_ctx_id);
> +
> +	return nfp_release_stats_entry(app, temp_ctx_id);
>  }
>  
>  int nfp_flower_metadata_init(struct nfp_app *app)
> @@ -362,6 +472,15 @@ int nfp_flower_metadata_init(struct nfp_app *app)
>  		return -ENOMEM;
>  	}
>  
> +	/* Init ring buffer and unallocated stats_ids. */
> +	priv->stats_ids.free_list.buf =
> +		vmalloc(NFP_FL_STATS_ENTRY_RS * NFP_FL_STATS_ELEM_RS);
> +	if (!priv->stats_ids.free_list.buf) {
> +		vfree(priv->mask_ids.mask_id_free_list.buf);
> +		return -ENOMEM;

This is hiding a leak, I think.  There were 2 things allocate above.
Please add a proper unwind path with goto's - it makes catching bugs
like this much easier.

> +	}
> +	priv->stats_ids.init_unalloc = NFP_FL_REPEATED_HASH_MAX;
> +
>  	return 0;
>  }
>  

Re: [oss-drivers] Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Simon Horman]

On Wed, Jun 28, 2017 at 07:55:18PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2017 22:30:01 +0200, Simon Horman wrote:
> > From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > 
> > Previously there was no way of updating flow rule stats after they
> > have been offloaded to hardware. This is solved by keeping track of
> > stats received from hardware and providing this to the TC handler
> > on request.
> > 
> > Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> > @@ -334,7 +441,10 @@ int nfp_modify_flow_metadata(struct nfp_app *app,
> >  	/* Update flow payload with mask ids. */
> >  	nfp_flow->unmasked_data[NFP_FL_MASK_ID_LOCATION] = new_mask_id;
> >  
> > -	return 0;
> > +	/* Release the stats ctx id. */
> > +	temp_ctx_id = be32_to_cpu(nfp_flow->meta.host_ctx_id);
> > +
> > +	return nfp_release_stats_entry(app, temp_ctx_id);
> >  }
> >  
> >  int nfp_flower_metadata_init(struct nfp_app *app)
> > @@ -362,6 +472,15 @@ int nfp_flower_metadata_init(struct nfp_app *app)
> >  		return -ENOMEM;
> >  	}
> >  
> > +	/* Init ring buffer and unallocated stats_ids. */
> > +	priv->stats_ids.free_list.buf =
> > +		vmalloc(NFP_FL_STATS_ENTRY_RS * NFP_FL_STATS_ELEM_RS);
> > +	if (!priv->stats_ids.free_list.buf) {
> > +		vfree(priv->mask_ids.mask_id_free_list.buf);
> > +		return -ENOMEM;
> 
> This is hiding a leak, I think.  There were 2 things allocate above.
> Please add a proper unwind path with goto's - it makes catching bugs
> like this much easier.

Sure, will do.

Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
>
> Previously there was no way of updating flow rule stats after they
> have been offloaded to hardware. This is solved by keeping track of
> stats received from hardware and providing this to the TC handler
> on request.


You are using the term/variable "unmasked_data" across the code, to
make it clear
the keys provided to the driver are masked by the tcflower kernel code
[1] - what do
you mean by unmasked data?

Or.

[1] f93bd17 net/sched: cls_flower: Use masked key when calling HW offloads

Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Simon Horman]

On Thu, Jun 29, 2017 at 05:39:34PM +0300, Or Gerlitz wrote:
> On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> >
> > Previously there was no way of updating flow rule stats after they
> > have been offloaded to hardware. This is solved by keeping track of
> > stats received from hardware and providing this to the TC handler
> > on request.
> 
> 
> You are using the term/variable "unmasked_data" across the code, to
> make it clear
> the keys provided to the driver are masked by the tcflower kernel code
> [1] - what do
> you mean by unmasked data?
> 
> Or.
> 
> [1] f93bd17 net/sched: cls_flower: Use masked key when calling HW offloads

>From a firmware point of view there is an unmasked key and a mask.
These are referred to as unmasked_data and mask_data although in
TC supplies a masked key which the firmware can also handle.

Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
>
> Previously there was no way of updating flow rule stats after they
> have been offloaded to hardware. This is solved by keeping track of
> stats received from hardware and providing this to the TC handler
> on request.

> +static void
> +nfp_flower_update_stats(struct nfp_app *app, struct nfp_fl_stats_frame *stats)
> +{
> +       struct nfp_fl_payload *nfp_flow;
> +       unsigned long flower_cookie;
> +
> +       flower_cookie = be64_to_cpu(stats->stats_cookie);
> +
> +       rcu_read_lock();
> +       nfp_flow = nfp_flower_find_in_fl_table(app, flower_cookie);
> +       if (!nfp_flow)
> +               goto exit_rcu_unlock;
> +
> +       if (nfp_flow->meta.host_ctx_id != stats->stats_con_id)
> +               goto exit_rcu_unlock;
> +
> +       spin_lock(&nfp_flow->lock);
> +       nfp_flow->stats.pkts += be32_to_cpu(stats->pkt_count);
> +       nfp_flow->stats.bytes += be64_to_cpu(stats->byte_count);

you're using += with the values you get form the fw, are they incremental?

> +       nfp_flow->stats.used = jiffies;

if nothing was changed since your last reading, it's wrong to say that
used == NOW

> +       spin_unlock(&nfp_flow->lock);

if indeed you need to keep a clone of earlier calls to correctly
compute the (last)used value,
maybe you can get rid of the locking?

Re: [PATCH net-next v2 8/9] nfp: add a stats handler for flower offloads

[Simon Horman]

On Thu, Jun 29, 2017 at 06:16:41PM +0300, Or Gerlitz wrote:
> On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> >
> > Previously there was no way of updating flow rule stats after they
> > have been offloaded to hardware. This is solved by keeping track of
> > stats received from hardware and providing this to the TC handler
> > on request.
> 
> > +static void
> > +nfp_flower_update_stats(struct nfp_app *app, struct nfp_fl_stats_frame *stats)
> > +{
> > +       struct nfp_fl_payload *nfp_flow;
> > +       unsigned long flower_cookie;
> > +
> > +       flower_cookie = be64_to_cpu(stats->stats_cookie);
> > +
> > +       rcu_read_lock();
> > +       nfp_flow = nfp_flower_find_in_fl_table(app, flower_cookie);
> > +       if (!nfp_flow)
> > +               goto exit_rcu_unlock;
> > +
> > +       if (nfp_flow->meta.host_ctx_id != stats->stats_con_id)
> > +               goto exit_rcu_unlock;
> > +
> > +       spin_lock(&nfp_flow->lock);
> > +       nfp_flow->stats.pkts += be32_to_cpu(stats->pkt_count);
> > +       nfp_flow->stats.bytes += be64_to_cpu(stats->byte_count);
> 
> you're using += with the values you get form the fw, are they incremental?

Yes, they are incremental.

> 
> > +       nfp_flow->stats.used = jiffies;
> 
> if nothing was changed since your last reading, it's wrong to say that
> used == NOW

This function is called on receipt of a message from the firmware.
And the firmware will only send a message if there is a change in
the counters.

> 
> > +       spin_unlock(&nfp_flow->lock);
> 
> if indeed you need to keep a clone of earlier calls to correctly
> compute the (last)used value,
> maybe you can get rid of the locking?

[PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[Simon Horman]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Previously the flower offloads never sends messages to the hardware,
and never registers a handler for receiving messages from hardware.
This patch enables the flower offloads to send control messages to
hardware when adding and removing flow rules. Additionally it
registers a control message rx handler for receiving stats updates
from hardware for each offloaded flow.

Additionally this patch adds 4 control message types; Add, modify and
delete flow, as well as flow stats. It also allows
nfp_flower_cmsg_get_data() to be used outside of cmsg.c.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/cmsg.c   |  6 ++-
 drivers/net/ethernet/netronome/nfp/flower/cmsg.h   |  7 +++
 .../net/ethernet/netronome/nfp/flower/offload.c    | 56 ++++++++++++++++++++++
 3 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.c b/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
index 0f5410aa66d6..dd7fa9cf225f 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
@@ -36,6 +36,7 @@
 #include <linux/skbuff.h>
 #include <net/dst_metadata.h>
 
+#include "main.h"
 #include "../nfpcore/nfp_cpp.h"
 #include "../nfp_net_repr.h"
 #include "./cmsg.h"
@@ -52,7 +53,7 @@ nfp_flower_cmsg_get_hdr(struct sk_buff *skb)
 	return (struct nfp_flower_cmsg_hdr *)skb->data;
 }
 
-static struct sk_buff *
+struct sk_buff *
 nfp_flower_cmsg_alloc(struct nfp_app *app, unsigned int size,
 		      enum nfp_flower_cmsg_type_port type)
 {
@@ -143,6 +144,9 @@ void nfp_flower_cmsg_rx(struct nfp_app *app, struct sk_buff *skb)
 	case NFP_FLOWER_CMSG_TYPE_PORT_MOD:
 		nfp_flower_cmsg_portmod_rx(app, skb);
 		break;
+	case NFP_FLOWER_CMSG_TYPE_FLOW_STATS:
+		nfp_flower_rx_flow_stats(app, skb);
+		break;
 	default:
 		nfp_flower_cmsg_warn(app, "Cannot handle invalid repr control type %u\n",
 				     type);
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 736c4848f073..5a997feb6f80 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -245,7 +245,11 @@ struct nfp_flower_cmsg_hdr {
 
 /* Types defined for port related control messages  */
 enum nfp_flower_cmsg_type_port {
+	NFP_FLOWER_CMSG_TYPE_FLOW_ADD =		0,
+	NFP_FLOWER_CMSG_TYPE_FLOW_MOD =		1,
+	NFP_FLOWER_CMSG_TYPE_FLOW_DEL =		2,
 	NFP_FLOWER_CMSG_TYPE_PORT_MOD =		8,
+	NFP_FLOWER_CMSG_TYPE_FLOW_STATS =	15,
 	NFP_FLOWER_CMSG_TYPE_PORT_ECHO =	16,
 	NFP_FLOWER_CMSG_TYPE_MAX =		32,
 };
@@ -307,5 +311,8 @@ static inline void *nfp_flower_cmsg_get_data(struct sk_buff *skb)
 
 int nfp_flower_cmsg_portmod(struct nfp_repr *repr, bool carrier_ok);
 void nfp_flower_cmsg_rx(struct nfp_app *app, struct sk_buff *skb);
+struct sk_buff *
+nfp_flower_cmsg_alloc(struct nfp_app *app, unsigned int size,
+		      enum nfp_flower_cmsg_type_port type);
 
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 79df9d74f89e..73b3467f5bcd 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -44,6 +44,52 @@
 #include "../nfp_net.h"
 #include "../nfp_port.h"
 
+static int
+nfp_flower_xmit_flow(struct net_device *netdev,
+		     struct nfp_fl_payload *nfp_flow, u8 mtype)
+{
+	u32 meta_len, key_len, mask_len, act_len, tot_len;
+	struct nfp_repr *priv = netdev_priv(netdev);
+	struct sk_buff *skb;
+	unsigned char *msg;
+
+	meta_len =  sizeof(struct nfp_fl_rule_metadata);
+	key_len = nfp_flow->meta.key_len;
+	mask_len = nfp_flow->meta.mask_len;
+	act_len = nfp_flow->meta.act_len;
+
+	tot_len = meta_len + key_len + mask_len + act_len;
+
+	/* Convert to long words as firmware expects
+	 * lengths in units of NFP_FL_LW_SIZ.
+	 */
+	nfp_flow->meta.key_len /= NFP_FL_LW_SIZ;
+	nfp_flow->meta.mask_len /= NFP_FL_LW_SIZ;
+	nfp_flow->meta.act_len /= NFP_FL_LW_SIZ;
+
+	skb = nfp_flower_cmsg_alloc(priv->app, tot_len, mtype);
+	if (!skb)
+		return -ENOMEM;
+
+	msg = nfp_flower_cmsg_get_data(skb);
+	memcpy(msg, &nfp_flow->meta, meta_len);
+	memcpy(&msg[meta_len], nfp_flow->unmasked_data, key_len);
+	memcpy(&msg[meta_len + key_len], nfp_flow->mask_data, mask_len);
+	memcpy(&msg[meta_len + key_len + mask_len],
+	       nfp_flow->action_data, act_len);
+
+	/* Convert back to bytes as software expects
+	 * lengths in units of bytes.
+	 */
+	nfp_flow->meta.key_len *= NFP_FL_LW_SIZ;
+	nfp_flow->meta.mask_len *= NFP_FL_LW_SIZ;
+	nfp_flow->meta.act_len *= NFP_FL_LW_SIZ;
+
+	nfp_ctrl_tx(priv->app->ctrl, skb);
+
+	return 0;
+}
+
 static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
 {
 	return dissector_uses_key(f->dissector,
@@ -235,6 +281,11 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 	if (err)
 		goto err_destroy_flow;
 
+	err = nfp_flower_xmit_flow(netdev, flow_pay,
+				   NFP_FLOWER_CMSG_TYPE_FLOW_ADD);
+	if (err)
+		goto err_destroy_flow;
+
 	/* Deallocate flow payload when flower rule has been destroyed. */
 	kfree(key_layer);
 
@@ -274,6 +325,11 @@ nfp_flower_del_offload(struct nfp_app *app, struct net_device *netdev,
 	if (err)
 		goto err_free_flow;
 
+	err = nfp_flower_xmit_flow(netdev, nfp_flow,
+				   NFP_FLOWER_CMSG_TYPE_FLOW_DEL);
+	if (err)
+		goto err_free_flow;
+
 err_free_flow:
 	nfp_flower_deallocate_nfp(nfp_flow);
 	return err;
-- 
2.1.4

Re: [PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

> +nfp_flower_xmit_flow(struct net_device *netdev,
> +                    struct nfp_fl_payload *nfp_flow, u8 mtype)
> +{
> +       u32 meta_len, key_len, mask_len, act_len, tot_len;
> +       struct nfp_repr *priv = netdev_priv(netdev);
> +       struct sk_buff *skb;
> +       unsigned char *msg;
> +
> +       meta_len =  sizeof(struct nfp_fl_rule_metadata);
> +       key_len = nfp_flow->meta.key_len;
> +       mask_len = nfp_flow->meta.mask_len;
> +       act_len = nfp_flow->meta.act_len;
> +
> +       tot_len = meta_len + key_len + mask_len + act_len;
> +
> +       /* Convert to long words as firmware expects
> +        * lengths in units of NFP_FL_LW_SIZ.
> +        */
> +       nfp_flow->meta.key_len /= NFP_FL_LW_SIZ;
> +       nfp_flow->meta.mask_len /= NFP_FL_LW_SIZ;
> +       nfp_flow->meta.act_len /= NFP_FL_LW_SIZ;

better to use use x >>= 2 instead x /= 4

I saw it in other places across the patch/set

RE: [PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[David Laight]

From: Or Gerlitz
> Sent: 29 June 2017 14:47
...
> > +       nfp_flow->meta.key_len /= NFP_FL_LW_SIZ;
> > +       nfp_flow->meta.mask_len /= NFP_FL_LW_SIZ;
> > +       nfp_flow->meta.act_len /= NFP_FL_LW_SIZ;
> 
> better to use use x >>= 2 instead x /= 4
> 
> I saw it in other places across the patch/set

Provided the values are unsigned it makes no difference.
Having 2 matching constants just makes the code harder to read.

	David

Re: [PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[Or Gerlitz]

On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> +++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
> @@ -245,7 +245,11 @@ struct nfp_flower_cmsg_hdr {
>
>  /* Types defined for port related control messages  */
>  enum nfp_flower_cmsg_type_port {
> +       NFP_FLOWER_CMSG_TYPE_FLOW_ADD =         0,
> +       NFP_FLOWER_CMSG_TYPE_FLOW_MOD =         1,

AFAIU flower offload will always use delete and add and not modify,
if you think otherwise, would love to hear that. Didn't see you are using
the MOD define in the code anywhere.

Re: [PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[Simon Horman]

On Thu, Jun 29, 2017 at 06:21:53PM +0300, Or Gerlitz wrote:
> On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
> > @@ -245,7 +245,11 @@ struct nfp_flower_cmsg_hdr {
> >
> >  /* Types defined for port related control messages  */
> >  enum nfp_flower_cmsg_type_port {
> > +       NFP_FLOWER_CMSG_TYPE_FLOW_ADD =         0,
> > +       NFP_FLOWER_CMSG_TYPE_FLOW_MOD =         1,
> 
> AFAIU flower offload will always use delete and add and not modify,
> if you think otherwise, would love to hear that. Didn't see you are using
> the MOD define in the code anywhere.

The firmware API has a mod operation but you are correct that
we are not implementing that operation in this patch-set.

Re: [PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[Or Gerlitz]

On Thu, Jun 29, 2017 at 6:30 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> On Thu, Jun 29, 2017 at 06:21:53PM +0300, Or Gerlitz wrote:
>> On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
>> <simon.horman@netronome.com> wrote:
>> > +++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
>> > @@ -245,7 +245,11 @@ struct nfp_flower_cmsg_hdr {
>> >
>> >  /* Types defined for port related control messages  */
>> >  enum nfp_flower_cmsg_type_port {
>> > +       NFP_FLOWER_CMSG_TYPE_FLOW_ADD =         0,
>> > +       NFP_FLOWER_CMSG_TYPE_FLOW_MOD =         1,
>>
>> AFAIU flower offload will always use delete and add and not modify,
>> if you think otherwise, would love to hear that. Didn't see you are using
>> the MOD define in the code anywhere.
>
> The firmware API has a mod operation but you are correct that
> we are not implementing that operation in this patch-set.

that wasn't my comment. I said that AFAIU flower will never ask you to
modify, they
will ask to delete and later add, so I would be happy to know if you
see it differently.

Re: [PATCH net-next v2 9/9] nfp: add control message passing capabilities to flower offloads

[Simon Horman]

On Thu, Jun 29, 2017 at 06:59:22PM +0300, Or Gerlitz wrote:
> On Thu, Jun 29, 2017 at 6:30 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > On Thu, Jun 29, 2017 at 06:21:53PM +0300, Or Gerlitz wrote:
> >> On Wed, Jun 28, 2017 at 11:30 PM, Simon Horman
> >> <simon.horman@netronome.com> wrote:
> >> > +++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
> >> > @@ -245,7 +245,11 @@ struct nfp_flower_cmsg_hdr {
> >> >
> >> >  /* Types defined for port related control messages  */
> >> >  enum nfp_flower_cmsg_type_port {
> >> > +       NFP_FLOWER_CMSG_TYPE_FLOW_ADD =         0,
> >> > +       NFP_FLOWER_CMSG_TYPE_FLOW_MOD =         1,
> >>
> >> AFAIU flower offload will always use delete and add and not modify,
> >> if you think otherwise, would love to hear that. Didn't see you are using
> >> the MOD define in the code anywhere.
> >
> > The firmware API has a mod operation but you are correct that
> > we are not implementing that operation in this patch-set.
> 
> that wasn't my comment. I said that AFAIU flower will never ask you to
> modify, they
> will ask to delete and later add, so I would be happy to know if you
> see it differently.

Yes, I agree.

Sorry for not answering your question properly the first time.