diff mbox

[net-next,v3,19/26] switchdev: add new swdev_port_bridge_getlink

Message ID E4CD12F19ABA0C4D8729E087A761DC35067FEC4C@ORSMSX101.amr.corp.intel.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Arad, Ronen April 6, 2015, 9:58 p.m. UTC 
>-----Original Message-----

>From: Scott Feldman [mailto:sfeldma@gmail.com]

>Sent: Monday, April 06, 2015 2:13 PM

>To: Arad, Ronen

>Cc: netdev@vger.kernel.org; jiri@resnulli.us; roopa@cumulusnetworks.com;

>linux@roeck-us.net; f.fainelli@gmail.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@intel.com> wrote:

>>

>>

>>>-----Original Message-----

>>>From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]

>>>Sent: Thursday, April 02, 2015 1:10 AM

>>>To: netdev@vger.kernel.org

>>>Cc: jiri@resnulli.us; roopa@cumulusnetworks.com; linux@roeck-us.net;

>>>f.fainelli@gmail.com; Samudrala, Sridhar; Arad, Ronen

>>>Subject: [PATCH net-next v3 19/26] switchdev: add new

>>>swdev_port_bridge_getlink

>>>

>>>From: Scott Feldman <sfeldma@gmail.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.  

                        fprintf(fp, "\n");
                }
        }
-       fprintf(fp, "\n");
+       if (vlan_table_found)
+               fprintf(fp, "\n");
        fflush(fp);
        return 0;
 }

>

>-scott
diff mbox

Patch

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,