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 <f.faine...@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?
>+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;
>+}
