Re: [PATCH net-next v2 1/4] net: dsa: Add plumbing for port mirroring

[Florian Fainelli]

Le 01/28/17 à 01:14, Jiri Pirko a écrit :
> Sat, Jan 28, 2017 at 02:25:25AM CET, f.faine...@gmail.com wrote:
>> Add necessary plumbing at the slave network device level to have switch
>> drivers implement ndo_setup_tc() and most particularly the cls_matchall
>> classifier. We add support for two switch operations:
>>
>> port_add_mirror and port_del_mirror() which configure, on a per-port
>> basis the mirror parameters requested from the cls_matchall classifier.
>>
>> Code is largely borrowed from the Mellanox Spectrum switch driver.
>>
>> Signed-off-by: Florian Fainelli 
>> ---
> 
> [...]
> 
> 
>> +/*
>> + * Mirroring TC entry
>> + */
>> +struct dsa_mall_mirror_tc_entry {
>> +u8 to_local_port;
>> +bool ingress;
>> +};
>> +
>> +/*
>> + * TC matchall entry
>> + */
> 
> Why are you using multiline comment format for single line comments?

There are precedents in that file, but I will remove it.

> 
> 
>> +struct dsa_mall_tc_entry {
>> +struct list_head list;
>> +unsigned long cookie;
>> +enum dsa_port_mall_action_type type;
>> +union {
>> +struct dsa_mall_mirror_tc_entry mirror;
>> +};
>> +};
>> +
>> +
>> struct dsa_port {
>>  struct net_device   *netdev;
>>  struct device_node  *dn;
>> @@ -370,6 +397,15 @@ struct dsa_switch_ops {
>>  int (*port_mdb_dump)(struct dsa_switch *ds, int port,
>>   struct switchdev_obj_port_mdb *mdb,
>>   int (*cb)(struct switchdev_obj *obj));
>> +
>> +/*
>> + * TC integration
>> + */
>> +int (*port_mirror_add)(struct dsa_switch *ds, int port,
>> +   struct dsa_mall_mirror_tc_entry *mirror,
>> +   bool ingress);
>> +void(*port_mirror_del)(struct dsa_switch *ds, int port,
>> +   struct dsa_mall_mirror_tc_entry *mirror);
>> };
> 
> [...]
> 
> 
>> +static int dsa_slave_add_cls_matchall(struct net_device *dev,
>> +  __be16 protocol,
>> +  struct tc_cls_matchall_offload *cls,
>> +  bool ingress)
>> +{
>> +struct dsa_slave_priv *p = netdev_priv(dev);
>> +struct dsa_mall_tc_entry *mall_tc_entry;
>> +struct dsa_switch *ds = p->parent;
>> +struct net *net = dev_net(dev);
>> +struct dsa_slave_priv *to_p;
>> +struct net_device *to_dev;
>> +const struct tc_action *a;
>> +int err = -EOPNOTSUPP;
>> +LIST_HEAD(actions);
>> +int ifindex;
>> +
>> +if (!ds->ops->port_mirror_add)
>> +return err;
>> +
>> +if (!tc_single_action(cls->exts)) {
>> +netdev_err(dev, "only singular actions are supported\n");
> 
> Why you note the user in this case, but in case he tries to add
> non-supported action you don't note him?

Will remove that message.

> 
> 
>> +return err;
>> +}
>> +
>> +mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
>> +if (!mall_tc_entry)
>> +return -ENOMEM;
>> +mall_tc_entry->cookie = cls->cookie;
> 
> Hmm, I believe that this allocation and initialization should go into
> the "is_mirred if". You can do the checks in advance. That would also
> make the error path simplier.

Yes good point, seems like you may want to do the same in mlxsw since
that part of the code was loosely based on that too.

Thanks Jiri!
-- 
Florian

Re: [PATCH net-next v2 1/4] net: dsa: Add plumbing for port mirroring

[Jiri Pirko]

Sat, Jan 28, 2017 at 02:25:25AM CET, f.faine...@gmail.com wrote:
>Add necessary plumbing at the slave network device level to have switch
>drivers implement ndo_setup_tc() and most particularly the cls_matchall
>classifier. We add support for two switch operations:
>
>port_add_mirror and port_del_mirror() which configure, on a per-port
>basis the mirror parameters requested from the cls_matchall classifier.
>
>Code is largely borrowed from the Mellanox Spectrum switch driver.
>
>Signed-off-by: Florian Fainelli 
>---

[...]


>+/*
>+ * Mirroring TC entry
>+ */
>+struct dsa_mall_mirror_tc_entry {
>+  u8 to_local_port;
>+  bool ingress;
>+};
>+
>+/*
>+ * TC matchall entry
>+ */

Why are you using multiline comment format for single line comments?


>+struct dsa_mall_tc_entry {
>+  struct list_head list;
>+  unsigned long cookie;
>+  enum dsa_port_mall_action_type type;
>+  union {
>+  struct dsa_mall_mirror_tc_entry mirror;
>+  };
>+};
>+
>+
> struct dsa_port {
>   struct net_device   *netdev;
>   struct device_node  *dn;
>@@ -370,6 +397,15 @@ struct dsa_switch_ops {
>   int (*port_mdb_dump)(struct dsa_switch *ds, int port,
>struct switchdev_obj_port_mdb *mdb,
>int (*cb)(struct switchdev_obj *obj));
>+
>+  /*
>+   * TC integration
>+   */
>+  int (*port_mirror_add)(struct dsa_switch *ds, int port,
>+ struct dsa_mall_mirror_tc_entry *mirror,
>+ bool ingress);
>+  void(*port_mirror_del)(struct dsa_switch *ds, int port,
>+ struct dsa_mall_mirror_tc_entry *mirror);
> };

[...]


>+static int dsa_slave_add_cls_matchall(struct net_device *dev,
>+__be16 protocol,
>+struct tc_cls_matchall_offload *cls,
>+bool ingress)
>+{
>+  struct dsa_slave_priv *p = netdev_priv(dev);
>+  struct dsa_mall_tc_entry *mall_tc_entry;
>+  struct dsa_switch *ds = p->parent;
>+  struct net *net = dev_net(dev);
>+  struct dsa_slave_priv *to_p;
>+  struct net_device *to_dev;
>+  const struct tc_action *a;
>+  int err = -EOPNOTSUPP;
>+  LIST_HEAD(actions);
>+  int ifindex;
>+
>+  if (!ds->ops->port_mirror_add)
>+  return err;
>+
>+  if (!tc_single_action(cls->exts)) {
>+  netdev_err(dev, "only singular actions are supported\n");

Why you note the user in this case, but in case he tries to add
non-supported action you don't note him?


>+  return err;
>+  }
>+
>+  mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
>+  if (!mall_tc_entry)
>+  return -ENOMEM;
>+  mall_tc_entry->cookie = cls->cookie;

Hmm, I believe that this allocation and initialization should go into
the "is_mirred if". You can do the checks in advance. That would also
make the error path simplier.


>+
>+  tcf_exts_to_list(cls->exts, );
>+  a = list_first_entry(, struct tc_action, list);
>+
>+  if (is_tcf_mirred_egress_mirror(a) && protocol == htons(ETH_P_ALL)) {
>+  struct dsa_mall_mirror_tc_entry *mirror;
>+
>+  mall_tc_entry->type = DSA_PORT_MALL_MIRROR;
>+  mirror = _tc_entry->mirror;
>+
>+  ifindex = tcf_mirred_ifindex(a);
>+  to_dev = __dev_get_by_index(net, ifindex);
>+  if (!to_dev) {
>+  err = -EINVAL;
>+  goto err_add_action;
>+  }
>+
>+  if (!dsa_slave_dev_check(to_dev)) {
>+  err = -EOPNOTSUPP;
>+  goto err_add_action;
>+  }
>+
>+  to_p = netdev_priv(to_dev);
>+
>+  mirror->to_local_port = to_p->port;
>+  mirror->ingress = ingress;
>+
>+  err = ds->ops->port_mirror_add(ds, p->port, mirror, ingress);
>+  }
>+
>+  if (err)
>+  goto err_add_action;
>+
>+  list_add_tail(_tc_entry->list, >mall_tc_list);
>+  return 0;
>+
>+err_add_action:
>+  kfree(mall_tc_entry);
>+  return err;
>+}

[PATCH net-next v2 1/4] net: dsa: Add plumbing for port mirroring

[Florian Fainelli]

Add necessary plumbing at the slave network device level to have switch
drivers implement ndo_setup_tc() and most particularly the cls_matchall
classifier. We add support for two switch operations:

port_add_mirror and port_del_mirror() which configure, on a per-port
basis the mirror parameters requested from the cls_matchall classifier.

Code is largely borrowed from the Mellanox Spectrum switch driver.

Signed-off-by: Florian Fainelli 
---
 include/net/dsa.h  |  36 ++
 net/dsa/dsa_priv.h |   3 ++
 net/dsa/slave.c| 143 -
 3 files changed, 181 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 92fd795e9573..831a789594f1 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -20,6 +20,8 @@
 #include 
 #include 
 
+struct tc_action;
+
 enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = 0,
DSA_TAG_PROTO_DSA,
@@ -139,6 +141,31 @@ struct dsa_switch_tree {
const struct dsa_device_ops *tag_ops;
 };
 
+enum dsa_port_mall_action_type {
+   DSA_PORT_MALL_MIRROR,
+};
+
+/*
+ * Mirroring TC entry
+ */
+struct dsa_mall_mirror_tc_entry {
+   u8 to_local_port;
+   bool ingress;
+};
+
+/*
+ * TC matchall entry
+ */
+struct dsa_mall_tc_entry {
+   struct list_head list;
+   unsigned long cookie;
+   enum dsa_port_mall_action_type type;
+   union {
+   struct dsa_mall_mirror_tc_entry mirror;
+   };
+};
+
+
 struct dsa_port {
struct net_device   *netdev;
struct device_node  *dn;
@@ -370,6 +397,15 @@ struct dsa_switch_ops {
int (*port_mdb_dump)(struct dsa_switch *ds, int port,
 struct switchdev_obj_port_mdb *mdb,
 int (*cb)(struct switchdev_obj *obj));
+
+   /*
+* TC integration
+*/
+   int (*port_mirror_add)(struct dsa_switch *ds, int port,
+  struct dsa_mall_mirror_tc_entry *mirror,
+  bool ingress);
+   void(*port_mirror_del)(struct dsa_switch *ds, int port,
+  struct dsa_mall_mirror_tc_entry *mirror);
 };
 
 struct dsa_switch_driver {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 16194a4bb2fe..b10b03028b24 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -46,6 +46,9 @@ struct dsa_slave_priv {
 #ifdef CONFIG_NET_POLL_CONTROLLER
struct netpoll  *netpoll;
 #endif
+
+   /* TC context */
+   struct list_headmall_tc_list;
 };
 
 /* dsa.c */
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b8e58689a9a1..a5f9f1ebca2e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -16,12 +16,17 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include "dsa_priv.h"
 
+static bool dsa_slave_dev_check(struct net_device *dev);
+
 /* slave mii_bus handling ***/
 static int dsa_slave_phy_read(struct mii_bus *bus, int addr, int reg)
 {
@@ -994,6 +999,139 @@ static int dsa_slave_get_phys_port_name(struct net_device 
*dev,
return 0;
 }
 
+static struct dsa_mall_tc_entry *
+dsa_slave_mall_tc_entry_find(struct dsa_slave_priv *p,
+unsigned long cookie)
+{
+   struct dsa_mall_tc_entry *mall_tc_entry;
+
+   list_for_each_entry(mall_tc_entry, >mall_tc_list, list)
+   if (mall_tc_entry->cookie == cookie)
+   return mall_tc_entry;
+
+   return NULL;
+}
+
+static int dsa_slave_add_cls_matchall(struct net_device *dev,
+ __be16 protocol,
+ struct tc_cls_matchall_offload *cls,
+ bool ingress)
+{
+   struct dsa_slave_priv *p = netdev_priv(dev);
+   struct dsa_mall_tc_entry *mall_tc_entry;
+   struct dsa_switch *ds = p->parent;
+   struct net *net = dev_net(dev);
+   struct dsa_slave_priv *to_p;
+   struct net_device *to_dev;
+   const struct tc_action *a;
+   int err = -EOPNOTSUPP;
+   LIST_HEAD(actions);
+   int ifindex;
+
+   if (!ds->ops->port_mirror_add)
+   return err;
+
+   if (!tc_single_action(cls->exts)) {
+   netdev_err(dev, "only singular actions are supported\n");
+   return err;
+   }
+
+   mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
+   if (!mall_tc_entry)
+   return -ENOMEM;
+   mall_tc_entry->cookie = cls->cookie;
+
+   tcf_exts_to_list(cls->exts, );
+   a = list_first_entry(, struct tc_action, list);
+
+   if (is_tcf_mirred_egress_mirror(a) && protocol == htons(ETH_P_ALL)) {
+   struct dsa_mall_mirror_tc_entry *mirror;
+
+   mall_tc_entry->type = DSA_PORT_MALL_MIRROR;
+   mirror = _tc_entry->mirror;
+