lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 1 Apr 2015 18:05:22 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	roopa <roopa@...ulusnetworks.com>
Cc:	sfeldma@...il.com, netdev@...r.kernel.org, linux@...ck-us.net,
	f.fainelli@...il.com, sridhar.samudrala@...el.com,
	ronen.arad@...el.com
Subject: Re: [PATCH net-next v2 25/26] switchdev: convert
 swdev_fib_ipv4_add/del over to swdev_port_obj_add/del

Wed, Apr 01, 2015 at 05:05:49PM CEST, roopa@...ulusnetworks.com wrote:
>On 4/1/15, 3:08 AM, sfeldma@...il.com wrote:
>>From: Scott Feldman <sfeldma@...il.com>
>>
>>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 <sfeldma@...il.com>
>>---
>>  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.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists