[PATCH net-next 0/4] net: dsa: pass bridge device to drivers

[PATCH net-next 0/4] net: dsa: pass bridge device to drivers

[Vivien Didelot]

This patchset simplifies the DSA layer.

A switch may support multiple bridges with the same hardware VLAN. Thus a check
such as dsa_bridge_check_vlan_range must be moved from the DSA layer to the
concerned driver.

The first purpose of this patchset is to help moving this check to the
mv88e6xxx driver, which is the only one affected at the moment.

To do that, pass directly the bridge net_device structure down to the DSA
drivers, instead of calculating a bitmask of bridge members.

The second purpose is to prepare the replacement of the complex
port_vlan_getnext approach. A second patchset is ready to follow, implementing
port_vlan_dump and thus simplifying the DSA slave code one more time.

Note that this patchset applies on top of https://lkml.org/lkml/2016/2/5/532.

Vivien Didelot (4):
  net: dsa: mv88e6xxx: add port private structure
  net: dsa: pass bridge down to drivers
  net: dsa: mv88e6xxx: check hardware VLAN in use
  net: dsa: remove dsa_bridge_check_vlan_range

 Documentation/networking/dsa/dsa.txt |  7 +---
 drivers/net/dsa/bcm_sf2.c            | 12 +++---
 drivers/net/dsa/bcm_sf2.h            |  2 +
 drivers/net/dsa/mv88e6xxx.c          | 81 ++++++++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx.h          | 13 ++++--
 include/net/dsa.h                    |  5 +--
 net/dsa/slave.c                      | 81 +-----------------------------------
 7 files changed, 102 insertions(+), 99 deletions(-)

-- 
2.7.1

[PATCH net-next 1/4] net: dsa: mv88e6xxx: add port private structure

[Vivien Didelot]

Add a per-port mv88e6xxx_priv_port structure to store per-port related
data, instead of adding several arrays of DSA_MAX_PORTS elements in the
mv88e6xxx_priv_state structure.

It currently only contains the port STP state.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 4 ++--
 drivers/net/dsa/mv88e6xxx.h | 7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 512c8c0..b0e00ed 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1131,7 +1131,7 @@ int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state)
 	/* mv88e6xxx_port_stp_update may be called with softirqs disabled,
 	 * so we can not update the port state directly but need to schedule it.
 	 */
-	ps->port_state[port] = stp_state;
+	ps->ports[port].state = stp_state;
 	set_bit(port, &ps->port_state_update_mask);
 	schedule_work(&ps->bridge_work);
 
@@ -1925,7 +1925,7 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 	while (ps->port_state_update_mask) {
 		port = __ffs(ps->port_state_update_mask);
 		clear_bit(port, &ps->port_state_update_mask);
-		mv88e6xxx_set_port_state(ds, port, ps->port_state[port]);
+		mv88e6xxx_set_port_state(ds, port, ps->ports[port].state);
 	}
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index ca08f91..63a6f58 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -379,6 +379,10 @@ struct mv88e6xxx_vtu_stu_entry {
 	u8	data[DSA_MAX_PORTS];
 };
 
+struct mv88e6xxx_priv_port {
+	u8 state;
+};
+
 struct mv88e6xxx_priv_state {
 	/* When using multi-chip addressing, this mutex protects
 	 * access to the indirect access registers.  (In single-chip
@@ -415,8 +419,9 @@ struct mv88e6xxx_priv_state {
 	int		id; /* switch product id */
 	int		num_ports;	/* number of switch ports */
 
+	struct mv88e6xxx_priv_port	ports[DSA_MAX_PORTS];
+
 	unsigned long port_state_update_mask;
-	u8 port_state[DSA_MAX_PORTS];
 
 	struct work_struct bridge_work;
 };
-- 
2.7.1

[PATCH net-next 2/4] net: dsa: pass bridge down to drivers

[Vivien Didelot]

Some DSA drivers may or may not support multiple software bridges on top
of an hardware switch.

It is more convenient for them to access the bridge's net_device for
finer configuration.

Removing the need to craft and access a bitmask also simplifies the
code.

This patch changes the signature of bridge related functions, update DSA
drivers, and removes dsa_slave_br_port_mask.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/networking/dsa/dsa.txt |  7 ++-----
 drivers/net/dsa/bcm_sf2.c            | 12 +++++++-----
 drivers/net/dsa/bcm_sf2.h            |  2 ++
 drivers/net/dsa/mv88e6xxx.c          | 13 +++++++++++--
 drivers/net/dsa/mv88e6xxx.h          |  6 ++++--
 include/net/dsa.h                    |  5 ++---
 net/dsa/slave.c                      | 31 ++-----------------------------
 7 files changed, 30 insertions(+), 46 deletions(-)

diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
index aa9c1f9..ebf2153 100644
--- a/Documentation/networking/dsa/dsa.txt
+++ b/Documentation/networking/dsa/dsa.txt
@@ -524,17 +524,14 @@ Bridge layer
 - port_join_bridge: bridge layer function invoked when a given switch port is
   added to a bridge, this function should be doing the necessary at the switch
   level to permit the joining port from being added to the relevant logical
-  domain for it to ingress/egress traffic with other members of the bridge. DSA
-  does nothing but calculate a bitmask of switch ports currently members of the
-  specified bridge being requested the join
+  domain for it to ingress/egress traffic with other members of the bridge.
 
 - port_leave_bridge: bridge layer function invoked when a given switch port is
   removed from a bridge, this function should be doing the necessary at the
   switch level to deny the leaving port from ingress/egress traffic from the
   remaining bridge members. When the port leaves the bridge, it should be aged
   out at the switch hardware for the switch to (re) learn MAC addresses behind
-  this port. DSA calculates the bitmask of ports still members of the bridge
-  being left
+  this port.
 
 - port_stp_update: bridge layer function invoked when a given switch port STP
   state is computed by the bridge layer and should be propagated to switch
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 6f946fe..3f62759 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -483,16 +483,17 @@ static int bcm_sf2_sw_fast_age_port(struct dsa_switch  *ds, int port)
 }
 
 static int bcm_sf2_sw_br_join(struct dsa_switch *ds, int port,
-			      u32 br_port_mask)
+			      struct net_device *bridge)
 {
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
 	unsigned int i;
 	u32 reg, p_ctl;
 
+	priv->port_sts[port].bridge_dev = bridge;
 	p_ctl = core_readl(priv, CORE_PORT_VLAN_CTL_PORT(port));
 
 	for (i = 0; i < priv->hw_params.num_ports; i++) {
-		if (!((1 << i) & br_port_mask))
+		if (priv->port_sts[i].bridge_dev != bridge)
 			continue;
 
 		/* Add this local port to the remote port VLAN control
@@ -515,10 +516,10 @@ static int bcm_sf2_sw_br_join(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port,
-			       u32 br_port_mask)
+static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port)
 {
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
+	struct net_device *bridge = priv->port_sts[port].bridge_dev;
 	unsigned int i;
 	u32 reg, p_ctl;
 
@@ -526,7 +527,7 @@ static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port,
 
 	for (i = 0; i < priv->hw_params.num_ports; i++) {
 		/* Don't touch the remaining ports */
-		if (!((1 << i) & br_port_mask))
+		if (priv->port_sts[i].bridge_dev != bridge)
 			continue;
 
 		reg = core_readl(priv, CORE_PORT_VLAN_CTL_PORT(i));
@@ -541,6 +542,7 @@ static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port,
 
 	core_writel(priv, p_ctl, CORE_PORT_VLAN_CTL_PORT(port));
 	priv->port_sts[port].vlan_ctl_mask = p_ctl;
+	priv->port_sts[port].bridge_dev = NULL;
 
 	return 0;
 }
diff --git a/drivers/net/dsa/bcm_sf2.h b/drivers/net/dsa/bcm_sf2.h
index 6bba1c9..200b1f5 100644
--- a/drivers/net/dsa/bcm_sf2.h
+++ b/drivers/net/dsa/bcm_sf2.h
@@ -50,6 +50,8 @@ struct bcm_sf2_port_status {
 	struct ethtool_eee eee;
 
 	u32 vlan_ctl_mask;
+
+	struct net_device *bridge_dev;
 };
 
 struct bcm_sf2_arl_entry {
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index b0e00ed..2e515e8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1889,13 +1889,22 @@ unlock:
 	return err;
 }
 
-int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port, u32 members)
+int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
+			       struct net_device *bridge)
 {
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+
+	ps->ports[port].bridge_dev = bridge;
+
 	return 0;
 }
 
-int mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port, u32 members)
+int mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
 {
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+
+	ps->ports[port].bridge_dev = NULL;
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 63a6f58..260b491 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -380,6 +380,7 @@ struct mv88e6xxx_vtu_stu_entry {
 };
 
 struct mv88e6xxx_priv_port {
+	struct net_device *bridge_dev;
 	u8 state;
 };
 
@@ -481,8 +482,9 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
 int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
 int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
 		      struct phy_device *phydev, struct ethtool_eee *e);
-int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port, u32 members);
-int mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port, u32 members);
+int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
+			       struct net_device *bridge);
+int mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port);
 int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state);
 int mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_vlan *vlan,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 26a0e86..1c845d7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -297,9 +297,8 @@ struct dsa_switch_driver {
 	 * Bridge integration
 	 */
 	int	(*port_join_bridge)(struct dsa_switch *ds, int port,
-				    u32 br_port_mask);
-	int	(*port_leave_bridge)(struct dsa_switch *ds, int port,
-				     u32 br_port_mask);
+				    struct net_device *bridge);
+	int	(*port_leave_bridge)(struct dsa_switch *ds, int port);
 	int	(*port_stp_update)(struct dsa_switch *ds, int port,
 				   u8 state);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 40b9ca7..755b005 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -385,31 +385,6 @@ static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return -EOPNOTSUPP;
 }
 
-/* Return a bitmask of all ports being currently bridged within a given bridge
- * device. Note that on leave, the mask will still return the bitmask of ports
- * currently bridged, prior to port removal, and this is exactly what we want.
- */
-static u32 dsa_slave_br_port_mask(struct dsa_switch *ds,
-				  struct net_device *bridge)
-{
-	struct dsa_slave_priv *p;
-	unsigned int port;
-	u32 mask = 0;
-
-	for (port = 0; port < DSA_MAX_PORTS; port++) {
-		if (!dsa_is_port_initialized(ds, port))
-			continue;
-
-		p = netdev_priv(ds->ports[port]);
-
-		if (ds->ports[port]->priv_flags & IFF_BRIDGE_PORT &&
-		    p->bridge_dev == bridge)
-			mask |= 1 << port;
-	}
-
-	return mask;
-}
-
 static int dsa_slave_stp_update(struct net_device *dev, u8 state)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
@@ -533,8 +508,7 @@ static int dsa_slave_bridge_port_join(struct net_device *dev,
 	p->bridge_dev = br;
 
 	if (ds->drv->port_join_bridge)
-		ret = ds->drv->port_join_bridge(ds, p->port,
-						dsa_slave_br_port_mask(ds, br));
+		ret = ds->drv->port_join_bridge(ds, p->port, br);
 
 	return ret;
 }
@@ -547,8 +521,7 @@ static int dsa_slave_bridge_port_leave(struct net_device *dev)
 
 
 	if (ds->drv->port_leave_bridge)
-		ret = ds->drv->port_leave_bridge(ds, p->port,
-						 dsa_slave_br_port_mask(ds, p->bridge_dev));
+		ret = ds->drv->port_leave_bridge(ds, p->port);
 
 	p->bridge_dev = NULL;
 
-- 
2.7.1

Re: [PATCH net-next 2/4] net: dsa: pass bridge down to drivers

[Florian Fainelli]

2016-02-12 9:09 GMT-08:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> Some DSA drivers may or may not support multiple software bridges on top
> of an hardware switch.
>
> It is more convenient for them to access the bridge's net_device for
> finer configuration.
>
> Removing the need to craft and access a bitmask also simplifies the
> code.
>
> This patch changes the signature of bridge related functions, update DSA
> drivers, and removes dsa_slave_br_port_mask.

This is the specific patch that depends on the patch you mention in
your cover letter, I applied it anyway, ignored the rejects in the
mv88e6xxx driver since I wanted to test bcm_sf2 only and the hardware
is still programmed the same way before and after:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
--
Florian

[PATCH net-next 3/4] net: dsa: mv88e6xxx: check hardware VLAN in use

[Vivien Didelot]

The DSA drivers now have access to the VLAN prepare phase and the bridge
net_device. It is easier to check for overlapping bridges from within
the driver. Thus add such check in mv88e6xxx_port_vlan_prepare.

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 2e515e8..685dcb0 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid,
 	return 0;
 }
 
+static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
+					u16 vid_begin, u16 vid_end)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_stu_entry vlan;
+	int i, err;
+
+	if (!vid_begin)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&ps->smi_mutex);
+
+	err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1);
+	if (err)
+		goto unlock;
+
+	do {
+		err = _mv88e6xxx_vtu_getnext(ds, &vlan);
+		if (err)
+			goto unlock;
+
+		if (!vlan.valid)
+			break;
+
+		if (vlan.vid > vid_end)
+			break;
+
+		for (i = 0; i < ps->num_ports; ++i) {
+			if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
+				continue;
+
+			if (vlan.data[i] ==
+			    GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
+				continue;
+
+			if (ps->ports[i].bridge_dev ==
+			    ps->ports[port].bridge_dev)
+				break; /* same bridge, check next VLAN */
+
+			netdev_warn(ds->ports[port],
+				    "hardware VLAN %d already used by %s\n",
+				    vlan.vid,
+				    netdev_name(ps->ports[i].bridge_dev));
+			err = -EOPNOTSUPP;
+			goto unlock;
+		}
+	} while (vlan.vid < vid_end);
+
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return err;
+}
+
 int mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_vlan *vlan,
 				struct switchdev_trans *trans)
 {
+	int err;
+
 	/* We reserve a few VLANs to isolate unbridged ports */
 	if (vlan->vid_end >= 4000)
 		return -EOPNOTSUPP;
 
+	/* If the requested port doesn't belong to the same bridge as the VLAN
+	 * members, do not support it (yet) and fallback to software VLAN.
+	 */
+	err = mv88e6xxx_port_check_hw_vlan(ds, port, vlan->vid_begin,
+					   vlan->vid_end);
+	if (err)
+		return err;
+
 	/* We don't need any dynamic resource from the kernel (yet),
 	 * so skip the prepare phase.
 	 */
-- 
2.7.1

Re: [PATCH net-next 3/4] net: dsa: mv88e6xxx: check hardware VLAN in use

[Sergei Shtylyov]

Hello.

On 2/12/2016 8:09 PM, Vivien Didelot wrote:

> The DSA drivers now have access to the VLAN prepare phase and the bridge
> net_device. It is easier to check for overlapping bridges from within
> the driver. Thus add such check in mv88e6xxx_port_vlan_prepare.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 64 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 2e515e8..685dcb0 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid,
>   	return 0;
>   }
>
> +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
> +					u16 vid_begin, u16 vid_end)
> +{
> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> +	struct mv88e6xxx_vtu_stu_entry vlan;
> +	int i, err;
> +
> +	if (!vid_begin)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&ps->smi_mutex);
> +
> +	err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1);
> +	if (err)
> +		goto unlock;
> +
> +	do {
> +		err = _mv88e6xxx_vtu_getnext(ds, &vlan);
> +		if (err)
> +			goto unlock;

    Why are you not using *break*?

> +
> +		if (!vlan.valid)
> +			break;
> +
> +		if (vlan.vid > vid_end)
> +			break;
> +
> +		for (i = 0; i < ps->num_ports; ++i) {
> +			if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
> +				continue;
> +
> +			if (vlan.data[i] ==
> +			    GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
> +				continue;
> +
> +			if (ps->ports[i].bridge_dev ==
> +			    ps->ports[port].bridge_dev)
> +				break; /* same bridge, check next VLAN */
> +
> +			netdev_warn(ds->ports[port],
> +				    "hardware VLAN %d already used by %s\n",
> +				    vlan.vid,
> +				    netdev_name(ps->ports[i].bridge_dev));
> +			err = -EOPNOTSUPP;
> +			goto unlock;
> +		}

    Why not *break*?

> +	} while (vlan.vid < vid_end);
> +
> +unlock:
> +	mutex_unlock(&ps->smi_mutex);
> +
> +	return err;
> +}
> +
[...]

MBR, Sergei

Re: [PATCH net-next 3/4] net: dsa: mv88e6xxx: check hardware VLAN in use

[Vivien Didelot]

Hi Sergei,

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:

> Hello.
>
> On 2/12/2016 8:09 PM, Vivien Didelot wrote:
>
>> The DSA drivers now have access to the VLAN prepare phase and the bridge
>> net_device. It is easier to check for overlapping bridges from within
>> the driver. Thus add such check in mv88e6xxx_port_vlan_prepare.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>   drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index 2e515e8..685dcb0 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid,
>>   	return 0;
>>   }
>>
>> +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>> +					u16 vid_begin, u16 vid_end)
>> +{
>> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> +	struct mv88e6xxx_vtu_stu_entry vlan;
>> +	int i, err;
>> +
>> +	if (!vid_begin)
>> +		return -EOPNOTSUPP;
>> +
>> +	mutex_lock(&ps->smi_mutex);
>> +
>> +	err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1);
>> +	if (err)
>> +		goto unlock;
>> +
>> +	do {
>> +		err = _mv88e6xxx_vtu_getnext(ds, &vlan);
>> +		if (err)
>> +			goto unlock;
>
>     Why are you not using *break*?

I use goto for explicit error handling, and break for expected flow.

>> +
>> +		if (!vlan.valid)
>> +			break;
>> +
>> +		if (vlan.vid > vid_end)
>> +			break;
>> +
>> +		for (i = 0; i < ps->num_ports; ++i) {
>> +			if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
>> +				continue;
>> +
>> +			if (vlan.data[i] ==
>> +			    GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
>> +				continue;
>> +
>> +			if (ps->ports[i].bridge_dev ==
>> +			    ps->ports[port].bridge_dev)
>> +				break; /* same bridge, check next VLAN */
>> +
>> +			netdev_warn(ds->ports[port],
>> +				    "hardware VLAN %d already used by %s\n",
>> +				    vlan.vid,
>> +				    netdev_name(ps->ports[i].bridge_dev));
>> +			err = -EOPNOTSUPP;
>> +			goto unlock;
>> +		}
>
>     Why not *break*?

Because here it would only break the for loop, and not the while loop.

>
>> +	} while (vlan.vid < vid_end);
>> +
>> +unlock:
>> +	mutex_unlock(&ps->smi_mutex);
>> +
>> +	return err;
>> +}
>> +
> [...]
>
> MBR, Sergei

Thanks,
-v

Re: [PATCH net-next 3/4] net: dsa: mv88e6xxx: check hardware VLAN in use

[Sergei Shtylyov]

On 02/13/2016 10:53 PM, Vivien Didelot wrote:

>>> The DSA drivers now have access to the VLAN prepare phase and the bridge
>>> net_device. It is easier to check for overlapping bridges from within
>>> the driver. Thus add such check in mv88e6xxx_port_vlan_prepare.
>>>
>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>> ---
>>>    drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>>> index 2e515e8..685dcb0 100644
>>> --- a/drivers/net/dsa/mv88e6xxx.c
>>> +++ b/drivers/net/dsa/mv88e6xxx.c
>>> @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid,
>>>    	return 0;
>>>    }
>>>
>>> +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>>> +					u16 vid_begin, u16 vid_end)
>>> +{
>>> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>> +	struct mv88e6xxx_vtu_stu_entry vlan;
>>> +	int i, err;
>>> +
>>> +	if (!vid_begin)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	mutex_lock(&ps->smi_mutex);
>>> +
>>> +	err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1);
>>> +	if (err)
>>> +		goto unlock;
>>> +
>>> +	do {
>>> +		err = _mv88e6xxx_vtu_getnext(ds, &vlan);
>>> +		if (err)
>>> +			goto unlock;
>>
>>      Why are you not using *break*?
>
> I use goto for explicit error handling, and break for expected flow.

    Thought you'd say so. :-)
    I still think *break* is preferable...

>>> +
>>> +		if (!vlan.valid)
>>> +			break;
>>> +
>>> +		if (vlan.vid > vid_end)
>>> +			break;
>>> +
>>> +		for (i = 0; i < ps->num_ports; ++i) {
>>> +			if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
>>> +				continue;
>>> +
>>> +			if (vlan.data[i] ==
>>> +			    GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
>>> +				continue;
>>> +
>>> +			if (ps->ports[i].bridge_dev ==
>>> +			    ps->ports[port].bridge_dev)
>>> +				break; /* same bridge, check next VLAN */
>>> +
>>> +			netdev_warn(ds->ports[port],
>>> +				    "hardware VLAN %d already used by %s\n",
>>> +				    vlan.vid,
>>> +				    netdev_name(ps->ports[i].bridge_dev));
>>> +			err = -EOPNOTSUPP;
>>> +			goto unlock;
>>> +		}
>>
>>      Why not *break*?
>
> Because here it would only break the for loop, and not the while loop.

    Oops, I overlooked the *for* loop. Sorry about that.

>>
>>> +	} while (vlan.vid < vid_end);
>>> +
>>> +unlock:
>>> +	mutex_unlock(&ps->smi_mutex);
>>> +
>>> +	return err;
>>> +}
>>> +
>> [...]
>
> Thanks,
> -v

MBR, Sergei

[PATCH net-next 4/4] net: dsa: remove dsa_bridge_check_vlan_range

[Vivien Didelot]

DSA drivers may support multiple bridge groups with the same hardware
VLAN. The mv88e6xxx driver which cannot yet, already has its own check
for overlapping bridges. Thus remove the check from the DSA layer.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c | 50 --------------------------------------------------
 1 file changed, 50 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 755b005..bcf383c 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -201,47 +201,6 @@ out:
 	return 0;
 }
 
-static int dsa_bridge_check_vlan_range(struct dsa_switch *ds,
-				       const struct net_device *bridge,
-				       u16 vid_begin, u16 vid_end)
-{
-	struct dsa_slave_priv *p;
-	struct net_device *dev, *vlan_br;
-	DECLARE_BITMAP(members, DSA_MAX_PORTS);
-	DECLARE_BITMAP(untagged, DSA_MAX_PORTS);
-	u16 vid;
-	int member, err;
-
-	if (!ds->drv->vlan_getnext || !vid_begin)
-		return -EOPNOTSUPP;
-
-	vid = vid_begin - 1;
-
-	do {
-		err = ds->drv->vlan_getnext(ds, &vid, members, untagged);
-		if (err)
-			break;
-
-		if (vid > vid_end)
-			break;
-
-		member = find_first_bit(members, DSA_MAX_PORTS);
-		if (member == DSA_MAX_PORTS)
-			continue;
-
-		dev = ds->ports[member];
-		p = netdev_priv(dev);
-		vlan_br = p->bridge_dev;
-		if (vlan_br == bridge)
-			continue;
-
-		netdev_dbg(vlan_br, "hardware VLAN %d already in use\n", vid);
-		return -EOPNOTSUPP;
-	} while (vid < vid_end);
-
-	return err == -ENOENT ? 0 : err;
-}
-
 static int dsa_slave_port_vlan_add(struct net_device *dev,
 				   const struct switchdev_obj_port_vlan *vlan,
 				   struct switchdev_trans *trans)
@@ -254,15 +213,6 @@ static int dsa_slave_port_vlan_add(struct net_device *dev,
 		if (!ds->drv->port_vlan_prepare || !ds->drv->port_vlan_add)
 			return -EOPNOTSUPP;
 
-		/* If the requested port doesn't belong to the same bridge as
-		 * the VLAN members, fallback to software VLAN (hopefully).
-		 */
-		err = dsa_bridge_check_vlan_range(ds, p->bridge_dev,
-						  vlan->vid_begin,
-						  vlan->vid_end);
-		if (err)
-			return err;
-
 		err = ds->drv->port_vlan_prepare(ds, p->port, vlan, trans);
 		if (err)
 			return err;
-- 
2.7.1

Re: [PATCH net-next 0/4] net: dsa: pass bridge device to drivers

[Vivien Didelot]

Hi all,

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> This patchset simplifies the DSA layer.
>
> A switch may support multiple bridges with the same hardware VLAN. Thus a check
> such as dsa_bridge_check_vlan_range must be moved from the DSA layer to the
> concerned driver.
>
> The first purpose of this patchset is to help moving this check to the
> mv88e6xxx driver, which is the only one affected at the moment.
>
> To do that, pass directly the bridge net_device structure down to the DSA
> drivers, instead of calculating a bitmask of bridge members.
>
> The second purpose is to prepare the replacement of the complex
> port_vlan_getnext approach. A second patchset is ready to follow, implementing
> port_vlan_dump and thus simplifying the DSA slave code one more time.
>
> Note that this patchset applies on top of https://lkml.org/lkml/2016/2/5/532.

The dependent fix from net is now merged into net-next, which makes this
patchset apply well. I'm sending the second patchset right away.

Thanks,
-v

Re: [PATCH net-next 0/4] net: dsa: pass bridge device to drivers

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Fri, 12 Feb 2016 12:09:37 -0500

> This patchset simplifies the DSA layer.
> 
> A switch may support multiple bridges with the same hardware VLAN. Thus a check
> such as dsa_bridge_check_vlan_range must be moved from the DSA layer to the
> concerned driver.
> 
> The first purpose of this patchset is to help moving this check to the
> mv88e6xxx driver, which is the only one affected at the moment.
> 
> To do that, pass directly the bridge net_device structure down to the DSA
> drivers, instead of calculating a bitmask of bridge members.
> 
> The second purpose is to prepare the replacement of the complex
> port_vlan_getnext approach. A second patchset is ready to follow, implementing
> port_vlan_dump and thus simplifying the DSA slave code one more time.
> 
> Note that this patchset applies on top of https://lkml.org/lkml/2016/2/5/532.

Series applied, thanks Vivien.