Re: [PATCH V3] net-next: dsa: add FIB support

On 13/09/2016 18:21, Vivien Didelot wrote:

Hi John,

John Crispin [off-list ref] writes:

quoted

@@ -237,6 +237,7 @@ struct switchdev_obj;
 struct switchdev_obj_port_fdb;
 struct switchdev_obj_port_mdb;
 struct switchdev_obj_port_vlan;
+struct switchdev_obj_ipv4_fib;

Can you keep it ordered please (put obj_ipv4 above port_fdb).

quoted

 
 struct dsa_switch_ops {
 	struct list_head	list;

@@ -386,6 +387,18 @@ 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));
+
+	/*
+	 * IPV4 routing
+	 */
+	int	(*ipv4_fib_prepare)(struct dsa_switch *ds, int port,
+				    const struct switchdev_obj_ipv4_fib *fib4,
+				    struct switchdev_trans *trans);
+	int	(*ipv4_fib_add)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_ipv4_fib *fib4,
+				struct switchdev_trans *trans);

DSA *_add ops should return void, since no error is supposed to occure in
the commit phase.

If they are port-based operations, please prefix them with "port_",
otherwise, the int port parameter is not necessary.

quoted

+	int	(*ipv4_fib_del)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_ipv4_fib *fib4);
 };
 
 void register_switch_driver(struct dsa_switch_ops *type);

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9ecbe78..c974ac0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c

@@ -334,6 +334,38 @@ static int dsa_slave_port_mdb_dump(struct net_device *dev,
 	return -EOPNOTSUPP;
 }
 
+static int dsa_slave_ipv4_fib_add(struct net_device *dev,
+				  const struct switchdev_obj_ipv4_fib *fib4,
+				  struct switchdev_trans *trans)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int ret;
+
+	if (!ds->ops->ipv4_fib_prepare || !ds->ops->ipv4_fib_add)
+		return -EOPNOTSUPP;
+
+	if (switchdev_trans_ph_prepare(trans))
+		ret = ds->ops->ipv4_fib_prepare(ds, p->port, fib4, trans);
+	else
+		ret = ds->ops->ipv4_fib_add(ds, p->port, fib4, trans);
+
+	return ret;
+}

Please see dsa_slave_port_vlan_add for a better logic with the prepare
phase and void add routine.

quoted

+
+static int dsa_slave_ipv4_fib_del(struct net_device *dev,
+				  const struct switchdev_obj_ipv4_fib *fib4)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int ret = -EOPNOTSUPP;
+
+	if (ds->ops->ipv4_fib_del)
+		ret = ds->ops->ipv4_fib_del(ds, p->port, fib4);
+
+	return ret;
+}

Just curious, isn't there a dump operation for SWITCHDEV_OBJ_ID_IPV4_FIB?

quoted

+
 static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);

@@ -465,6 +497,11 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 					      SWITCHDEV_OBJ_PORT_VLAN(obj),
 					      trans);
 		break;
+	case SWITCHDEV_OBJ_ID_IPV4_FIB:
+		err = dsa_slave_ipv4_fib_add(dev,
+					     SWITCHDEV_OBJ_IPV4_FIB(obj),
+					     trans);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;

@@ -490,6 +527,10 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 		err = dsa_slave_port_vlan_del(dev,
 					      SWITCHDEV_OBJ_PORT_VLAN(obj));
 		break;
+	case SWITCHDEV_OBJ_ID_IPV4_FIB:
+		err = dsa_slave_ipv4_fib_del(dev,
+					     SWITCHDEV_OBJ_IPV4_FIB(obj));
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;

Please keep the SWITCHDEV_OBJ_ID_IPV4_FIB case ordered with other cases
as well.

I'm adding Jiri's in the loop, since he has started a thread on FIB
notifications a few days ago, his feedback might be interesting. If I'm
not mistaken, there is a plan to factorize FID routines (not sure).

Thanks,

        Vivien

Hi Vivien,

i sent an email to Jiri earlier today and he asked me to drop this until
his notification series got merged.

	John