From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next v2 25/26] switchdev: convert swdev_fib_ipv4_add/del over to swdev_port_obj_add/del Date: Wed, 1 Apr 2015 18:05:22 +0200 Message-ID: <20150401160522.GE2025@nanopsycho.orion> References: <1427882882-2533-1-git-send-email-sfeldma@gmail.com> <1427882882-2533-26-git-send-email-sfeldma@gmail.com> <551C094D.7000707@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: sfeldma@gmail.com, netdev@vger.kernel.org, linux@roeck-us.net, f.fainelli@gmail.com, sridhar.samudrala@intel.com, ronen.arad@intel.com To: roopa Return-path: Received: from mail-wi0-f180.google.com ([209.85.212.180]:33373 "EHLO mail-wi0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752726AbbDAQF0 (ORCPT ); Wed, 1 Apr 2015 12:05:26 -0400 Received: by wixm2 with SMTP id m2so38845704wix.0 for ; Wed, 01 Apr 2015 09:05:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: <551C094D.7000707@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: Wed, Apr 01, 2015 at 05:05:49PM CEST, roopa@cumulusnetworks.com wrote: >On 4/1/15, 3:08 AM, sfeldma@gmail.com wrote: >>From: Scott Feldman >> >>The IPv4 FIB ops convert nicely to the swdev objs and we're left with only >>four swdev ops: port get/set and port add/del. Other objs will follow, such >>as FDB. So go ahead and convert IPv4 FIB over to swdev obj for consistency, >>anticipating more objs to come. >> >>Signed-off-by: Scott Feldman >>--- >> drivers/net/ethernet/rocker/rocker.c | 42 +++++++++++------------------ >> include/net/switchdev.h | 21 +++++++-------- >> net/switchdev/switchdev.c | 49 +++++++++++++++++++++------------- >> 3 files changed, 56 insertions(+), 56 deletions(-) >> >>diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c >>index b7e54c3..b34efd8 100644 >>--- a/drivers/net/ethernet/rocker/rocker.c >>+++ b/drivers/net/ethernet/rocker/rocker.c >>@@ -4244,12 +4244,19 @@ static int rocker_port_vlans_add(struct rocker_port *rocker_port, >> static int rocker_port_obj_add(struct net_device *dev, struct swdev_obj *obj) >> { >> struct rocker_port *rocker_port = netdev_priv(dev); >>+ struct swdev_obj_ipv4_fib *fib4; >> int err = 0; >> switch (obj->id) { >> case SWDEV_OBJ_PORT_VLAN: >> err = rocker_port_vlans_add(rocker_port, &obj->vlan); >> break; >>+ case SWDEV_OBJ_IPV4_FIB: >>+ fib4 = &obj->ipv4_fib; >>+ err = rocker_port_fib_ipv4(rocker_port, fib4->dst, >>+ fib4->dst_len, fib4->fi, >>+ fib4->tb_id, 0); >>+ break; >> default: >> err = -EOPNOTSUPP; >> break; >>@@ -4289,12 +4296,20 @@ static int rocker_port_vlans_del(struct rocker_port *rocker_port, >> static int rocker_port_obj_del(struct net_device *dev, struct swdev_obj *obj) >> { >> struct rocker_port *rocker_port = netdev_priv(dev); >>+ struct swdev_obj_ipv4_fib *fib4; >> int err = 0; >> switch (obj->id) { >> case SWDEV_OBJ_PORT_VLAN: >> err = rocker_port_vlans_del(rocker_port, &obj->vlan); >> break; >>+ case SWDEV_OBJ_IPV4_FIB: >>+ fib4 = &obj->ipv4_fib; >>+ err = rocker_port_fib_ipv4(rocker_port, fib4->dst, >>+ fib4->dst_len, fib4->fi, >>+ fib4->tb_id, >>+ ROCKER_OP_FLAG_REMOVE); >>+ break; >> default: >> err = -EOPNOTSUPP; >> break; >>@@ -4303,38 +4318,11 @@ static int rocker_port_obj_del(struct net_device *dev, struct swdev_obj *obj) >> return err; >> } >>-static int rocker_port_swdev_fib_ipv4_add(struct net_device *dev, >>- __be32 dst, int dst_len, >>- struct fib_info *fi, >>- u8 tos, u8 type, >>- u32 nlflags, u32 tb_id) >>-{ >>- struct rocker_port *rocker_port = netdev_priv(dev); >>- int flags = 0; >>- >>- return rocker_port_fib_ipv4(rocker_port, dst, dst_len, >>- fi, tb_id, flags); >>-} >>- >>-static int rocker_port_swdev_fib_ipv4_del(struct net_device *dev, >>- __be32 dst, int dst_len, >>- struct fib_info *fi, >>- u8 tos, u8 type, u32 tb_id) >>-{ >>- struct rocker_port *rocker_port = netdev_priv(dev); >>- int flags = ROCKER_OP_FLAG_REMOVE; >>- >>- return rocker_port_fib_ipv4(rocker_port, dst, dst_len, >>- fi, tb_id, flags); >>-} >>- >> static const struct swdev_ops rocker_port_swdev_ops = { >> .swdev_port_attr_get = rocker_port_attr_get, >> .swdev_port_attr_set = rocker_port_attr_set, >> .swdev_port_obj_add = rocker_port_obj_add, >> .swdev_port_obj_del = rocker_port_obj_del, >>- .swdev_fib_ipv4_add = rocker_port_swdev_fib_ipv4_add, >>- .swdev_fib_ipv4_del = rocker_port_swdev_fib_ipv4_del, >> }; >> /******************** >>diff --git a/include/net/switchdev.h b/include/net/switchdev.h >>index 37a2607..efaac55 100644 >>--- a/include/net/switchdev.h >>+++ b/include/net/switchdev.h >>@@ -39,6 +39,7 @@ struct fib_info; >> enum swdev_obj_id { >> SWDEV_OBJ_UNDEFINED, >> SWDEV_OBJ_PORT_VLAN, >>+ SWDEV_OBJ_IPV4_FIB, >> }; >> struct swdev_obj { >>@@ -50,6 +51,15 @@ struct swdev_obj { >> u16 vid_start; >> u16 vid_end; >> } vlan; >>+ struct swdev_obj_ipv4_fib { /* IPV4_FIB */ >>+ u32 dst; >>+ int dst_len; >>+ struct fib_info *fi; >>+ u8 tos; >>+ u8 type; >>+ u32 nlflags; >>+ u32 tb_id; >>+ } ipv4_fib; >> }; >> }; >>@@ -63,10 +73,6 @@ struct swdev_obj { >> * @swdev_port_obj_add: Add an object to port (see swdev_obj). >> * >> * @swdev_port_obj_del: Delete an object from port (see swdev_obj). >>- * >>- * @swdev_fib_ipv4_add: Called to add/modify IPv4 route to switch device. >>- * >>- * @swdev_fib_ipv4_del: Called to delete IPv4 route from switch device. >> */ >> struct swdev_ops { >> int (*swdev_port_attr_get)(struct net_device *dev, >>@@ -77,13 +83,6 @@ struct swdev_ops { >> struct swdev_obj *obj); >> int (*swdev_port_obj_del)(struct net_device *dev, >> struct swdev_obj *obj); >>- int (*swdev_fib_ipv4_add)(struct net_device *dev, __be32 dst, >>- int dst_len, struct fib_info *fi, >>- u8 tos, u8 type, u32 nlflags, >>- u32 tb_id); >>- int (*swdev_fib_ipv4_del)(struct net_device *dev, __be32 dst, >>- int dst_len, struct fib_info *fi, >>- u8 tos, u8 type, u32 tb_id); >> }; >> enum swdev_notifier_type { >>diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >>index 98c631c..4dbd4c4 100644 >>--- a/net/switchdev/switchdev.c >>+++ b/net/switchdev/switchdev.c >>@@ -535,8 +535,19 @@ static struct net_device *swdev_get_dev_by_nhs(struct fib_info *fi) >> int swdev_fib_ipv4_add(u32 dst, int dst_len, struct fib_info *fi, >> u8 tos, u8 type, u32 nlflags, u32 tb_id) >> { >>+ struct swdev_obj fib_obj = { >>+ .id = SWDEV_OBJ_IPV4_FIB, >>+ .ipv4_fib = { >>+ .dst = htonl(dst), >>+ .dst_len = dst_len, >>+ .fi = fi, >>+ .tos = tos, >>+ .type = type, >>+ .nlflags = nlflags, >>+ .tb_id = tb_id, >>+ }, >>+ }; >> struct net_device *dev; >>- const struct swdev_ops *ops; >> int err = 0; >> /* Don't offload route if using custom ip rules or if >>@@ -554,15 +565,10 @@ int swdev_fib_ipv4_add(u32 dst, int dst_len, struct fib_info *fi, >> dev = swdev_get_dev_by_nhs(fi); >> if (!dev) >> return 0; >>- ops = dev->swdev_ops; >>- >>- if (ops->swdev_fib_ipv4_add) { >>- err = ops->swdev_fib_ipv4_add(dev, htonl(dst), dst_len, >>- fi, tos, type, nlflags, >>- tb_id); >>- if (!err) >>- fi->fib_flags |= RTNH_F_EXTERNAL; >>- } >>+ >>+ err = swdev_port_obj_add(dev, &fib_obj); >>+ if (!err) >>+ fi->fib_flags |= RTNH_F_EXTERNAL; >> return err; >> } >>@@ -583,8 +589,19 @@ EXPORT_SYMBOL_GPL(swdev_fib_ipv4_add); >> int swdev_fib_ipv4_del(u32 dst, int dst_len, struct fib_info *fi, >> u8 tos, u8 type, u32 tb_id) >> { >>+ struct swdev_obj fib_obj = { >>+ .id = SWDEV_OBJ_IPV4_FIB, >>+ .ipv4_fib = { >>+ .dst = htonl(dst), >>+ .dst_len = dst_len, >>+ .fi = fi, >>+ .tos = tos, >>+ .type = type, >>+ .nlflags = 0, >>+ .tb_id = tb_id, >>+ }, >>+ }; >This looks nice. However, my only concern is we have now ended up adding a >whole layer of abstraction to all objects. The api abstraction seemed fine. >But, carrying it to objects and duplicating every object in the swdev layer >seems too much duplication. Maybe its just me. >we will end up adding this for bond attributes...vxlan...fdb and nftables +1 >etc. > >The advantage of switchdev in the kernel was to have access the existing >kernel api and objects directly from switchdev layer. >In which case, to me, it would be better to skip this extra layer of objects. >And keep the way you had it originally. I agree. >