On 29/09/17 12:29, Nikolay Aleksandrov wrote:
> On 28/09/17 20:34, Jiri Pirko wrote:
>> From: Yotam Gigi <yot...@mellanox.com>
>>
>> In order to allow the ipmr module to do partial multicast forwarding
>> according to the device parent ID, add the device parent ID field to the
>> VIF struct. This way, the forwarding path can use the parent ID field
>> without invoking switchdev calls, which requires the RTNL lock.
>>
>> When a new VIF is added, set the device parent ID field in it by invoking
>> the switchdev_port_attr_get call.
>>
>> Signed-off-by: Yotam Gigi <yot...@mellanox.com>
>> Reviewed-by: Ido Schimmel <ido...@mellanox.com>
>> Signed-off-by: Jiri Pirko <j...@mellanox.com>
>> ---
>> include/linux/mroute.h | 2 ++
>> net/ipv4/ipmr.c | 9 +++++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/include/linux/mroute.h b/include/linux/mroute.h
>> index b072a84..a46577f 100644
>> --- a/include/linux/mroute.h
>> +++ b/include/linux/mroute.h
>> @@ -57,6 +57,8 @@ static inline bool ipmr_rule_default(const struct fib_rule
>> *rule)
>>
>> struct vif_device {
>> struct net_device *dev; /* Device we are using
>> */
>> + struct netdev_phys_item_id dev_parent_id; /* Device parent ID
>> */
>> + bool dev_parent_id_valid;
>> unsigned long bytes_in,bytes_out;
>> unsigned long pkt_in,pkt_out; /* Statistics
>> */
>> unsigned long rate_limit; /* Traffic shaping (NI)
>> */
>> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
>> index 292a8e8..4566c54 100644
>> --- a/net/ipv4/ipmr.c
>> +++ b/net/ipv4/ipmr.c
>> @@ -67,6 +67,7 @@
>> #include <net/fib_rules.h>
>> #include <linux/netconf.h>
>> #include <net/nexthop.h>
>> +#include <net/switchdev.h>
>>
>> struct ipmr_rule {
>> struct fib_rule common;
>> @@ -868,6 +869,9 @@ static int vif_add(struct net *net, struct mr_table *mrt,
>> struct vifctl *vifc, int mrtsock)
>> {
>> int vifi = vifc->vifc_vifi;
>> + struct switchdev_attr attr = {
>> + .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
>> + };
>> struct vif_device *v = &mrt->vif_table[vifi];
>> struct net_device *dev;
>> struct in_device *in_dev;
>> @@ -942,6 +946,11 @@ static int vif_add(struct net *net, struct mr_table
>> *mrt,
>>
>> /* Fill in the VIF structures */
>>
>> + attr.orig_dev = dev;
>> + if (!switchdev_port_attr_get(dev, &attr)) {
>> + v->dev_parent_id_valid = true;
>> + memcpy(v->dev_parent_id.id, attr.u.ppid.id, attr.u.ppid.id_len);
>
> Hmm, shouldn't you set dev_parent_id.id_len too ? It would seem
> netdev_phys_item_id_same()
> uses it in the comparison and without the len it would always look like
> they're the same
> because memcmp will simply return 0 with count = 0.
Also maybe we can use the non-zero id_len as a signal that it was set and drop
the dev_parent_id_valid
field altogether, it would seem there's no valid reason to have id_len == 0 and
yet expect a valid
parent_id.
>
>> + }
>> v->rate_limit = vifc->vifc_rate_limit;
>> v->local = vifc->vifc_lcl_addr.s_addr;
>> v->remote = vifc->vifc_rmt_addr.s_addr;
>>
>
