DSA and underlying 802.1Q encapsulation

DSA and underlying 802.1Q encapsulation

[Vivien Didelot]

Hi,

I'm doing tests with VLAN support in DSA and I noticed that the EDSA 
frame is prepended with a 802.1q header once queued to the underlying 
network device, in net/dsa/tag_edsa.c:

    skb->dev = p->parent->dst->master_netdev;
    dev_queue_xmit(skb);

This issue can be observed with the following dump:

    curl -s http://ix.io/iIv | tcpdump -en -r -

I suspect that the DSA code must clear some VLAN flags in the skb
structure, in order to prevent the additional encapsulation by the lower
level. Does this make sense?

Thanks,
-v

Re: DSA and underlying 802.1Q encapsulation

[Guenter Roeck]

On Tue, May 26, 2015 at 06:29:57PM -0400, Vivien Didelot wrote:
> Hi,
> 
> I'm doing tests with VLAN support in DSA and I noticed that the EDSA 
> frame is prepended with a 802.1q header once queued to the underlying 
> network device, in net/dsa/tag_edsa.c:
> 
>     skb->dev = p->parent->dst->master_netdev;
>     dev_queue_xmit(skb);
> 
> This issue can be observed with the following dump:
> 
>     curl -s http://ix.io/iIv | tcpdump -en -r -
> 
> I suspect that the DSA code must clear some VLAN flags in the skb
> structure, in order to prevent the additional encapsulation by the lower
> level. Does this make sense?
> 

Hi Vivien,

Interesting question. Does the underlying network device support VLAN HW
acceleration (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX) ?

If yes, the dsa code may need to move the tag into the header.
If we are lucky, a call to vlan_hwaccel_push_inside() might do it.

Do you have some vlan dsa code to share, by any chance ? That might
save me some time, as I am looking into it as well.

Thanks,
Guenter

Re: DSA and underlying 802.1Q encapsulation

[Vivien Didelot]

Hi Guenter,

> Interesting question. Does the underlying network device support VLAN HW
> acceleration (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX) ?

Yes, in our case, it's an IGB device.

I also set the DSA slave_dev->features and slave_dev->vlan_features to
master->vlan_features | NETIF_F_VLAN_FEATURES | NETIF_F_HW_SWITCH_OFFLOAD, 
not sure if it is good or not.

> If yes, the dsa code may need to move the tag into the header.
> If we are lucky, a call to vlan_hwaccel_push_inside() might do it.

Thanks, I'm currently looking into it and doing some tests, I'm coming back to 
you asap.

> Do you have some vlan dsa code to share, by any chance ? That might
> save me some time, as I am looking into it as well.

Yes, I am about the send an RFC for fully integrated 802.1q VLAN support on 
Marvell 88E6352 devices.

Thanks,
-v

Re: DSA and underlying 802.1Q encapsulation

[Guenter Roeck]

On 05/27/2015 01:48 PM, Vivien Didelot wrote:
> Hi Guenter,
>
>> Interesting question. Does the underlying network device support VLAN HW
>> acceleration (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX) ?
>
> Yes, in our case, it's an IGB device.
>
> I also set the DSA slave_dev->features and slave_dev->vlan_features to
> master->vlan_features | NETIF_F_VLAN_FEATURES | NETIF_F_HW_SWITCH_OFFLOAD,
> not sure if it is good or not.
>
I have experimented with NETIF_F_HW_VLAN_CTAG_FILTER, with associated code
in the switch driver, to provide vlan filtering in the switch, but so far
I think I am doing something wrong.

Do you have lock debugging enabled in your code ? I am getting a recursive
lock warning due to a recursive call to dev_mc_sync(). I think we may have
to implement lock nesting for dsa, similar to how it id done for vlan
support, but I have not been able to figure out how exactly it works yet.

>> If yes, the dsa code may need to move the tag into the header.
>> If we are lucky, a call to vlan_hwaccel_push_inside() might do it.
>
> Thanks, I'm currently looking into it and doing some tests, I'm coming back to
> you asap.
>
>> Do you have some vlan dsa code to share, by any chance ? That might
>> save me some time, as I am looking into it as well.
>
> Yes, I am about the send an RFC for fully integrated 802.1q VLAN support on
> Marvell 88E6352 devices.
>

Excellent, that is going to safe me a lot of work! If you don't mind,
please Cc: me as I am not subscribed to the network mailing list.

Thanks,
Guenter

Re: DSA and underlying 802.1Q encapsulation

[Andrew Lunn]

> Do you have lock debugging enabled in your code ? I am getting a recursive
> lock warning due to a recursive call to dev_mc_sync(). I think we may have
> to implement lock nesting for dsa, similar to how it id done for vlan
> support, but I have not been able to figure out how exactly it works yet.

I might be able to help, since i solve two similar problems already in
DSA, one for MDIO bus, and a second one for transmit buffers.

     Andrew

Re: DSA and underlying 802.1Q encapsulation

[Guenter Roeck]

On 05/27/2015 02:05 PM, Andrew Lunn wrote:
>> Do you have lock debugging enabled in your code ? I am getting a recursive
>> lock warning due to a recursive call to dev_mc_sync(). I think we may have
>> to implement lock nesting for dsa, similar to how it id done for vlan
>> support, but I have not been able to figure out how exactly it works yet.
>
> I might be able to help, since i solve two similar problems already in
> DSA, one for MDIO bus, and a second one for transmit buffers.
>
>       Andrew
>
What I did is to create a vlan interface on one of the dsa slave ports
and enabling it.

This results in a call to dev_mc_sync() on the dsa interface, which is passed
on to the real interface. dev_mc_sync() calls netif_addr_lock_nested(to),
which results in the recursive lock message.

  ifconfig/2291 is trying to acquire lock:
   (_xmit_ETHER/1){+.....}, at: [<ffffffff8175b757>] dev_mc_sync+0x57/0xa0

  but task is already holding lock:
   (_xmit_ETHER/1){+.....}, at: [<ffffffff8175b757>] dev_mc_sync+0x57/0xa0

  other info that might help us debug this:
   Possible unsafe locking scenario:

         CPU0
         ----
    lock(_xmit_ETHER/1);
    lock(_xmit_ETHER/1);

Pretty much inevitable as far as I can see.

Tentative solution would be to implement nesting as in the 802.1q code,
ie similar to vlan_dev_get_lock_subclass() and
	vlan->nest_level = dev_get_nest_level(real_dev, is_vlan_dev) + 1;
but I am not sure on how to set nest_level.
Can we call dev_get_nest_level() with the Ethernet (cpu) interface as
parameter and add 1 ? If so, what should the second parameter evaluate ?
Or is there a better solution ?

Thanks,
Guenter

Re: DSA and underlying 802.1Q encapsulation

[Andrew Lunn]

On Wed, May 27, 2015 at 03:51:59PM -0700, Guenter Roeck wrote:
> On 05/27/2015 02:05 PM, Andrew Lunn wrote:
> >>Do you have lock debugging enabled in your code ? I am getting a recursive
> >>lock warning due to a recursive call to dev_mc_sync(). I think we may have
> >>to implement lock nesting for dsa, similar to how it id done for vlan
> >>support, but I have not been able to figure out how exactly it works yet.
> >
> >I might be able to help, since i solve two similar problems already in
> >DSA, one for MDIO bus, and a second one for transmit buffers.
> >
> >      Andrew
> >
> What I did is to create a vlan interface on one of the dsa slave ports
> and enabling it.
> 
> This results in a call to dev_mc_sync() on the dsa interface, which is passed
> on to the real interface. dev_mc_sync() calls netif_addr_lock_nested(to),
> which results in the recursive lock message.
> 
>  ifconfig/2291 is trying to acquire lock:
>   (_xmit_ETHER/1){+.....}, at: [<ffffffff8175b757>] dev_mc_sync+0x57/0xa0
> 
>  but task is already holding lock:
>   (_xmit_ETHER/1){+.....}, at: [<ffffffff8175b757>] dev_mc_sync+0x57/0xa0
> 
>  other info that might help us debug this:
>   Possible unsafe locking scenario:
> 
>         CPU0
>         ----
>    lock(_xmit_ETHER/1);
>    lock(_xmit_ETHER/1);
> 
> Pretty much inevitable as far as I can see.

It looks like DSA needs to implement ndo_get_lock_subclass().

We can probably copy the code from macvlan.c.

   Andrew

Re: DSA and underlying 802.1Q encapsulation

[Guenter Roeck]

On 05/27/2015 06:46 PM, Andrew Lunn wrote:
> On Wed, May 27, 2015 at 03:51:59PM -0700, Guenter Roeck wrote:
>> On 05/27/2015 02:05 PM, Andrew Lunn wrote:
>>>> Do you have lock debugging enabled in your code ? I am getting a recursive
>>>> lock warning due to a recursive call to dev_mc_sync(). I think we may have
>>>> to implement lock nesting for dsa, similar to how it id done for vlan
>>>> support, but I have not been able to figure out how exactly it works yet.
>>>
>>> I might be able to help, since i solve two similar problems already in
>>> DSA, one for MDIO bus, and a second one for transmit buffers.
>>>
>>>       Andrew
>>>
>> What I did is to create a vlan interface on one of the dsa slave ports
>> and enabling it.
>>
>> This results in a call to dev_mc_sync() on the dsa interface, which is passed
>> on to the real interface. dev_mc_sync() calls netif_addr_lock_nested(to),
>> which results in the recursive lock message.
>>
>>   ifconfig/2291 is trying to acquire lock:
>>    (_xmit_ETHER/1){+.....}, at: [<ffffffff8175b757>] dev_mc_sync+0x57/0xa0
>>
>>   but task is already holding lock:
>>    (_xmit_ETHER/1){+.....}, at: [<ffffffff8175b757>] dev_mc_sync+0x57/0xa0
>>
>>   other info that might help us debug this:
>>    Possible unsafe locking scenario:
>>
>>          CPU0
>>          ----
>>     lock(_xmit_ETHER/1);
>>     lock(_xmit_ETHER/1);
>>
>> Pretty much inevitable as far as I can see.
>
> It looks like DSA needs to implement ndo_get_lock_subclass().
>
> We can probably copy the code from macvlan.c.
>

Not entirely, but I think I have an idea. I'll give it a try.

Guenter

Re: DSA and underlying 802.1Q encapsulation

[Vivien Didelot]

Hi Guenter,

>>> If yes, the dsa code may need to move the tag into the header.
>>> If we are lucky, a call to vlan_hwaccel_push_inside() might do it.
>>
>> Thanks, I'm currently looking into it and doing some tests, I'm coming back to
>> you asap.

Issue fixed, thanks! vlan_hwaccel_push_inside() adds additional memmove, but
this approach is simpler.

>>> Do you have some vlan dsa code to share, by any chance ? That might
>>> save me some time, as I am looking into it as well.
>>
>> Yes, I am about the send an RFC for fully integrated 802.1q VLAN support on
>> Marvell 88E6352 devices.
>>
> Excellent, that is going to safe me a lot of work! If you don't mind,
> please Cc: me as I am not subscribed to the network mailing list.

My work is based on v4.1-rc3. Unfortunately the VLAN support breaks on latest
net-next/master (e.g. NETIF_F_HW_SWITCH_OFFLOAD is removed; .ndo_bridge_setlink
used to set tag and PVID, is moved to switchdev.)

Are you willing to give it a try based on v4.1-rc3 in the meantime?
Otherwise, I'll send the RFC as soon as the support is functional on net-next.

Thanks,
-v

Re: DSA and underlying 802.1Q encapsulation

[Guenter Roeck]

On 05/28/2015 06:44 AM, Vivien Didelot wrote:
> Hi Guenter,
>
>>>> If yes, the dsa code may need to move the tag into the header.
>>>> If we are lucky, a call to vlan_hwaccel_push_inside() might do it.
>>>
>>> Thanks, I'm currently looking into it and doing some tests, I'm coming back to
>>> you asap.
>
> Issue fixed, thanks! vlan_hwaccel_push_inside() adds additional memmove, but
> this approach is simpler.
>
>>>> Do you have some vlan dsa code to share, by any chance ? That might
>>>> save me some time, as I am looking into it as well.
>>>
>>> Yes, I am about the send an RFC for fully integrated 802.1q VLAN support on
>>> Marvell 88E6352 devices.
>>>
>> Excellent, that is going to safe me a lot of work! If you don't mind,
>> please Cc: me as I am not subscribed to the network mailing list.
>
> My work is based on v4.1-rc3. Unfortunately the VLAN support breaks on latest
> net-next/master (e.g. NETIF_F_HW_SWITCH_OFFLOAD is removed; .ndo_bridge_setlink
> used to set tag and PVID, is moved to switchdev.)
>
> Are you willing to give it a try based on v4.1-rc3 in the meantime?
> Otherwise, I'll send the RFC as soon as the support is functional on net-next.
>

I already rebased my code to net-next, not for the switchdev changes but
to catch the dsa changes, so net-next would be much better. I'd like to get
a glance at your code, though, if that is possible.

Thanks,
Guenter

[RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Vivien Didelot]

This RFC is based on v4.1-rc3.

It is meant to get a glance to the commits responsible to implement the
necessary NDOs between DSA and the Marvell 88E6352 switch driver.

With this support, I am able to create VLANs with (un)tagged ports, setting
their default VID, from a bridge.

To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:

    ip link add name br0 type bridge
    [...]
    ip link set dev swp2 up master br0
    [...]
    bridge vlan add vid 400 pvid untagged dev swp2
    bridge vlan add vid 400 pvid untagged dev swp3
    bridge vlan add vid 400 dev swp4
    [...]
    ip link add link br0 name br0.400 type vlan id 400
    [...]
    bridge vlan add dev br0 vid 400 self

The code is currently being rebased to the latest net-next/master.

Seems like the way to go now is through switchdev attr getter/setter...

Vivien Didelot (3):
  net: dsa: add basic support for VLAN ndo
  net: dsa: mv88e6xxx: add support for VTU operations
  net: dsa: mv88e6352: add support for VLAN

 drivers/net/dsa/mv88e6352.c |   3 +
 drivers/net/dsa/mv88e6xxx.c | 309 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  28 ++++
 include/net/dsa.h           |   9 ++
 net/dsa/slave.c             |  76 ++++++++++-
 5 files changed, 423 insertions(+), 2 deletions(-)

-- 
2.4.1

[RFC 1/3] net: dsa: add basic support for VLAN ndo

[Vivien Didelot]

This patch adds the ndo_vlan_rx_add_vid, ndo_vlan_rx_kill_vid, and
ndo_bridge_setlink wrapper operations, used to create and remove VLAN
entries in a DSA switch VLAN database.

The switch drivers have to implement the port_vlan_add, port_vlan_kill,
and port_bridge_setlink functions, in order to support VLANs.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h |  9 +++++++
 net/dsa/slave.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63b..cf02357 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -19,6 +19,7 @@
 #include <linux/phy.h>
 #include <linux/phy_fixed.h>
 #include <linux/ethtool.h>
+#include <uapi/linux/if_bridge.h>
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE = 0,
@@ -302,6 +303,14 @@ struct dsa_switch_driver {
 			   const unsigned char *addr, u16 vid);
 	int	(*fdb_getnext)(struct dsa_switch *ds, int port,
 			       unsigned char *addr, bool *is_static);
+
+	/*
+	 * VLAN support
+	 */
+	int	(*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid);
+	int	(*port_vlan_kill)(struct dsa_switch *ds, int port, u16 vid);
+	int	(*port_bridge_setlink)(struct dsa_switch *ds, int port,
+				       struct bridge_vlan_info *vinfo);
 };
 
 void register_switch_driver(struct dsa_switch_driver *type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 827cda56..72c3ff0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -412,6 +412,71 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+static int dsa_slave_vlan_rx_add_vid(struct net_device *dev,
+				     __be16 proto, u16 vid)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (!ds->drv->port_vlan_add)
+		return -EOPNOTSUPP;
+
+	netdev_dbg(dev, "adding to VLAN %d\n", vid);
+
+	return ds->drv->port_vlan_add(ds, p->port, vid);
+}
+
+static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev,
+				      __be16 proto, u16 vid)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (!ds->drv->port_vlan_kill)
+		return -EOPNOTSUPP;
+
+	netdev_dbg(dev, "removing from VLAN %d\n", vid);
+
+	return ds->drv->port_vlan_kill(ds, p->port, vid);
+}
+
+static int dsa_slave_bridge_setlink(struct net_device *dev,
+				    struct nlmsghdr *nlh, u16 flags)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	struct nlattr *afspec;
+	struct nlattr *attr;
+	struct bridge_vlan_info *vinfo = NULL;
+	int rem;
+
+	if (!ds->drv->port_bridge_setlink)
+		return -EOPNOTSUPP;
+
+	afspec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
+	if (!afspec)
+		return -EINVAL;
+
+	nla_for_each_nested(attr, afspec, rem) {
+		if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
+			continue;
+
+		if (nla_len(attr) != sizeof(struct bridge_vlan_info))
+			return -EINVAL;
+
+		vinfo = nla_data(attr);
+	}
+
+	if (!vinfo)
+		return -EINVAL;
+
+	netdev_dbg(dev, "setting link to VLAN %d%s%s\n", vinfo->vid,
+		   vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED ?  " untagged" : "",
+		   vinfo->flags & BRIDGE_VLAN_INFO_PVID ? " (default)" : "");
+
+	return ds->drv->port_bridge_setlink(ds, p->port, vinfo);
+}
+
 
 /* ethtool operations *******************************************************/
 static int
@@ -673,6 +738,9 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_fdb_dump		= dsa_slave_fdb_dump,
 	.ndo_do_ioctl		= dsa_slave_ioctl,
 	.ndo_get_iflink		= dsa_slave_get_iflink,
+	.ndo_vlan_rx_add_vid	= dsa_slave_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid	= dsa_slave_vlan_rx_kill_vid,
+	.ndo_bridge_setlink	= dsa_slave_bridge_setlink,
 };
 
 static const struct swdev_ops dsa_slave_swdev_ops = {
@@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features;
+	slave_dev->features = master->vlan_features |
+		NETIF_F_VLAN_FEATURES |
+		NETIF_F_HW_SWITCH_OFFLOAD;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	eth_hw_addr_inherit(slave_dev, master);
 	slave_dev->tx_queue_len = 0;
@@ -863,7 +933,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 
 	SET_NETDEV_DEV(slave_dev, parent);
 	slave_dev->dev.of_node = ds->pd->port_dn[port];
-	slave_dev->vlan_features = master->vlan_features;
+	slave_dev->vlan_features = master->vlan_features |
+		NETIF_F_VLAN_FEATURES |
+		NETIF_F_HW_SWITCH_OFFLOAD;
 
 	p = netdev_priv(slave_dev);
 	p->dev = slave_dev;
-- 
2.4.1

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Scott Feldman]

On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> This patch adds the ndo_vlan_rx_add_vid, ndo_vlan_rx_kill_vid, and
> ndo_bridge_setlink wrapper operations, used to create and remove VLAN
> entries in a DSA switch VLAN database.
>
> The switch drivers have to implement the port_vlan_add, port_vlan_kill,
> and port_bridge_setlink functions, in order to support VLANs.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  include/net/dsa.h |  9 +++++++
>  net/dsa/slave.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63b..cf02357 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -19,6 +19,7 @@
>  #include <linux/phy.h>
>  #include <linux/phy_fixed.h>
>  #include <linux/ethtool.h>
> +#include <uapi/linux/if_bridge.h>
>
>  enum dsa_tag_protocol {
>         DSA_TAG_PROTO_NONE = 0,
> @@ -302,6 +303,14 @@ struct dsa_switch_driver {
>                            const unsigned char *addr, u16 vid);
>         int     (*fdb_getnext)(struct dsa_switch *ds, int port,
>                                unsigned char *addr, bool *is_static);
> +
> +       /*
> +        * VLAN support
> +        */
> +       int     (*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid);
> +       int     (*port_vlan_kill)(struct dsa_switch *ds, int port, u16 vid);
> +       int     (*port_bridge_setlink)(struct dsa_switch *ds, int port,
> +                                      struct bridge_vlan_info *vinfo);
>  };
>
>  void register_switch_driver(struct dsa_switch_driver *type);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 827cda56..72c3ff0 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -412,6 +412,71 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
>         return NETDEV_TX_OK;
>  }
>
> +static int dsa_slave_vlan_rx_add_vid(struct net_device *dev,
> +                                    __be16 proto, u16 vid)
> +{
> +       struct dsa_slave_priv *p = netdev_priv(dev);
> +       struct dsa_switch *ds = p->parent;
> +
> +       if (!ds->drv->port_vlan_add)
> +               return -EOPNOTSUPP;
> +
> +       netdev_dbg(dev, "adding to VLAN %d\n", vid);
> +
> +       return ds->drv->port_vlan_add(ds, p->port, vid);
> +}
> +
> +static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev,
> +                                     __be16 proto, u16 vid)
> +{
> +       struct dsa_slave_priv *p = netdev_priv(dev);
> +       struct dsa_switch *ds = p->parent;
> +
> +       if (!ds->drv->port_vlan_kill)
> +               return -EOPNOTSUPP;
> +
> +       netdev_dbg(dev, "removing from VLAN %d\n", vid);
> +
> +       return ds->drv->port_vlan_kill(ds, p->port, vid);
> +}
> +
> +static int dsa_slave_bridge_setlink(struct net_device *dev,
> +                                   struct nlmsghdr *nlh, u16 flags)
> +{
> +       struct dsa_slave_priv *p = netdev_priv(dev);
> +       struct dsa_switch *ds = p->parent;
> +       struct nlattr *afspec;
> +       struct nlattr *attr;
> +       struct bridge_vlan_info *vinfo = NULL;
> +       int rem;
> +
> +       if (!ds->drv->port_bridge_setlink)
> +               return -EOPNOTSUPP;
> +
> +       afspec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
> +       if (!afspec)
> +               return -EINVAL;
> +
> +       nla_for_each_nested(attr, afspec, rem) {
> +               if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
> +                       continue;
> +
> +               if (nla_len(attr) != sizeof(struct bridge_vlan_info))
> +                       return -EINVAL;
> +
> +               vinfo = nla_data(attr);
> +       }

This is only processing the first IFLA_BRIDGE_VLAN_INFO in the
IFLA_AF_SPEC nest.  More can be packed in the nest to support ranges
of vlans.

> +       if (!vinfo)
> +               return -EINVAL;
> +
> +       netdev_dbg(dev, "setting link to VLAN %d%s%s\n", vinfo->vid,
> +                  vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED ?  " untagged" : "",
> +                  vinfo->flags & BRIDGE_VLAN_INFO_PVID ? " (default)" : "");
> +
> +       return ds->drv->port_bridge_setlink(ds, p->port, vinfo);
> +}
> +
>
>  /* ethtool operations *******************************************************/
>  static int
> @@ -673,6 +738,9 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
>         .ndo_fdb_dump           = dsa_slave_fdb_dump,
>         .ndo_do_ioctl           = dsa_slave_ioctl,
>         .ndo_get_iflink         = dsa_slave_get_iflink,
> +       .ndo_vlan_rx_add_vid    = dsa_slave_vlan_rx_add_vid,
> +       .ndo_vlan_rx_kill_vid   = dsa_slave_vlan_rx_kill_vid,
> +       .ndo_bridge_setlink     = dsa_slave_bridge_setlink,
>  };
>
>  static const struct swdev_ops dsa_slave_swdev_ops = {
> @@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>         if (slave_dev == NULL)
>                 return -ENOMEM;
>
> -       slave_dev->features = master->vlan_features;
> +       slave_dev->features = master->vlan_features |
> +               NETIF_F_VLAN_FEATURES |
> +               NETIF_F_HW_SWITCH_OFFLOAD;
>         slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>         eth_hw_addr_inherit(slave_dev, master);
>         slave_dev->tx_queue_len = 0;
> @@ -863,7 +933,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>
>         SET_NETDEV_DEV(slave_dev, parent);
>         slave_dev->dev.of_node = ds->pd->port_dn[port];
> -       slave_dev->vlan_features = master->vlan_features;
> +       slave_dev->vlan_features = master->vlan_features |
> +               NETIF_F_VLAN_FEATURES |
> +               NETIF_F_HW_SWITCH_OFFLOAD;
>
>         p = netdev_priv(slave_dev);
>         p->dev = slave_dev;
> --
> 2.4.1
>

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Or Gerlitz]

On Fri, May 29, 2015 at 12:37 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> @@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>         if (slave_dev == NULL)
>                 return -ENOMEM;
>
> -       slave_dev->features = master->vlan_features;
> +       slave_dev->features = master->vlan_features |
> +               NETIF_F_VLAN_FEATURES |
> +               NETIF_F_HW_SWITCH_OFFLOAD;

wait... didn't commit 7889cbee8357aaed85898d028829dfb4f75bae2c  remove
NETIF_F_HW_SWITCH_OFFLOAD?

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Vivien Didelot]

Hi,

----- On May 29, 2015, at 11:24 AM, Or Gerlitz gerlitz.or@gmail.com wrote:

> On Fri, May 29, 2015 at 12:37 AM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
>> @@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
>> *parent,
>>         if (slave_dev == NULL)
>>                 return -ENOMEM;
>>
>> -       slave_dev->features = master->vlan_features;
>> +       slave_dev->features = master->vlan_features |
>> +               NETIF_F_VLAN_FEATURES |
>> +               NETIF_F_HW_SWITCH_OFFLOAD;
> 
> wait... didn't commit 7889cbee8357aaed85898d028829dfb4f75bae2c  remove
> NETIF_F_HW_SWITCH_OFFLOAD?

Indeed, note that this RFC is based on v4.1-rc3. This will become unneeded I guess.

BTW, given the commit message, I didn't really understand why?

Thanks,
-v

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Or Gerlitz]

On Fri, May 29, 2015 at 6:38 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Hi,
>
> ----- On May 29, 2015, at 11:24 AM, Or Gerlitz gerlitz.or@gmail.com wrote:
>
>> On Fri, May 29, 2015 at 12:37 AM, Vivien Didelot
>> <vivien.didelot@savoirfairelinux.com> wrote:
>>> @@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
>>> *parent,
>>>         if (slave_dev == NULL)
>>>                 return -ENOMEM;
>>>
>>> -       slave_dev->features = master->vlan_features;
>>> +       slave_dev->features = master->vlan_features |
>>> +               NETIF_F_VLAN_FEATURES |
>>> +               NETIF_F_HW_SWITCH_OFFLOAD;
>>
>> wait... didn't commit 7889cbee8357aaed85898d028829dfb4f75bae2c  remove
>> NETIF_F_HW_SWITCH_OFFLOAD?
>
> Indeed, note that this RFC is based on v4.1-rc3. This will become unneeded I guess.


You should rebase networking patches proposed for the next kernel
against the net-next tree.

> BTW, given the commit message, I didn't really understand why?

M2, I thought it was unsuccessful commit message and made a comment to
the maintainer, he didn't accept it.

Or.

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Guenter Roeck]

On 05/29/2015 08:51 AM, Or Gerlitz wrote:
> On Fri, May 29, 2015 at 6:38 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
>> Hi,
>>
>> ----- On May 29, 2015, at 11:24 AM, Or Gerlitz gerlitz.or@gmail.com wrote:
>>
>>> On Fri, May 29, 2015 at 12:37 AM, Vivien Didelot
>>> <vivien.didelot@savoirfairelinux.com> wrote:
>>>> @@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
>>>> *parent,
>>>>          if (slave_dev == NULL)
>>>>                  return -ENOMEM;
>>>>
>>>> -       slave_dev->features = master->vlan_features;
>>>> +       slave_dev->features = master->vlan_features |
>>>> +               NETIF_F_VLAN_FEATURES |
>>>> +               NETIF_F_HW_SWITCH_OFFLOAD;
>>>
>>> wait... didn't commit 7889cbee8357aaed85898d028829dfb4f75bae2c  remove
>>> NETIF_F_HW_SWITCH_OFFLOAD?
>>
>> Indeed, note that this RFC is based on v4.1-rc3. This will become unneeded I guess.
>
>
> You should rebase networking patches proposed for the next kernel
> against the net-next tree.
>
>> BTW, given the commit message, I didn't really understand why?
>
> M2, I thought it was unsuccessful commit message and made a comment to
> the maintainer, he didn't accept it.
>
Vivien,

sorry for asking for an early set of your patches. Obviously the idea was not
to create trouble for anyone :-(. I wasn't aware that netdev only accepts patches
which apply to the latest net-next, even if sent as RFC. My fault, I guess.

Maybe next time we can share patches in private first if we have a similar
situation ?

Thanks,
Guenter

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Vivien Didelot]

On May 29, 2015, at 6:15 PM, Guenter Roeck linux@roeck-us.net wrote:
> On 05/29/2015 08:51 AM, Or Gerlitz wrote:
>> On Fri, May 29, 2015 at 6:38 PM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
>>> Hi,
>>>
>>> ----- On May 29, 2015, at 11:24 AM, Or Gerlitz gerlitz.or@gmail.com wrote:
>>>
>>>> On Fri, May 29, 2015 at 12:37 AM, Vivien Didelot
>>>> <vivien.didelot@savoirfairelinux.com> wrote:
>>>>> @@ -854,7 +922,9 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
>>>>> *parent,
>>>>>          if (slave_dev == NULL)
>>>>>                  return -ENOMEM;
>>>>>
>>>>> -       slave_dev->features = master->vlan_features;
>>>>> +       slave_dev->features = master->vlan_features |
>>>>> +               NETIF_F_VLAN_FEATURES |
>>>>> +               NETIF_F_HW_SWITCH_OFFLOAD;
>>>>
>>>> wait... didn't commit 7889cbee8357aaed85898d028829dfb4f75bae2c  remove
>>>> NETIF_F_HW_SWITCH_OFFLOAD?
>>>
>>> Indeed, note that this RFC is based on v4.1-rc3. This will become unneeded I
>>> guess.
>>
>>
>> You should rebase networking patches proposed for the next kernel
>> against the net-next tree.
>>
>>> BTW, given the commit message, I didn't really understand why?
>>
>> M2, I thought it was unsuccessful commit message and made a comment to
>> the maintainer, he didn't accept it.
>>
> Vivien,
> 
> sorry for asking for an early set of your patches. Obviously the idea was not
> to create trouble for anyone :-(. I wasn't aware that netdev only accepts
> patches
> which apply to the latest net-next, even if sent as RFC. My fault, I guess.
> 
> Maybe next time we can share patches in private first if we have a similar
> situation ?

Guenter,

I agree. Maybe sending this to netdev too was a bad idea, my bad.

Scott,

Can I ask details about this NETIF_F_HW_SWITCH_OFFLOAD flag? Why is it
unneeded?

Thanks,
-v

Re: [RFC 1/3] net: dsa: add basic support for VLAN ndo

[Scott Feldman]

On Fri, May 29, 2015 at 3:57 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:

> Scott,
>
> Can I ask details about this NETIF_F_HW_SWITCH_OFFLOAD flag? Why is it
> unneeded?

Sorry, the commit message for that changes wasn't very good because it
didn't capture the email discussion leading up.

The issue I had with the flag was it could be set/cleared at any time
and it wasn't clear what the driver should do in response.

Let's say flag is on for swp1, on startup.  swp1 driver offloads L2/L3
to the device.  Next user clears flag with ethtool.  What does that
mean?  That existing offloads should be removed from device?  Or just
stop adding/removing offloads while flag is off?  Stopping offloads
can cause problems for L3 offload by breaking LPM rules.  (You could
end up with a shorter prefix route in HW and a longer prefix route in
SW, and the HW route will win, which would be bad).

So Roopa wants to revisit the flag.  I might be that tightening up
when the flag can be set/cleared will fix the sequencing issues.

-scott

[RFC 2/3] net: dsa: mv88e6xxx: add support for VTU operations

[Vivien Didelot]

This commit implements the port_vlan_add, port_vlan_kill, and
port_bridge_setlink dsa_switch_driver functions to access the VTU, and
thus add support for adding, removing VLANs, and joining ports to them.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 309 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  28 ++++
 2 files changed, 337 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index cf309aa9..2f4c99f 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2,6 +2,9 @@
  * net/dsa/mv88e6xxx.c - Marvell 88e6xxx switch chip support
  * Copyright (c) 2008 Marvell Semiconductor
  *
+ * Copyright (c) 2015 CMC Electronics, Inc.
+ *	Added support for 802.1q VTU operations
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -1241,6 +1244,312 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 	}
 }
 
+static int _mv88e6xxx_vtu_wait(struct dsa_switch *ds)
+{
+	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_VTU_OP,
+			       GLOBAL_VTU_OP_BUSY);
+}
+
+static int _mv88e6xxx_vtu_cmd(struct dsa_switch *ds, u16 op)
+{
+	int ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_OP, op);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_wait(ds);
+}
+
+static int _mv88e6xxx_stu_loadpurge(struct dsa_switch *ds, u8 sid, bool valid)
+{
+	int ret, data;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	data = sid & GLOBAL_VTU_SID_MASK;
+	if (valid)
+		data |= GLOBAL_VTU_VID_VALID;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID, data);
+	if (ret < 0)
+		return ret;
+
+	/* Unused (yet) data registers */
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_8_11, 0);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_STU_LOAD_PURGE);
+}
+
+static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
+				  struct mv88e6xxx_vtu_entry *entry)
+{
+	int ret, i;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
+				   vid & GLOBAL_VTU_VID_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
+	if (ret < 0)
+		return ret;
+
+	entry->vid = ret & GLOBAL_VTU_VID_MASK;
+	entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
+
+	if (entry->valid) {
+		/* Ports 0-3, offsets 0, 4, 8, 12 */
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < 4; ++i)
+			entry->tags[i] = (ret >> (i * 4)) & 3;
+
+		/* Ports 4-6, offsets 0, 4, 8 */
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
+		if (ret < 0)
+			return ret;
+
+		for (i = 4; i < 7; ++i)
+			entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
+
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
+		if (ret < 0)
+			return ret;
+
+		entry->fid = ret & GLOBAL_VTU_FID_MASK;
+
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
+		if (ret < 0)
+			return ret;
+
+		entry->sid = ret & GLOBAL_VTU_SID_MASK;
+	}
+
+	return 0;
+}
+
+static int _mv88e6xxx_vtu_loadpurge(struct dsa_switch *ds,
+				    struct mv88e6xxx_vtu_entry *entry)
+{
+	u16 data = 0;
+	int ret, i;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	if (entry->valid) {
+		/* Set Data Register, ports 0-3, offsets 0, 4, 8, 12 */
+		for (data = i = 0; i < 4; ++i)
+			data |= entry->tags[i] << (i * 4);
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3,
+					   data);
+		if (ret < 0)
+			return ret;
+
+		/* Set Data Register, ports 4-6, offsets 0, 4, 8 */
+		for (data = 0, i = 4; i < 7; ++i)
+			data |= entry->tags[i] << ((i - 4) * 4);
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7,
+					   data);
+		if (ret < 0)
+			return ret;
+
+		/* Unused Data Register */
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_8_11,
+					   0);
+		if (ret < 0)
+			return ret;
+
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_SID,
+					   entry->sid & GLOBAL_VTU_SID_MASK);
+		if (ret < 0)
+			return ret;
+
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_FID,
+					   entry->fid & GLOBAL_VTU_FID_MASK);
+		if (ret < 0)
+			return ret;
+
+		/* Valid bit set means load, unset means purge */
+		data = GLOBAL_VTU_VID_VALID;
+	}
+
+	data |= entry->vid & GLOBAL_VTU_VID_MASK;
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID, data);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_LOAD_PURGE);
+}
+
+int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_entry entry = { 0 };
+	int prev_vid = vid ? vid - 1 : 4095;
+	int i, ret;
+
+	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
+	if (!vid)
+		return 0;
+
+	/* The DSA port-based VLAN setup reserves FID 0 to DSA_MAX_PORTS;
+	 * we will use the next FIDs for 802.1q;
+	 * thus, forbid the last DSA_MAX_PORTS VLANs.
+	 */
+	if (vid > 4095 - DSA_MAX_PORTS)
+		return -EINVAL;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	/* If the VLAN does not exist, re-initialize the entry for addition */
+	if (entry.vid != vid || !entry.valid) {
+		memset(&entry, 0, sizeof(entry));
+		entry.valid = true;
+		entry.vid = vid;
+		entry.fid = DSA_MAX_PORTS + vid;
+		entry.sid = 0; /* We don't use 802.1s (yet) */
+
+		/* A VTU entry must have a valid STU entry (undocumented).
+		 * The default STU pointer for a VTU entry is 0. If per VLAN
+		 * spanning tree is not used then only one STU entry is needed
+		 * to cover all VTU entries. Thus, validate the STU entry 0.
+		 */
+		ret = _mv88e6xxx_stu_loadpurge(ds, 0, true);
+		if (ret < 0)
+			goto unlock;
+
+		for (i = 0; i < ps->num_ports; ++i)
+			entry.tags[i] = dsa_is_cpu_port(ds, i) ?
+				GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED :
+				GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER;
+	}
+
+	entry.tags[port] = GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED;
+
+	ret = _mv88e6xxx_vtu_loadpurge(ds, &entry);
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
+int mv88e6xxx_port_vlan_kill(struct dsa_switch *ds, int port, u16 vid)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_entry entry = { 0 };
+	int i, ret, prev_vid = vid ? vid - 1 : 4095;
+	bool keep = false;
+
+	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
+	if (!vid)
+		return 0;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	if (entry.vid != vid || !entry.valid) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	entry.tags[port] = GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER;
+
+	/* keep the VLAN unless all ports are excluded */
+	for (i = 0; i < ps->num_ports; ++i) {
+		if (dsa_is_cpu_port(ds, i))
+			continue;
+
+		if (entry.tags[i] != GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) {
+			keep = true;
+			break;
+		}
+	}
+
+	entry.valid = keep;
+	ret = _mv88e6xxx_vtu_loadpurge(ds, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	if (!keep)
+		ret = _mv88e6xxx_update_bridge_config(ds, entry.fid);
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
+int mv88e6xxx_port_bridge_setlink(struct dsa_switch *ds, int port,
+				  struct bridge_vlan_info *vinfo)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_entry entry = { 0 };
+	int prev_vid = vinfo->vid > 0 ? vinfo->vid - 1 : 4095;
+	int ret;
+
+	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
+	if (!vinfo->vid)
+		return 0;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	if (entry.vid != vinfo->vid || !entry.valid) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	/* Set port default VID */
+	if (vinfo->flags & BRIDGE_VLAN_INFO_PVID) {
+		ret = _mv88e6xxx_reg_write(ds, REG_PORT(port),
+					   PORT_DEFAULT_VLAN,
+					   vinfo->vid & PORT_DEFAULT_VLAN_MASK);
+		if (ret < 0)
+			goto unlock;
+	}
+
+	/* Update VTU entry with the new port membership */
+	entry.tags[port] = vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED ?
+		GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED :
+		GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED;
+	ret = _mv88e6xxx_vtu_loadpurge(ds, &entry);
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
 int mv88e6xxx_setup_port_common(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index e045154..383b17af 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -73,6 +73,7 @@
 #define PORT_CONTROL_1		0x05
 #define PORT_BASE_VLAN		0x06
 #define PORT_DEFAULT_VLAN	0x07
+#define PORT_DEFAULT_VLAN_MASK	0xfff
 #define PORT_CONTROL_2		0x08
 #define PORT_RATE_CONTROL	0x09
 #define PORT_RATE_CONTROL_2	0x0a
@@ -96,6 +97,10 @@
 #define GLOBAL_MAC_01		0x01
 #define GLOBAL_MAC_23		0x02
 #define GLOBAL_MAC_45		0x03
+#define GLOBAL_VTU_FID		0x02 /* 6352 only? */
+#define GLOBAL_VTU_FID_MASK	0xfff
+#define GLOBAL_VTU_SID		0x03 /* 6352 only? */
+#define GLOBAL_VTU_SID_MASK	0x3f
 #define GLOBAL_CONTROL		0x04
 #define GLOBAL_CONTROL_SW_RESET		BIT(15)
 #define GLOBAL_CONTROL_PPU_ENABLE	BIT(14)
@@ -112,9 +117,20 @@
 #define GLOBAL_CONTROL_TCAM_EN		BIT(1)
 #define GLOBAL_CONTROL_EEPROM_DONE_EN	BIT(0)
 #define GLOBAL_VTU_OP		0x05
+#define GLOBAL_VTU_OP_BUSY	BIT(15)
+#define GLOBAL_VTU_OP_FLUSH_ALL		((1 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_VTU_LOAD_PURGE	((3 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_VTU_GET_NEXT	((4 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_STU_LOAD_PURGE	((5 << 12) | GLOBAL_VTU_OP_BUSY)
 #define GLOBAL_VTU_VID		0x06
+#define GLOBAL_VTU_VID_MASK	0xfff
+#define GLOBAL_VTU_VID_VALID	BIT(12)
 #define GLOBAL_VTU_DATA_0_3	0x07
 #define GLOBAL_VTU_DATA_4_7	0x08
+#define GLOBAL_VTU_DATA_MEMBER_TAG_UNMODIFIED	0
+#define GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED	1
+#define GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED	2
+#define GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER	3
 #define GLOBAL_VTU_DATA_8_11	0x09
 #define GLOBAL_ATU_CONTROL	0x0a
 #define GLOBAL_ATU_OP		0x0b
@@ -205,6 +221,14 @@
 #define GLOBAL2_QOS_WEIGHT	0x1c
 #define GLOBAL2_MISC		0x1d
 
+struct mv88e6xxx_vtu_entry {
+	u16	vid;
+	u16	fid;
+	u8	sid;
+	bool	valid;
+	u8	tags[DSA_MAX_PORTS];
+};
+
 struct mv88e6xxx_priv_state {
 	/* When using multi-chip addressing, this mutex protects
 	 * access to the indirect access registers.  (In single-chip
@@ -310,6 +334,10 @@ int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port,
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg);
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val);
+int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid);
+int mv88e6xxx_port_vlan_kill(struct dsa_switch *ds,int port, u16 vid);
+int mv88e6xxx_port_bridge_setlink(struct dsa_switch *ds,int port,
+				  struct bridge_vlan_info *vinfo);
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
 extern struct dsa_switch_driver mv88e6352_switch_driver;
-- 
2.4.1

Re: [RFC 2/3] net: dsa: mv88e6xxx: add support for VTU operations

[Guenter Roeck]

Hi Vivien,

On 05/28/2015 02:37 PM, Vivien Didelot wrote:
> This commit implements the port_vlan_add, port_vlan_kill, and
> port_bridge_setlink dsa_switch_driver functions to access the VTU, and
> thus add support for adding, removing VLANs, and joining ports to them.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Pretty much similar to what I have, except for the stu part
which I have completely missing, and I use the default port FIDs.

I wonder if we can really use 'fid = DSA_MAX_PORTS + vid'.
Problem I see is that there may be multiple bridge groups on a switch,
and some ports may not be part of a bridge group. If the same fid is used
for the same vid on multiple ports which belong to different bridge groups,
don't we get into trouble with the address database ?

My assumption was that we have to use a separate fid for each vid per
bridge group (or port if the port is not in a bridge group) if 802.1s
is used, and that we should be able to use the port fid otherwise.
Is that wrong ?

When reporting addresses, how do we associate fdb entries to a vid ?
Or, in other words, how do we handle the fdb entries associated
with a fid in __mv88e6xxx_port_getnext ?

Thanks,
Guenter

[RFC 3/3] net: dsa: mv88e6352: add support for VLAN

[Vivien Didelot]

This commit adds support for the VTU operations to the mv88e6352 driver.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6352.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 8b0d54f..8396a2e 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -554,6 +554,9 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.fdb_add		= mv88e6xxx_port_fdb_add,
 	.fdb_del		= mv88e6xxx_port_fdb_del,
 	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
+	.port_vlan_add		= mv88e6xxx_port_vlan_add,
+	.port_vlan_kill		= mv88e6xxx_port_vlan_kill,
+	.port_bridge_setlink	= mv88e6xxx_port_bridge_setlink,
 };
 
 MODULE_ALIAS("platform:mv88e6352");
-- 
2.4.1

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Scott Feldman]

On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> This RFC is based on v4.1-rc3.
>
> It is meant to get a glance to the commits responsible to implement the
> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>
> With this support, I am able to create VLANs with (un)tagged ports, setting
> their default VID, from a bridge.
>
> To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:
>
>     ip link add name br0 type bridge
>     [...]
>     ip link set dev swp2 up master br0
>     [...]
>     bridge vlan add vid 400 pvid untagged dev swp2
>     bridge vlan add vid 400 pvid untagged dev swp3
>     bridge vlan add vid 400 dev swp4
>     [...]
>     ip link add link br0 name br0.400 type vlan id 400
>     [...]
>     bridge vlan add dev br0 vid 400 self
>
> The code is currently being rebased to the latest net-next/master.
>
> Seems like the way to go now is through switchdev attr getter/setter...

Indeed, for dsa_slave you should be able to port this to switchdev and
set your ndo_bridge_setlink/dellink handlers to
switchdev_port_bridge_setlink/dellink.  (And also implement the
switchdev ops for vlans).

If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.  The
setlink/dellink callbacks will give the same info (and more, e.g.
pvid, untagged flags) and you'll automatically get support for stacked
drivers, for example if you bonded swp2/3 and then included that bond
in your vlan bridge.  Your commands will be slightly modified: when
adding the vid to the port, specify master and self:

bridge vlan add vid 400 dev swp4 master self

-scott

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Vivien Didelot]

Hi Scott,

On May 29, 2015, at 1:02 AM, Scott Feldman sfeldma@gmail.com wrote:
> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
>> This RFC is based on v4.1-rc3.
>>
>> It is meant to get a glance to the commits responsible to implement the
>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>
>> With this support, I am able to create VLANs with (un)tagged ports, setting
>> their default VID, from a bridge.
>>
>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:
>>
>>     ip link add name br0 type bridge
>>     [...]
>>     ip link set dev swp2 up master br0
>>     [...]
>>     bridge vlan add vid 400 pvid untagged dev swp2
>>     bridge vlan add vid 400 pvid untagged dev swp3
>>     bridge vlan add vid 400 dev swp4
>>     [...]
>>     ip link add link br0 name br0.400 type vlan id 400
>>     [...]
>>     bridge vlan add dev br0 vid 400 self
>>
>> The code is currently being rebased to the latest net-next/master.
>>
>> Seems like the way to go now is through switchdev attr getter/setter...
>
> Indeed, for dsa_slave you should be able to port this to switchdev and
> set your ndo_bridge_setlink/dellink handlers to
> switchdev_port_bridge_setlink/dellink.  (And also implement the
> switchdev ops for vlans).
>
> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.  The
> setlink/dellink callbacks will give the same info (and more, e.g.
> pvid, untagged flags) and you'll automatically get support for stacked
> drivers, for example if you bonded swp2/3 and then included that bond
> in your vlan bridge.  Your commands will be slightly modified: when
> adding the vid to the port, specify master and self:
>
> bridge vlan add vid 400 dev swp4 master self

Thanks a lot! I will send a complete RFC soon.

-v

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Guenter Roeck]

Scott,

On 05/28/2015 10:02 PM, Scott Feldman wrote:
> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
>> This RFC is based on v4.1-rc3.
>>
>> It is meant to get a glance to the commits responsible to implement the
>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>
>> With this support, I am able to create VLANs with (un)tagged ports, setting
>> their default VID, from a bridge.
>>
>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:
>>
>>      ip link add name br0 type bridge
>>      [...]
>>      ip link set dev swp2 up master br0
>>      [...]
>>      bridge vlan add vid 400 pvid untagged dev swp2
>>      bridge vlan add vid 400 pvid untagged dev swp3
>>      bridge vlan add vid 400 dev swp4
>>      [...]
>>      ip link add link br0 name br0.400 type vlan id 400
>>      [...]
>>      bridge vlan add dev br0 vid 400 self
>>
>> The code is currently being rebased to the latest net-next/master.
>>
>> Seems like the way to go now is through switchdev attr getter/setter...
>
> Indeed, for dsa_slave you should be able to port this to switchdev and
> set your ndo_bridge_setlink/dellink handlers to
> switchdev_port_bridge_setlink/dellink.  (And also implement the
> switchdev ops for vlans).
>
> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.  The

Those functions are called from net/8021q/vlan_core.c if a vlan is configured
on or removed from an interface. Does that result in a call to setlink/dellink
as well, even if a switch port is not a member of a bridge group ?

Thanks,
Guenter

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Scott Feldman]

On Fri, May 29, 2015 at 3:42 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> Scott,
>
>
> On 05/28/2015 10:02 PM, Scott Feldman wrote:
>>
>> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
>> <vivien.didelot@savoirfairelinux.com> wrote:
>>>
>>> This RFC is based on v4.1-rc3.
>>>
>>> It is meant to get a glance to the commits responsible to implement the
>>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>>
>>> With this support, I am able to create VLANs with (un)tagged ports,
>>> setting
>>> their default VID, from a bridge.
>>>
>>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2
>>> and
>>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like
>>> this:
>>>
>>>      ip link add name br0 type bridge
>>>      [...]
>>>      ip link set dev swp2 up master br0
>>>      [...]
>>>      bridge vlan add vid 400 pvid untagged dev swp2
>>>      bridge vlan add vid 400 pvid untagged dev swp3
>>>      bridge vlan add vid 400 dev swp4
>>>      [...]
>>>      ip link add link br0 name br0.400 type vlan id 400
>>>      [...]
>>>      bridge vlan add dev br0 vid 400 self
>>>
>>> The code is currently being rebased to the latest net-next/master.
>>>
>>> Seems like the way to go now is through switchdev attr getter/setter...
>>
>>
>> Indeed, for dsa_slave you should be able to port this to switchdev and
>> set your ndo_bridge_setlink/dellink handlers to
>> switchdev_port_bridge_setlink/dellink.  (And also implement the
>> switchdev ops for vlans).
>>
>> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
>> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.  The
>
>
> Those functions are called from net/8021q/vlan_core.c if a vlan is
> configured on or removed from an interface. Does that result in a call to
> setlink/dellink as well, even if a switch port is not a member of a bridge group ?

No, not if you're using 8021q module for vlan setup on non-bridged
ports.  If using 8021q module, you'll need to retain the ndo ops.

The alternative to the ndo ops for non-bridged ports is to use the
bridge_setlink/dellink interface on self:

  bridge vlan add vid VID dev DEV self

The would call into DEV port's bridge_setlink to add VID to port.  The
driver would setup HW to ingress VID tagged frames and egress VID
tagged frames on DEV port.

Lots of ways to slice and dice this I guess.

-scott

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Guenter Roeck]

On 05/31/2015 09:48 AM, Scott Feldman wrote:
> On Fri, May 29, 2015 at 3:42 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> Scott,
>>
>>
>> On 05/28/2015 10:02 PM, Scott Feldman wrote:
>>>
>>> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
>>> <vivien.didelot@savoirfairelinux.com> wrote:
>>>>
>>>> This RFC is based on v4.1-rc3.
>>>>
>>>> It is meant to get a glance to the commits responsible to implement the
>>>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>>>
>>>> With this support, I am able to create VLANs with (un)tagged ports,
>>>> setting
>>>> their default VID, from a bridge.
>>>>
>>>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2
>>>> and
>>>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like
>>>> this:
>>>>
>>>>       ip link add name br0 type bridge
>>>>       [...]
>>>>       ip link set dev swp2 up master br0
>>>>       [...]
>>>>       bridge vlan add vid 400 pvid untagged dev swp2
>>>>       bridge vlan add vid 400 pvid untagged dev swp3
>>>>       bridge vlan add vid 400 dev swp4
>>>>       [...]
>>>>       ip link add link br0 name br0.400 type vlan id 400
>>>>       [...]
>>>>       bridge vlan add dev br0 vid 400 self
>>>>
>>>> The code is currently being rebased to the latest net-next/master.
>>>>
>>>> Seems like the way to go now is through switchdev attr getter/setter...
>>>
>>>
>>> Indeed, for dsa_slave you should be able to port this to switchdev and
>>> set your ndo_bridge_setlink/dellink handlers to
>>> switchdev_port_bridge_setlink/dellink.  (And also implement the
>>> switchdev ops for vlans).
>>>
>>> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
>>> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.  The
>>
>>
>> Those functions are called from net/8021q/vlan_core.c if a vlan is
>> configured on or removed from an interface. Does that result in a call to
>> setlink/dellink as well, even if a switch port is not a member of a bridge group ?
>
> No, not if you're using 8021q module for vlan setup on non-bridged
> ports.  If using 8021q module, you'll need to retain the ndo ops.
>
> The alternative to the ndo ops for non-bridged ports is to use the
> bridge_setlink/dellink interface on self:
>
>    bridge vlan add vid VID dev DEV self
>
> The would call into DEV port's bridge_setlink to add VID to port.  The
> driver would setup HW to ingress VID tagged frames and egress VID
> tagged frames on DEV port.
>

Hi Scott,

If I understand you correctly, that means we would expect users to
use bridge commands even on non-bridged dsa ports. I don't think we can
make this kind of assumption. Users will expect configure VLANs on
non-bridge ports as they would normally configure VLANs, using the 8021q
module.

So I guess we'll have to support the ndo ops for dsa.

Thanks,
Guenter

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Scott Feldman]

On Sun, May 31, 2015 at 10:06 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 05/31/2015 09:48 AM, Scott Feldman wrote:
>>
>> On Fri, May 29, 2015 at 3:42 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> Scott,
>>>
>>>
>>> On 05/28/2015 10:02 PM, Scott Feldman wrote:
>>>>
>>>>
>>>> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
>>>> <vivien.didelot@savoirfairelinux.com> wrote:
>>>>>
>>>>>
>>>>> This RFC is based on v4.1-rc3.
>>>>>
>>>>> It is meant to get a glance to the commits responsible to implement the
>>>>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>>>>
>>>>> With this support, I am able to create VLANs with (un)tagged ports,
>>>>> setting
>>>>> their default VID, from a bridge.
>>>>>
>>>>> To create a bridge containing all switch ports, with a VLAN ID 400,
>>>>> swp2
>>>>> and
>>>>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like
>>>>> this:
>>>>>
>>>>>       ip link add name br0 type bridge
>>>>>       [...]
>>>>>       ip link set dev swp2 up master br0
>>>>>       [...]
>>>>>       bridge vlan add vid 400 pvid untagged dev swp2
>>>>>       bridge vlan add vid 400 pvid untagged dev swp3
>>>>>       bridge vlan add vid 400 dev swp4
>>>>>       [...]
>>>>>       ip link add link br0 name br0.400 type vlan id 400
>>>>>       [...]
>>>>>       bridge vlan add dev br0 vid 400 self
>>>>>
>>>>> The code is currently being rebased to the latest net-next/master.
>>>>>
>>>>> Seems like the way to go now is through switchdev attr getter/setter...
>>>>
>>>>
>>>>
>>>> Indeed, for dsa_slave you should be able to port this to switchdev and
>>>> set your ndo_bridge_setlink/dellink handlers to
>>>> switchdev_port_bridge_setlink/dellink.  (And also implement the
>>>> switchdev ops for vlans).
>>>>
>>>> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
>>>> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.  The
>>>
>>>
>>>
>>> Those functions are called from net/8021q/vlan_core.c if a vlan is
>>> configured on or removed from an interface. Does that result in a call to
>>> setlink/dellink as well, even if a switch port is not a member of a
>>> bridge group ?
>>
>>
>> No, not if you're using 8021q module for vlan setup on non-bridged
>> ports.  If using 8021q module, you'll need to retain the ndo ops.
>>
>> The alternative to the ndo ops for non-bridged ports is to use the
>> bridge_setlink/dellink interface on self:
>>
>>    bridge vlan add vid VID dev DEV self
>>
>> The would call into DEV port's bridge_setlink to add VID to port.  The
>> driver would setup HW to ingress VID tagged frames and egress VID
>> tagged frames on DEV port.
>>
>
> Hi Scott,
>
> If I understand you correctly, that means we would expect users to
> use bridge commands even on non-bridged dsa ports. I don't think we can
> make this kind of assumption. Users will expect configure VLANs on
> non-bridge ports as they would normally configure VLANs, using the 8021q
> module.
>
> So I guess we'll have to support the ndo ops for dsa.

I think that's fine.  There is flexibility here.  Using the "bridge"
command for non-bridged ports is a little weird.  You'll still need
setlink to get the PVID/untagged flags, for the bridged-port cases, as
was done in the original RFC patch.

I wonder if a new command "vlan" for the iproute2 pkg would be useful?
 It would absorb the vconfig command options, making vconfig obsolete.
Some of vconfig functionality is already in iproute2, for example "ip
link add link ... type vlan ..."

-scott

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Florian Fainelli]

On 31/05/15 14:21, Scott Feldman wrote:
>> Hi Scott,
>>
>> If I understand you correctly, that means we would expect users to
>> use bridge commands even on non-bridged dsa ports. I don't think we can
>> make this kind of assumption. Users will expect configure VLANs on
>> non-bridge ports as they would normally configure VLANs, using the 8021q
>> module.
>>
>> So I guess we'll have to support the ndo ops for dsa.
> 
> I think that's fine.  There is flexibility here.  Using the "bridge"
> command for non-bridged ports is a little weird.  You'll still need
> setlink to get the PVID/untagged flags, for the bridged-port cases, as
> was done in the original RFC patch.
> 
> I wonder if a new command "vlan" for the iproute2 pkg would be useful?
>  It would absorb the vconfig command options, making vconfig obsolete.
> Some of vconfig functionality is already in iproute2, for example "ip
> link add link ... type vlan ..."

I would definitively like to see that. Right now, this is a little
confusing for users to realize that using bridge + VLAN filtering is a
lot more powerful to configure a switch rather than using
vconfig/iproute add type vlan.
-- 
Florian

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Vivien Didelot]

On May 29, 2015, at 1:02 AM, Scott Feldman sfeldma@gmail.com wrote:
> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
>> This RFC is based on v4.1-rc3.
>>
>> It is meant to get a glance to the commits responsible to implement the
>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>
>> With this support, I am able to create VLANs with (un)tagged ports, setting
>> their default VID, from a bridge.
>>
>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:
>>
>>     ip link add name br0 type bridge
>>     [...]
>>     ip link set dev swp2 up master br0
>>     [...]
>>     bridge vlan add vid 400 pvid untagged dev swp2
>>     bridge vlan add vid 400 pvid untagged dev swp3
>>     bridge vlan add vid 400 dev swp4
>>     [...]
>>     ip link add link br0 name br0.400 type vlan id 400
>>     [...]
>>     bridge vlan add dev br0 vid 400 self
>>
>> The code is currently being rebased to the latest net-next/master.
>>
>> Seems like the way to go now is through switchdev attr getter/setter...
> 
> Indeed, for dsa_slave you should be able to port this to switchdev and
> set your ndo_bridge_setlink/dellink handlers to
> switchdev_port_bridge_setlink/dellink.  (And also implement the
> switchdev ops for vlans).
> 
> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.

Scott,

In fact I have to define these ndo, otherwise I get the "Buggy VLAN
acceleration in driver!" warning from net/core/dev.c and the switch
ports won't register.

I'm actually defining a noop function for them in dsa_slave_netdev_ops.

Is it correct to set NETIF_F_HW_VLAN_CTAG_FILTER in slave_dev->features?

> The setlink/dellink callbacks will give the same info (and more, e.g.
> pvid, untagged flags) and you'll automatically get support for stacked
> drivers, for example if you bonded swp2/3 and then included that bond
> in your vlan bridge.  Your commands will be slightly modified: when
> adding the vid to the port, specify master and self:
> 
> bridge vlan add vid 400 dev swp4 master self

Thanks it works! Now the switch VLAN database is consistent with the
bridge commands, I'm sending a complete RFC very soon.

Scott, David,

I use this mail to expose a potential problem between iproute2 and the
kernel, found with my previous code. When issuing "ip link set dev swp0
master br0", ndo_vlan_rx_add_vid is called, but not ndo_bridge_setlink,
which results in an inconsistency between my switch VLAN database (and
port settings) and "bridge vlan", which shows "swp0   1 PVID Egress
Untagged".

Seems like there is a call to ndo_bridge_setlink to add somewhere, but I
have no clue where.

In the meantime, I call "bridge vlan add vid 1 dev swp0 pvid untagged
[master self]" at boot, to be consistent with the bridge output.

Thanks,
-v

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Scott Feldman]

On Mon, Jun 1, 2015 at 5:18 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
>
> On May 29, 2015, at 1:02 AM, Scott Feldman sfeldma@gmail.com wrote:
>> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
>>> This RFC is based on v4.1-rc3.
>>>
>>> It is meant to get a glance to the commits responsible to implement the
>>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>>
>>> With this support, I am able to create VLANs with (un)tagged ports, setting
>>> their default VID, from a bridge.
>>>
>>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
>>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:
>>>
>>>     ip link add name br0 type bridge
>>>     [...]
>>>     ip link set dev swp2 up master br0
>>>     [...]
>>>     bridge vlan add vid 400 pvid untagged dev swp2
>>>     bridge vlan add vid 400 pvid untagged dev swp3
>>>     bridge vlan add vid 400 dev swp4
>>>     [...]
>>>     ip link add link br0 name br0.400 type vlan id 400
>>>     [...]
>>>     bridge vlan add dev br0 vid 400 self
>>>
>>> The code is currently being rebased to the latest net-next/master.
>>>
>>> Seems like the way to go now is through switchdev attr getter/setter...
>>
>> Indeed, for dsa_slave you should be able to port this to switchdev and
>> set your ndo_bridge_setlink/dellink handlers to
>> switchdev_port_bridge_setlink/dellink.  (And also implement the
>> switchdev ops for vlans).
>>
>> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
>> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.
>
> Scott,
>
> In fact I have to define these ndo, otherwise I get the "Buggy VLAN
> acceleration in driver!" warning from net/core/dev.c and the switch
> ports won't register.
>
> I'm actually defining a noop function for them in dsa_slave_netdev_ops.
>
> Is it correct to set NETIF_F_HW_VLAN_CTAG_FILTER in slave_dev->features?

If your nooping ndo VLAN ops then just remove setting
NETIF_F_HW_VLAN_CTAG_FILTER and then you can remove the noop funcs.

>> The setlink/dellink callbacks will give the same info (and more, e.g.
>> pvid, untagged flags) and you'll automatically get support for stacked
>> drivers, for example if you bonded swp2/3 and then included that bond
>> in your vlan bridge.  Your commands will be slightly modified: when
>> adding the vid to the port, specify master and self:
>>
>> bridge vlan add vid 400 dev swp4 master self
>
> Thanks it works! Now the switch VLAN database is consistent with the
> bridge commands, I'm sending a complete RFC very soon.
>
> Scott, David,
>
> I use this mail to expose a potential problem between iproute2 and the
> kernel, found with my previous code. When issuing "ip link set dev swp0
> master br0", ndo_vlan_rx_add_vid is called, but not ndo_bridge_setlink,

Remove NETIF_F_HW_VLAN_CTAG_FILTER and ndo_vlan_rx_add_vid will not be called.

Issuing "ip link set dev swp0 master br0" should only be setting the
bridge member, not setting up any VLAN.  I suspect when you did this
swp0 was admin UP and you're getting untagged VLAN 0 installed, which
is the call to ndo_vlan_rx_add_vid.

> which results in an inconsistency between my switch VLAN database (and
> port settings) and "bridge vlan", which shows "swp0   1 PVID Egress
> Untagged".

So that is a result of /sys/class/net/br0/bridge/default_pvid set to
1.  If you don't want that, turn default_pvid off:

echo 0 >/sys/class/net/br0/bridge/default_pvid

Now you'll see "None" in the "bridge vlan" output.

> Seems like there is a call to ndo_bridge_setlink to add somewhere, but I
> have no clue where.
>
> In the meantime, I call "bridge vlan add vid 1 dev swp0 pvid untagged
> [master self]" at boot, to be consistent with the bridge output.

Or turn off default_pvid.

Re: [RFC 0/3] DSA and Marvell 88E6352 802.1q support

[Vivien Didelot]

Hi Scott,

On Jun 2, 2015, at 2:18 AM, Scott Feldman sfeldma@gmail.com wrote:
> On Mon, Jun 1, 2015 at 5:18 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
>>
>> On May 29, 2015, at 1:02 AM, Scott Feldman sfeldma@gmail.com wrote:
>>> On Thu, May 28, 2015 at 2:37 PM, Vivien Didelot
>>> <vivien.didelot@savoirfairelinux.com> wrote:
>>>> This RFC is based on v4.1-rc3.
>>>>
>>>> It is meant to get a glance to the commits responsible to implement the
>>>> necessary NDOs between DSA and the Marvell 88E6352 switch driver.
>>>>
>>>> With this support, I am able to create VLANs with (un)tagged ports, setting
>>>> their default VID, from a bridge.
>>>>
>>>> To create a bridge containing all switch ports, with a VLAN ID 400, swp2 and
>>>> swp3 untagged (pvid), and swp4 tagged, the userspace commands look like this:
>>>>
>>>>     ip link add name br0 type bridge
>>>>     [...]
>>>>     ip link set dev swp2 up master br0
>>>>     [...]
>>>>     bridge vlan add vid 400 pvid untagged dev swp2
>>>>     bridge vlan add vid 400 pvid untagged dev swp3
>>>>     bridge vlan add vid 400 dev swp4
>>>>     [...]
>>>>     ip link add link br0 name br0.400 type vlan id 400
>>>>     [...]
>>>>     bridge vlan add dev br0 vid 400 self
>>>>
>>>> The code is currently being rebased to the latest net-next/master.
>>>>
>>>> Seems like the way to go now is through switchdev attr getter/setter...
>>>
>>> Indeed, for dsa_slave you should be able to port this to switchdev and
>>> set your ndo_bridge_setlink/dellink handlers to
>>> switchdev_port_bridge_setlink/dellink.  (And also implement the
>>> switchdev ops for vlans).
>>>
>>> If you use switchdev_port_bridge_setlink/dellink, you shouldn't need
>>> to implement ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid at all.
>>
>> Scott,
>>
>> In fact I have to define these ndo, otherwise I get the "Buggy VLAN
>> acceleration in driver!" warning from net/core/dev.c and the switch
>> ports won't register.
>>
>> I'm actually defining a noop function for them in dsa_slave_netdev_ops.
>>
>> Is it correct to set NETIF_F_HW_VLAN_CTAG_FILTER in slave_dev->features?
> 
> If your nooping ndo VLAN ops then just remove setting
> NETIF_F_HW_VLAN_CTAG_FILTER and then you can remove the noop funcs.
> 
>>> The setlink/dellink callbacks will give the same info (and more, e.g.
>>> pvid, untagged flags) and you'll automatically get support for stacked
>>> drivers, for example if you bonded swp2/3 and then included that bond
>>> in your vlan bridge.  Your commands will be slightly modified: when
>>> adding the vid to the port, specify master and self:
>>>
>>> bridge vlan add vid 400 dev swp4 master self
>>
>> Thanks it works! Now the switch VLAN database is consistent with the
>> bridge commands, I'm sending a complete RFC very soon.
>>
>> Scott, David,
>>
>> I use this mail to expose a potential problem between iproute2 and the
>> kernel, found with my previous code. When issuing "ip link set dev swp0
>> master br0", ndo_vlan_rx_add_vid is called, but not ndo_bridge_setlink,
> 
> Remove NETIF_F_HW_VLAN_CTAG_FILTER and ndo_vlan_rx_add_vid will not be called.
> 
> Issuing "ip link set dev swp0 master br0" should only be setting the
> bridge member, not setting up any VLAN.  I suspect when you did this
> swp0 was admin UP and you're getting untagged VLAN 0 installed, which
> is the call to ndo_vlan_rx_add_vid.
> 
>> which results in an inconsistency between my switch VLAN database (and
>> port settings) and "bridge vlan", which shows "swp0   1 PVID Egress
>> Untagged".
> 
> So that is a result of /sys/class/net/br0/bridge/default_pvid set to
> 1.  If you don't want that, turn default_pvid off:
> 
> echo 0 >/sys/class/net/br0/bridge/default_pvid
> 
> Now you'll see "None" in the "bridge vlan" output.
> 
>> Seems like there is a call to ndo_bridge_setlink to add somewhere, but I
>> have no clue where.
>>
>> In the meantime, I call "bridge vlan add vid 1 dev swp0 pvid untagged
>> [master self]" at boot, to be consistent with the bridge output.
> 
> Or turn off default_pvid.

Thanks, I confirm both fixes work. Thanks a lot.
-v