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

[Vivien Didelot]

Hi John,

John Crispin <john@xxxxxxxxxxx> writes:

> @@ -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).

>  
>  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.

> +	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.

> +
> +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?

> +
>  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