On 13/09/2016 18:21, Vivien Didelot wrote:
> Hi John,
>
> John Crispin <j...@phrozen.org> 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
Hi Vivien,
i sent an email to Jiri earlier today and he asked me to drop this until
his notification series got merged.
John
>
