Date:	Mon, 6 Apr 2015 21:58:28 +0000
From:	"Arad, Ronen" <ronen.arad@...el.com>
To:	Scott Feldman <sfeldma@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:	"jiri@...nulli.us" <jiri@...nulli.us>,
	"roopa@...ulusnetworks.com" <roopa@...ulusnetworks.com>,
	"linux@...ck-us.net" <linux@...ck-us.net>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [PATCH net-next v3 19/26] switchdev: add new
 swdev_port_bridge_getlink



>-----Original Message-----
>From: Scott Feldman [mailto:sfeldma@...il.com]
>Sent: Monday, April 06, 2015 2:13 PM
>To: Arad, Ronen
>Cc: netdev@...r.kernel.org; jiri@...nulli.us; roopa@...ulusnetworks.com;
>linux@...ck-us.net; f.fainelli@...il.com; Samudrala, Sridhar
>Subject: Re: [PATCH net-next v3 19/26] switchdev: add new
>swdev_port_bridge_getlink
>
>On Sat, Apr 4, 2015 at 9:56 AM, Arad, Ronen <ronen.arad@...el.com> wrote:
>>
>>
>>>-----Original Message-----
>>>From: sfeldma@...il.com [mailto:sfeldma@...il.com]
>>>Sent: Thursday, April 02, 2015 1:10 AM
>>>To: netdev@...r.kernel.org
>>>Cc: jiri@...nulli.us; roopa@...ulusnetworks.com; linux@...ck-us.net;
>>>f.fainelli@...il.com; Samudrala, Sridhar; Arad, Ronen
>>>Subject: [PATCH net-next v3 19/26] switchdev: add new
>>>swdev_port_bridge_getlink
>>>
>>>From: Scott Feldman <sfeldma@...il.com>
>>>
>>>Like bridge_setlink, add swdev wrapper to handle bridge_getlink and call
>into
>>>port driver to get port attrs.  For now, only BR_LEARNING and
>BR_LEARNING_SYNC
>>>are returned.  To add more, we'll probably want to break away from
>>>ndo_dflt_bridge_getlink() and build the netlink skb directly in the swdev
>>>code.
>
>[cut]
>
>>
>> swdev_port_bridge_getlink and the function it wraps
>> ndo_dflt_bridge_getlink are only useful when all you need is to return
>> a set of brport attributes.
>> When a switchdev driver supports VLAN filtering without being enslaved
>> to a bridge this no longer works. ndo_dflt_bridge_getlink will end up in
>> a netlink RTM_NEWLINK message without the VLAN filtering info which is
>> maintained internally by the switchdev driver.
>> The driver will have to duplicate the code in ndo_dflt_bridge_getlink in
>> order to return a single RTM_NEWLINK message per port.
>> Can we break ndo_dflt_bridge_getlink to components that could be used
>> by such driver?
>> It would be useful to have one function that could fill the
>> IFLA_PROTINFO and another one for the IFLA_AF_SPEC.
>
>I couldn't find a consumer of the RTM_NEWLINK msg returned from
>RTM_GETLINK that wants the VLAN part of afspec.  Did I miss one?

It is in my driver which is not up-streamed yet.
I don't know if we really need to make any changes in the kernel.
It could be appropriate to generate two separate netlink messages from
the driver for a single port:
1) What ndo_dflt_bridge_getlink returns. This consists of IFLA_AF_SPEC
  With bridge mode and self flag and IFLA_PROTINFO with bridge flags.
2) A second message with VLAN table in IFLA_AF_SPEC. This is what the
  bridge module would have returned had the port been enslaved to a 
  bridge.
It seems that "bridge vlan" command had to cope with that anyway.
I made a change to iproute2 bridge command such that is defers 
printing the interface name until it encounters the first IFLA_AF_SPEC
which includes an IFLA_BRIDGE_VLAN_INFO attribute instead of doing it
unconditionally for every IFLA_AF_SPEC.

Here is my suggested iproute2 patch. I can post it if the suggested handling of AF_BRIDGE getlink (as I described above) is acceptable/
desirable.  

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 2ae739c..de423b9 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -145,6 +145,7 @@ static int print_vlan(const struct sockaddr_nl *who,
        struct ifinfomsg *ifm = NLMSG_DATA(n);
        int len = n->nlmsg_len;
        struct rtattr * tb[IFLA_MAX+1];
+       bool vlan_table_found = false;
 
        if (n->nlmsg_type != RTM_NEWLINK) {
                fprintf(stderr, "Not RTM_NEWLINK: %08x %08x %08x\n",
@@ -174,13 +175,22 @@ static int print_vlan(const struct sockaddr_nl *who,
                struct rtattr *i, *list = tb[IFLA_AF_SPEC];
                int rem = RTA_PAYLOAD(list);
 
-               fprintf(fp, "%s", ll_index_to_name(ifm->ifi_index));
                for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {                        struct bridge_vlan_info *vinfo;
 
                        if (i->rta_type != IFLA_BRIDGE_VLAN_INFO)
                                continue;
 
: 
+                       /* Printing interface name is deferred until first
+                        * IFLA_BRIDGE_VLAN_INFO is seen since there could be
+                        * IFLA_AF_SPEC with only IFLA_BRIDGE_MODE or/and
+                        * IFLA_BRIDGE_FLAGS
+                        */
+                       if (!vlan_table_found) {
+                               vlan_table_found = true;
+                               fprintf(fp, "%s", ll_index_to_name(ifm->ifi_inde+                       }
+
                        vinfo = RTA_DATA(i);
                        if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END)
                                fprintf(fp, "-%hu", vinfo->vid);
@@ -195,7 +205,8 @@ static int print_vlan(const struct sockaddr_nl *who,
                        fprintf(fp, "\n");
                }
        }
-       fprintf(fp, "\n");
+       if (vlan_table_found)
+               fprintf(fp, "\n");
        fflush(fp);
        return 0;
 }

>
>-scott

Powered by blists - more mailing lists