Re: [PATCH net-next v2 1/4] net: dsa: Add plumbing for port mirroring
From: Jiri Pirko <jiri@resnulli.us>
Date: 2017-01-28 09:23:14
Sat, Jan 28, 2017 at 02:25:25AM CET, f.fainelli@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 <f.fainelli@gmail.com> ---
[...]
+/*
+ * 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?
quoted hunk ↗ jump to hunk
+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, &actions);
+ a = list_first_entry(&actions, 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 = &mall_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(&mall_tc_entry->list, &p->mall_tc_list);
+ return 0;
+
+err_add_action:
+ kfree(mall_tc_entry);
+ return err;
+}