[PATCH net-next v3 0/5] net: dsa: remove .set_addr

[PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Vivien Didelot]

An Ethernet switch may support having a MAC address, which can be used
as the switch's source address in transmitted full-duplex Pause frames.

If a DSA switch supports the related .set_addr operation, the DSA core
sets the master's MAC address on the switch.

This won't make sense anymore in a multi-CPU ports system, because there
won't be a unique master device assigned to a switch tree.

Moreover this operation is confusing because it makes the user think
that it could be used to program the switch with the MAC address of the
CPU/management port such that MAC address learning can be disabled on
said port, but in fact, that's not how it is currently used.

To fix this, assign a random MAC address at setup time in the mv88e6060
and mv88e6xxx drivers before removing .set_addr completely from DSA.

Changes in v3:
  - include fix for mv88e6060 switch MAC address setter.

Changes in v2:
  - remove .set_addr implementation from drivers and use a random MAC.

Vivien Didelot (5):
  net: dsa: mv88e6xxx: setup random mac address
  net: dsa: mv88e6060: fix switch MAC address
  net: dsa: mv88e6060: setup random mac address
  net: dsa: dsa_loop: remove .set_addr
  net: dsa: remove .set_addr

 drivers/net/dsa/dsa_loop.c       |  8 --------
 drivers/net/dsa/mv88e6060.c      | 37 ++++++++++++++++++++++++++-----------
 drivers/net/dsa/mv88e6xxx/chip.c | 33 +++++++++++++++++----------------
 include/net/dsa.h                |  1 -
 net/dsa/dsa2.c                   |  6 ------
 net/dsa/legacy.c                 |  6 ------
 6 files changed, 43 insertions(+), 48 deletions(-)

-- 
2.14.2

[PATCH net-next v3 1/5] net: dsa: mv88e6xxx: setup random mac address

[Vivien Didelot]

An Ethernet switch may support having a MAC address, which can be used
as the switch's source address in transmitted full-duplex Pause frames.

If a DSA switch supports the related .set_addr operation, the DSA core
sets the master's MAC address on the switch. This won't make sense
anymore in a multi-CPU ports system, because there won't be a unique
master device assigned to a switch tree.

Instead, setup the switch from within the Marvell driver with a random
MAC address, and remove the .set_addr implementation.

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d74c7335c512..76cf383dcf90 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -932,6 +932,19 @@ static int mv88e6xxx_irl_setup(struct mv88e6xxx_chip *chip)
 	return 0;
 }
 
+static int mv88e6xxx_mac_setup(struct mv88e6xxx_chip *chip)
+{
+	if (chip->info->ops->set_switch_mac) {
+		u8 addr[ETH_ALEN];
+
+		eth_random_addr(addr);
+
+		return chip->info->ops->set_switch_mac(chip, addr);
+	}
+
+	return 0;
+}
+
 static int mv88e6xxx_pvt_map(struct mv88e6xxx_chip *chip, int dev, int port)
 {
 	u16 pvlan = 0;
@@ -2013,6 +2026,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	if (err)
 		goto unlock;
 
+	err = mv88e6xxx_mac_setup(chip);
+	if (err)
+		goto unlock;
+
 	err = mv88e6xxx_phy_setup(chip);
 	if (err)
 		goto unlock;
@@ -2043,21 +2060,6 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
-static int mv88e6xxx_set_addr(struct dsa_switch *ds, u8 *addr)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-	int err;
-
-	if (!chip->info->ops->set_switch_mac)
-		return -EOPNOTSUPP;
-
-	mutex_lock(&chip->reg_lock);
-	err = chip->info->ops->set_switch_mac(chip, addr);
-	mutex_unlock(&chip->reg_lock);
-
-	return err;
-}
-
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
@@ -3785,7 +3787,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.probe			= mv88e6xxx_drv_probe,
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
 	.setup			= mv88e6xxx_setup,
-	.set_addr		= mv88e6xxx_set_addr,
 	.adjust_link		= mv88e6xxx_adjust_link,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
-- 
2.14.2

[PATCH net-next v3 2/5] net: dsa: mv88e6060: fix switch MAC address

[Vivien Didelot]

The 88E6060 Ethernet switch always transmits the multicast bit of the
switch MAC address as a zero. It re-uses the corresponding bit 8 of the
register "Switch MAC Address Register Bytes 0 & 1" for "DiffAddr".

If the "DiffAddr" bit is 0, then all ports transmit the same source
address. If it is set to 1, then bit 2:0 are used for the port number.

The mv88e6060 driver is currently wrongly shifting the MAC address byte
0 by 9. To fix this, shift it by 8 as usual and clear its bit 0.

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

diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index 621cdc46ad81..d64be2b83d3c 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -214,8 +214,14 @@ static int mv88e6060_setup(struct dsa_switch *ds)
 
 static int mv88e6060_set_addr(struct dsa_switch *ds, u8 *addr)
 {
-	/* Use the same MAC Address as FD Pause frames for all ports */
-	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, (addr[0] << 9) | addr[1]);
+	u16 val = addr[0] << 8 | addr[1];
+
+	/* The multicast bit is always transmitted as a zero, so the switch uses
+	 * bit 8 for "DiffAddr", where 0 means all ports transmit the same SA.
+	 */
+	val &= 0xfeff;
+
+	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, val);
 	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_23, (addr[2] << 8) | addr[3]);
 	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_45, (addr[4] << 8) | addr[5]);
 
-- 
2.14.2

[PATCH net-next v3 3/5] net: dsa: mv88e6060: setup random mac address

[Vivien Didelot]

As for mv88e6xxx, setup the switch from within the mv88e6060 driver with
a random MAC address, and remove the .set_addr implementation.

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

diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index d64be2b83d3c..6173be889d95 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/etherdevice.h>
 #include <linux/jiffies.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -188,6 +189,27 @@ static int mv88e6060_setup_port(struct dsa_switch *ds, int p)
 	return 0;
 }
 
+static int mv88e6060_setup_addr(struct dsa_switch *ds)
+{
+	u8 addr[ETH_ALEN];
+	u16 val;
+
+	eth_random_addr(addr);
+
+	val = addr[0] << 8 | addr[1];
+
+	/* The multicast bit is always transmitted as a zero, so the switch uses
+	 * bit 8 for "DiffAddr", where 0 means all ports transmit the same SA.
+	 */
+	val &= 0xfeff;
+
+	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, val);
+	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_23, (addr[2] << 8) | addr[3]);
+	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_45, (addr[4] << 8) | addr[5]);
+
+	return 0;
+}
+
 static int mv88e6060_setup(struct dsa_switch *ds)
 {
 	int ret;
@@ -203,6 +225,10 @@ static int mv88e6060_setup(struct dsa_switch *ds)
 	if (ret < 0)
 		return ret;
 
+	ret = mv88e6060_setup_addr(ds);
+	if (ret < 0)
+		return ret;
+
 	for (i = 0; i < MV88E6060_PORTS; i++) {
 		ret = mv88e6060_setup_port(ds, i);
 		if (ret < 0)
@@ -212,22 +238,6 @@ static int mv88e6060_setup(struct dsa_switch *ds)
 	return 0;
 }
 
-static int mv88e6060_set_addr(struct dsa_switch *ds, u8 *addr)
-{
-	u16 val = addr[0] << 8 | addr[1];
-
-	/* The multicast bit is always transmitted as a zero, so the switch uses
-	 * bit 8 for "DiffAddr", where 0 means all ports transmit the same SA.
-	 */
-	val &= 0xfeff;
-
-	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, val);
-	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_23, (addr[2] << 8) | addr[3]);
-	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_45, (addr[4] << 8) | addr[5]);
-
-	return 0;
-}
-
 static int mv88e6060_port_to_phy_addr(int port)
 {
 	if (port >= 0 && port < MV88E6060_PORTS)
@@ -262,7 +272,6 @@ static const struct dsa_switch_ops mv88e6060_switch_ops = {
 	.get_tag_protocol = mv88e6060_get_tag_protocol,
 	.probe		= mv88e6060_drv_probe,
 	.setup		= mv88e6060_setup,
-	.set_addr	= mv88e6060_set_addr,
 	.phy_read	= mv88e6060_phy_read,
 	.phy_write	= mv88e6060_phy_write,
 };
-- 
2.14.2

[PATCH net-next v3 4/5] net: dsa: dsa_loop: remove .set_addr

[Vivien Didelot]

The .set_addr function does nothing, remove the dsa_loop implementation
before getting rid of it completely in DSA.

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

diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index d55051abf4ed..3a3f4f7ba364 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -110,13 +110,6 @@ static void dsa_loop_get_ethtool_stats(struct dsa_switch *ds, int port,
 		data[i] = ps->ports[port].mib[i].val;
 }
 
-static int dsa_loop_set_addr(struct dsa_switch *ds, u8 *addr)
-{
-	dev_dbg(ds->dev, "%s\n", __func__);
-
-	return 0;
-}
-
 static int dsa_loop_phy_read(struct dsa_switch *ds, int port, int regnum)
 {
 	struct dsa_loop_priv *ps = ds->priv;
@@ -263,7 +256,6 @@ static const struct dsa_switch_ops dsa_loop_driver = {
 	.get_strings		= dsa_loop_get_strings,
 	.get_ethtool_stats	= dsa_loop_get_ethtool_stats,
 	.get_sset_count		= dsa_loop_get_sset_count,
-	.set_addr		= dsa_loop_set_addr,
 	.phy_read		= dsa_loop_phy_read,
 	.phy_write		= dsa_loop_phy_write,
 	.port_bridge_join	= dsa_loop_port_bridge_join,
-- 
2.14.2

[PATCH net-next v3 5/5] net: dsa: remove .set_addr

[Vivien Didelot]

Now that there is no user for the .set_addr function, remove it from
DSA. If a switch supports this feature (like mv88e6xxx), the
implementation can be done in the driver setup.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h | 1 -
 net/dsa/dsa2.c    | 6 ------
 net/dsa/legacy.c  | 6 ------
 3 files changed, 13 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index ce1d622734d7..2746741f74cf 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -291,7 +291,6 @@ struct dsa_switch_ops {
 	enum dsa_tag_protocol (*get_tag_protocol)(struct dsa_switch *ds);
 
 	int	(*setup)(struct dsa_switch *ds);
-	int	(*set_addr)(struct dsa_switch *ds, u8 *addr);
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
 
 	/*
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 54ed054777bd..6ac9e11d385c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -336,12 +336,6 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 	if (err)
 		return err;
 
-	if (ds->ops->set_addr) {
-		err = ds->ops->set_addr(ds, dst->cpu_dp->netdev->dev_addr);
-		if (err < 0)
-			return err;
-	}
-
 	if (!ds->slave_mii_bus && ds->ops->phy_read) {
 		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
 		if (!ds->slave_mii_bus)
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index 19ff6e0a21dc..b0fefbffe082 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -172,12 +172,6 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 	if (ret)
 		return ret;
 
-	if (ops->set_addr) {
-		ret = ops->set_addr(ds, master->dev_addr);
-		if (ret < 0)
-			return ret;
-	}
-
 	if (!ds->slave_mii_bus && ops->phy_read) {
 		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
 		if (!ds->slave_mii_bus)
-- 
2.14.2

Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Fri, 13 Oct 2017 14:18:04 -0400

> An Ethernet switch may support having a MAC address, which can be used
> as the switch's source address in transmitted full-duplex Pause frames.
> 
> If a DSA switch supports the related .set_addr operation, the DSA core
> sets the master's MAC address on the switch.
> 
> This won't make sense anymore in a multi-CPU ports system, because there
> won't be a unique master device assigned to a switch tree.
> 
> Moreover this operation is confusing because it makes the user think
> that it could be used to program the switch with the MAC address of the
> CPU/management port such that MAC address learning can be disabled on
> said port, but in fact, that's not how it is currently used.
> 
> To fix this, assign a random MAC address at setup time in the mv88e6060
> and mv88e6xxx drivers before removing .set_addr completely from DSA.
> 
> Changes in v3:
>   - include fix for mv88e6060 switch MAC address setter.
> 
> Changes in v2:
>   - remove .set_addr implementation from drivers and use a random MAC.

Series applied,thanks Vivien.

RE: [PATCH net-next v3 3/5] net: dsa: mv88e6060: setup random mac address

[David Laight]

From: Vivien Didelot
> Sent: 13 October 2017 19:18
> As for mv88e6xxx, setup the switch from within the mv88e6060 driver with
> a random MAC address, and remove the .set_addr implementation.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6060.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> index d64be2b83d3c..6173be889d95 100644
> --- a/drivers/net/dsa/mv88e6060.c
> +++ b/drivers/net/dsa/mv88e6060.c
> @@ -9,6 +9,7 @@
>   */
> 
>  #include <linux/delay.h>
> +#include <linux/etherdevice.h>
>  #include <linux/jiffies.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -188,6 +189,27 @@ static int mv88e6060_setup_port(struct dsa_switch *ds, int p)
>  	return 0;
>  }
> 
> +static int mv88e6060_setup_addr(struct dsa_switch *ds)
> +{
> +	u8 addr[ETH_ALEN];
> +	u16 val;
> +
> +	eth_random_addr(addr);
> +
> +	val = addr[0] << 8 | addr[1];
> +
> +	/* The multicast bit is always transmitted as a zero, so the switch uses
> +	 * bit 8 for "DiffAddr", where 0 means all ports transmit the same SA.
> +	 */
> +	val &= 0xfeff;

The comment is probably ok, but the mask isn't needed.
eth_randmon_addr() won't return and address with the multicast bit set.

	David

RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Rodney Cummings]

I am concerned about this proposed change.

According to IEEE Std 802.1Q, a "higher layer entity" (i.e. end station) that is internal to the bridge (i.e. switch) can use the same individual global MAC address that the switch is using. That behavior is common.

Use of a local semi-random MAC address is dangerous on Ethernet. Local MAC addresses can only be safely used in very narrow use cases, most of which are Wi-Fi. Generally speaking, local MAC addresses can be duplicated in the network, which causes many Ethernet protocols to break down. Therefore, general-purpose implementations like DSA cannot use a local MAC address in a reliable manner.

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Vivien Didelot
> Sent: Friday, October 13, 2017 1:18 PM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; kernel@savoirfairelinux.com; David S.
> Miller <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>;
> Andrew Lunn <andrew@lunn.ch>; David Laight <David.Laight@ACULAB.COM>;
> Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Subject: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
> 
> An Ethernet switch may support having a MAC address, which can be used
> as the switch's source address in transmitted full-duplex Pause frames.
> 
> If a DSA switch supports the related .set_addr operation, the DSA core
> sets the master's MAC address on the switch.
> 
> This won't make sense anymore in a multi-CPU ports system, because there
> won't be a unique master device assigned to a switch tree.
> 
> Moreover this operation is confusing because it makes the user think
> that it could be used to program the switch with the MAC address of the
> CPU/management port such that MAC address learning can be disabled on
> said port, but in fact, that's not how it is currently used.
> 
> To fix this, assign a random MAC address at setup time in the mv88e6060
> and mv88e6xxx drivers before removing .set_addr completely from DSA.
> 
> Changes in v3:
>   - include fix for mv88e6060 switch MAC address setter.
> 
> Changes in v2:
>   - remove .set_addr implementation from drivers and use a random MAC.
> 
> Vivien Didelot (5):
>   net: dsa: mv88e6xxx: setup random mac address
>   net: dsa: mv88e6060: fix switch MAC address
>   net: dsa: mv88e6060: setup random mac address
>   net: dsa: dsa_loop: remove .set_addr
>   net: dsa: remove .set_addr
> 
>  drivers/net/dsa/dsa_loop.c       |  8 --------
>  drivers/net/dsa/mv88e6060.c      | 37 ++++++++++++++++++++++++++---------
> --
>  drivers/net/dsa/mv88e6xxx/chip.c | 33 +++++++++++++++++----------------
>  include/net/dsa.h                |  1 -
>  net/dsa/dsa2.c                   |  6 ------
>  net/dsa/legacy.c                 |  6 ------
>  6 files changed, 43 insertions(+), 48 deletions(-)
> 
> --
> 2.14.2

Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Andrew Lunn]

On Mon, Oct 16, 2017 at 03:23:42PM +0000, Rodney Cummings wrote:
> I am concerned about this proposed change.
> 
> According to IEEE Std 802.1Q, a "higher layer entity" (i.e. end
> station) that is internal to the bridge (i.e. switch) can use the
> same individual global MAC address that the switch is using. That
> behavior is common.
>
> Use of a local semi-random MAC address is dangerous on
> Ethernet. Local MAC addresses can only be safely used in very narrow
> use cases, most of which are Wi-Fi. Generally speaking, local MAC
> addresses can be duplicated in the network, which causes many
> Ethernet protocols to break down. Therefore, general-purpose
> implementations like DSA cannot use a local MAC address in a
> reliable manner.

Hi Rodney

This is interesting. So i did some research. The Marvell Switch only
uses this MAC address for Pause frames. Nothing else.

802.3-2015 Section 2, Annex 31B says:

  The globally assigned 48-bit multicast address 01-80-C2-00-00-01 has
  been reserved for use in MAC Control PAUSE frames for inhibiting
  transmission of data frames from a DTE in a full duplex mode IEEE
  802.3 LAN. IEEE 802.1D-conformant bridges will not forward frames
  sent to this multicast destination address, regardless of the state
  of the bridge’s ports, or whether or not the bridge
  implements the MAC Control sublayer. To allow generic full duplex
  flow control, stations implementing the PAUSE operation shall
  instruct the MAC (e.g., through layer management) to enable
  reception of frames with destination address equal to this multicast
  address.

  NOTE—By definition, an IEEE 802.3 LAN operating in full
  duplex mode comprises exactly two stations, thus there is no
  ambiguity regarding the destination DTE’s identity. The use
  of a well-known multicast address relieves the MAC Control sublayer
  and its client from having to know, and maintain knowledge of, the
  individual 48-bit address of the other DTE in a full duplex
  environment.

  When MAC Control PFC operation (see Annex 31D and IEEE Std 802.1Q)
  has been enabled, MAC Control PAUSE operation shall be disabled.

So, received Pause frames never leave the MAC. They don't get bridged,
nor do they get passed up for host processing. They are purely point
to point between two MAC peers. The destination is unambiguous. It is
simple the other MAC peer. The destination address makes it clear it
is a pause frame, the the source address seems to be unneeded.

In this context, a random MAC addresses are safe.

In the more general case, i would agree with you. Collisions are
possible, causing problems.

   Andrew

RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Rodney Cummings]

Hi Andrew,

I may have misunderstood.

If this MAC address is the destination, a local random MAC address doesn't work. PAUSE is using one of the 802.1Q reserved destination MAC addresses (as does LLDP, MSTP, 1588, etc). PAUSE needs to use that specially assigned address (01-80-C2-00-00-01).

I assumed we were talking about the source MAC address. For the source MAC address, I would agree that a local random MAC address works in some narrow scenarios.

My concern is that for a source MAC address, a local random MAC address is not safe in all networks, because it has the potential for duplication. That topic has been discussed quite a bit in IEEE 802.

Rodney

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, October 16, 2017 11:10 AM
> To: Rodney Cummings <rodney.cummings@ni.com>
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@savoirfairelinux.com; David S. Miller <davem@davemloft.net>;
> Florian Fainelli <f.fainelli@gmail.com>; David Laight
> <David.Laight@ACULAB.COM>
> Subject: Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
> 
> On Mon, Oct 16, 2017 at 03:23:42PM +0000, Rodney Cummings wrote:
> > I am concerned about this proposed change.
> >
> > According to IEEE Std 802.1Q, a "higher layer entity" (i.e. end
> > station) that is internal to the bridge (i.e. switch) can use the
> > same individual global MAC address that the switch is using. That
> > behavior is common.
> >
> > Use of a local semi-random MAC address is dangerous on
> > Ethernet. Local MAC addresses can only be safely used in very narrow
> > use cases, most of which are Wi-Fi. Generally speaking, local MAC
> > addresses can be duplicated in the network, which causes many
> > Ethernet protocols to break down. Therefore, general-purpose
> > implementations like DSA cannot use a local MAC address in a
> > reliable manner.
> 
> Hi Rodney
> 
> This is interesting. So i did some research. The Marvell Switch only
> uses this MAC address for Pause frames. Nothing else.
> 
> 802.3-2015 Section 2, Annex 31B says:
> 
>   The globally assigned 48-bit multicast address 01-80-C2-00-00-01 has
>   been reserved for use in MAC Control PAUSE frames for inhibiting
>   transmission of data frames from a DTE in a full duplex mode IEEE
>   802.3 LAN. IEEE 802.1D-conformant bridges will not forward frames
>   sent to this multicast destination address, regardless of the state
>   of the bridge’s ports, or whether or not the bridge
>   implements the MAC Control sublayer. To allow generic full duplex
>   flow control, stations implementing the PAUSE operation shall
>   instruct the MAC (e.g., through layer management) to enable
>   reception of frames with destination address equal to this multicast
>   address.
> 
>   NOTE—By definition, an IEEE 802.3 LAN operating in full
>   duplex mode comprises exactly two stations, thus there is no
>   ambiguity regarding the destination DTE’s identity. The use
>   of a well-known multicast address relieves the MAC Control sublayer
>   and its client from having to know, and maintain knowledge of, the
>   individual 48-bit address of the other DTE in a full duplex
>   environment.
> 
>   When MAC Control PFC operation (see Annex 31D and IEEE Std 802.1Q)
>   has been enabled, MAC Control PAUSE operation shall be disabled.
> 
> So, received Pause frames never leave the MAC. They don't get bridged,
> nor do they get passed up for host processing. They are purely point
> to point between two MAC peers. The destination is unambiguous. It is
> simple the other MAC peer. The destination address makes it clear it
> is a pause frame, the the source address seems to be unneeded.
> 
> In this context, a random MAC addresses are safe.
> 
> In the more general case, i would agree with you. Collisions are
> possible, causing problems.
> 
>    Andrew

RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[David Laight]

From: Andrew Lunn
> Sent: 16 October 2017 17:10
...
> So, received Pause frames never leave the MAC. They don't get bridged,
> nor do they get passed up for host processing. They are purely point
> to point between two MAC peers. The destination is unambiguous. It is
> simple the other MAC peer. The destination address makes it clear it
> is a pause frame, the the source address seems to be unneeded.
> 
> In this context, a random MAC addresses are safe.

Is there any reason why a fixed value (say 00:00:00:00:00:00)
can't be used?

> In the more general case, i would agree with you. Collisions are
> possible, causing problems.

For IP MAC addresses only go as far as the first router.
So the duplicates would (typically) have to be within the same subnet.
This makes the chance of a duplicate random address unlikely.

(Unless you have an un-subnetted class A network consisting of
multiple 1km long coax segments connected by 1km long repeaters
making a single collision domain.)

	David

Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Andrew Lunn]

On Mon, Oct 16, 2017 at 04:30:34PM +0000, David Laight wrote:
> From: Andrew Lunn
> > Sent: 16 October 2017 17:10
> ...
> > So, received Pause frames never leave the MAC. They don't get bridged,
> > nor do they get passed up for host processing. They are purely point
> > to point between two MAC peers. The destination is unambiguous. It is
> > simple the other MAC peer. The destination address makes it clear it
> > is a pause frame, the the source address seems to be unneeded.
> > 
> > In this context, a random MAC addresses are safe.
> 
> Is there any reason why a fixed value (say 00:00:00:00:00:00)
> can't be used?

I was going to suggest 42:42:42:42:42:42 :-)

  Andrew

Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Andrew Lunn]

On Mon, Oct 16, 2017 at 04:28:04PM +0000, Rodney Cummings wrote:
> Hi Andrew,
> 
> I may have misunderstood.
> 
> If this MAC address is the destination

Nope. This is the source address, for Pause frames.

> My concern is that for a source MAC address, a local random MAC
> address is not safe in all networks, because it has the potential
> for duplication. That topic has been discussed quite a bit in IEEE
> 802.

Duplications don't matter, for pause frames. The source address
appears to be unused. And these frames don't get passed the direct
peers MAC layer.

	Andrew

RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Rodney Cummings]

PAUSE is only one higher layer entity. The source MAC address may not matter there, but it definitely matters for other higher layer entities (like IEEE Std 1588).

The Marvell switch can run more than just PAUSE as a higher layer protocol.

How do you guarantee that this MAC address is used for PAUSE, and only for PAUSE?

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, October 16, 2017 11:48 AM
> To: Rodney Cummings <rodney.cummings@ni.com>
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@savoirfairelinux.com; David S. Miller <davem@davemloft.net>;
> Florian Fainelli <f.fainelli@gmail.com>; David Laight
> <David.Laight@ACULAB.COM>
> Subject: Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
> 
> On Mon, Oct 16, 2017 at 04:28:04PM +0000, Rodney Cummings wrote:
> > Hi Andrew,
> >
> > I may have misunderstood.
> >
> > If this MAC address is the destination
> 
> Nope. This is the source address, for Pause frames.
> 
> > My concern is that for a source MAC address, a local random MAC
> > address is not safe in all networks, because it has the potential
> > for duplication. That topic has been discussed quite a bit in IEEE
> > 802.
> 
> Duplications don't matter, for pause frames. The source address
> appears to be unused. And these frames don't get passed the direct
> peers MAC layer.
> 
> 	Andrew

Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Andrew Lunn]

> How do you guarantee that this MAC address is used for PAUSE, and only for PAUSE?

That is the only thing the data sheet says this MAC address is used
for.

	Andrew

RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr

[Rodney Cummings]

I stand corrected. Thanks for clearing that up Andrew.

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, October 16, 2017 11:57 AM
> To: Rodney Cummings <rodney.cummings@ni.com>
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@savoirfairelinux.com; David S. Miller <davem@davemloft.net>;
> Florian Fainelli <f.fainelli@gmail.com>; David Laight
> <David.Laight@ACULAB.COM>
> Subject: Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
> 
> > How do you guarantee that this MAC address is used for PAUSE, and only
> for PAUSE?
> 
> That is the only thing the data sheet says this MAC address is used
> for.
> 
> 	Andrew