[RFC net-next 0/6] ethernet: add eth_hw_addr_set_port()

While doing the last polishing of the drivers/ethernet
changes I realized we have a handful of drivers offsetting
some base MAC addr by an id. So I decided to add a helper
for it. The helper takes care of wrapping which is probably
not 100% necessary but seems like a good idea. And it saves
driver side LoC (the diffstat is actually negative if we
compare against the changes I'd have to make if I was to
convert all these drivers to not operate directly on
netdev->dev_addr).

Sending as RFC, because feedback welcome.. and it may
be weekend for Ido.

Jakub Kicinski (6):
  ethernet: add a helper for assigning port addresses
  ethernet: ocelot: use eth_hw_addr_set_port()
  ethernet: prestera: use eth_hw_addr_set_port()
  ethernet: fec: use eth_hw_addr_set_port()
  ethernet: mlxsw: use eth_hw_addr_set_port()
  ethernet: sparx5: use eth_hw_addr_set_port()

 drivers/net/ethernet/freescale/fec_main.c     |  5 +----
 .../ethernet/marvell/prestera/prestera_main.c |  5 +++--
 drivers/net/ethernet/mellanox/mlxsw/minimal.c |  9 +++-----
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  8 +++----
 .../ethernet/microchip/sparx5/sparx5_netdev.c |  4 +---
 drivers/net/ethernet/mscc/ocelot_net.c        |  3 +--
 include/linux/etherdevice.h                   | 21 +++++++++++++++++++
 7 files changed, 34 insertions(+), 21 deletions(-)

-- 
2.31.1

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: vladimir.oltean@nxp.com
CC: claudiu.manoil@nxp.com
CC: alexandre.belloni@bootlin.com
CC: UNGLinuxDriver@microchip.com
---
 drivers/net/ethernet/mscc/ocelot_net.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 9992bf06311d..1e4459dfe016 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c

@@ -1705,8 +1705,7 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target,
 		NETIF_F_HW_TC;
 	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_TC;
 
-	eth_hw_addr_set(dev, ocelot->base_mac);
-	dev->dev_addr[ETH_ALEN - 1] += port;
+	eth_hw_addr_set_port(dev, ocelot->base_mac, port);
 	ocelot_mact_learn(ocelot, PGID_CPU, dev->dev_addr,
 			  ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
 

-- 
2.31.1

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: qiangqing.zhang@nxp.com
---
 drivers/net/ethernet/freescale/fec_main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 47a6fc702ac7..bc5a1a4a34a9 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -1768,11 +1768,8 @@ static int fec_get_mac(struct net_device *ndev)
 		return 0;
 	}
 
-	eth_hw_addr_set(ndev, iap);
-
 	/* Adjust MAC if using macaddr */
-	if (iap == macaddr)
-		 ndev->dev_addr[ETH_ALEN-1] = macaddr[ETH_ALEN-1] + fep->dev_id;
+	eth_hw_addr_set_port(ndev, iap, iap == macaddr ? fep->dev_id : 0);
 
 	return 0;
 }

-- 
2.31.1

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

We need to make sure the last byte is zeroed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: vkochan@marvell.com
CC: tchornyi@marvell.com
---
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index b667f560b931..7d179927dabe 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c

@@ -290,6 +290,7 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
 {
 	struct prestera_port *port;
 	struct net_device *dev;
+	u8 addr[ETH_ALEN] = {};
 	int err;
 
 	dev = alloc_etherdev(sizeof(*port));

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
 	/* firmware requires that port's MAC address consist of the first
 	 * 5 bytes of the base MAC address
 	 */
-	memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-	dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+	memcpy(addr, sw->base_mac, dev->addr_len - 1);
+	eth_hw_addr_set_port(dev, addr, port->fp_id);
 
 	err = prestera_hw_port_mac_set(port, dev->dev_addr);
 	if (err) {

-- 
2.31.1

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@nvidia.com
CC: idosch@nvidia.com
---
 drivers/net/ethernet/mellanox/mlxsw/minimal.c  | 9 +++------
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 8 ++++----
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/minimal.c b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
index e0892f259adf..8137606df615 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/minimal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/minimal.c

@@ -202,18 +202,15 @@ mlxsw_m_port_dev_addr_get(struct mlxsw_m_port *mlxsw_m_port)
 	struct mlxsw_m *mlxsw_m = mlxsw_m_port->mlxsw_m;
 	struct net_device *dev = mlxsw_m_port->dev;
 	char ppad_pl[MLXSW_REG_PPAD_LEN];
+	u8 addr[ETH_ALEN];
 	int err;
 
 	mlxsw_reg_ppad_pack(ppad_pl, false, 0);
 	err = mlxsw_reg_query(mlxsw_m->core, MLXSW_REG(ppad), ppad_pl);
 	if (err)
 		return err;
-	mlxsw_reg_ppad_mac_memcpy_from(ppad_pl, dev->dev_addr);
-	/* The last byte value in base mac address is guaranteed
-	 * to be such it does not overflow when adding local_port
-	 * value.
-	 */
-	dev->dev_addr[ETH_ALEN - 1] += mlxsw_m_port->module + 1;
+	mlxsw_reg_ppad_mac_memcpy_from(ppad_pl, addr);
+	eth_hw_addr_set_port(dev, addr, mlxsw_m_port->module + 1);
 	return 0;
 }
 

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index d05850ff3a77..38521d9402b2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c

@@ -316,11 +316,11 @@ static int mlxsw_sp_port_dev_addr_set(struct mlxsw_sp_port *mlxsw_sp_port,
 static int mlxsw_sp_port_dev_addr_init(struct mlxsw_sp_port *mlxsw_sp_port)
 {
 	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
-	unsigned char *addr = mlxsw_sp_port->dev->dev_addr;
 
-	ether_addr_copy(addr, mlxsw_sp->base_mac);
-	addr[ETH_ALEN - 1] += mlxsw_sp_port->local_port;
-	return mlxsw_sp_port_dev_addr_set(mlxsw_sp_port, addr);
+	eth_hw_addr_set_port(mlxsw_sp_port->dev, mlxsw_sp->base_mac,
+			     mlxsw_sp_port->local_port);
+	return mlxsw_sp_port_dev_addr_set(mlxsw_sp_port,
+					  mlxsw_sp_port->dev->dev_addr);
 }
 
 static int mlxsw_sp_port_max_mtu_get(struct mlxsw_sp_port *mlxsw_sp_port, int *p_max_mtu)

-- 
2.31.1

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: lars.povlsen@microchip.com
CC: Steen.Hegelund@microchip.com
CC: UNGLinuxDriver@microchip.com
CC: bjarni.jonasson@microchip.com
CC: linux-arm-kernel@lists.infradead.org
---
 drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
index b21ebaa32d7e..d56822f6d09e 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c

@@ -200,7 +200,6 @@ struct net_device *sparx5_create_netdev(struct sparx5 *sparx5, u32 portno)
 {
 	struct sparx5_port *spx5_port;
 	struct net_device *ndev;
-	u64 val;
 
 	ndev = devm_alloc_etherdev(sparx5->dev, sizeof(struct sparx5_port));
 	if (!ndev)

@@ -216,8 +215,7 @@ struct net_device *sparx5_create_netdev(struct sparx5 *sparx5, u32 portno)
 	ndev->netdev_ops = &sparx5_port_netdev_ops;
 	ndev->ethtool_ops = &sparx5_ethtool_ops;
 
-	val = ether_addr_to_u64(sparx5->base_mac) + portno + 1;
-	u64_to_ether_addr(val, ndev->dev_addr);
+	eth_hw_addr_set_port(ndev, sparx5->base_mac, portno + 1);
 
 	return ndev;
 }

-- 
2.31.1

We have 5 drivers which offset base MAC addr by port id.
Create a helper for them.

This helper takes care of overflows, which some drivers
did not do, please complain if that's going to break
anything!

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@nvidia.com
CC: idosch@nvidia.com
CC: lars.povlsen@microchip.com
CC: Steen.Hegelund@microchip.com
CC: UNGLinuxDriver@microchip.com
CC: bjarni.jonasson@microchip.com
CC: linux-arm-kernel@lists.infradead.org
CC: qiangqing.zhang@nxp.com
CC: vkochan@marvell.com
CC: tchornyi@marvell.com
CC: vladimir.oltean@nxp.com
CC: claudiu.manoil@nxp.com
CC: alexandre.belloni@bootlin.com
CC: UNGLinuxDriver@microchip.com
---
 include/linux/etherdevice.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 23681c3d3b8a..157f6c7ac9ff 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h

@@ -551,6 +551,27 @@ static inline unsigned long compare_ether_header(const void *a, const void *b)
 #endif
 }
 
+/**
+ * eth_hw_addr_set_port - Generate and assign Ethernet address to a port
+ * @dev: pointer to port's net_device structure
+ * @base_addr: base Ethernet address
+ * @id: offset to add to the base address
+ *
+ * Assign a MAC address to the net_device using a base address and an offset.
+ * Commonly used by switch drivers which need to compute addresses for all
+ * their ports. addr_assign_type is not changed.
+ */
+static inline void eth_hw_addr_set_port(struct net_device *dev,
+					const u8 *base_addr, u8 id)
+{
+	u64 u = ether_addr_to_u64(base_addr);
+	u8 addr[ETH_ALEN];
+
+	u += id;
+	u64_to_ether_addr(u, addr);
+	eth_hw_addr_set(dev, addr);
+}
+
 /**
  * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
  * @skb: Buffer to pad

-- 
2.31.1

On 10/15/21 12:38 PM, Jakub Kicinski wrote:

To me, the words "_set_port" imply that you're going to force "id" into 
the byte, overwriting what is already there.  Since this instead is 
adding "id" to the byte, perhaps a better name would include the word 
"offset", maybe like eth_hw_addr_set_port_offset(), to better imply the 
actual operation.

Personally, I think my name suggestion is too long, but it gets my 
thought across.

sln

+{
+	u64 u = ether_addr_to_u64(base_addr);
+	u8 addr[ETH_ALEN];
+
+	u += id;
+	u64_to_ether_addr(u, addr);
+	eth_hw_addr_set(dev, addr);
+}
+
  /**
   * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
   * @skb: Buffer to pad

On Fri, 15 Oct 2021 14:36:00 -0700 Shannon Nelson wrote:

On 10/15/21 12:38 PM, Jakub Kicinski wrote:

quoted

We have 5 drivers which offset base MAC addr by port id.
Create a helper for them.

This helper takes care of overflows, which some drivers
did not do, please complain if that's going to break
anything!

quoted

+/**
+ * eth_hw_addr_set_port - Generate and assign Ethernet address to a port
+ * @dev: pointer to port's net_device structure
+ * @base_addr: base Ethernet address
+ * @id: offset to add to the base address
+ *
+ * Assign a MAC address to the net_device using a base address and an offset.
+ * Commonly used by switch drivers which need to compute addresses for all
+ * their ports. addr_assign_type is not changed.
+ */
+static inline void eth_hw_addr_set_port(struct net_device *dev,
+					const u8 *base_addr, u8 id)  

To me, the words "_set_port" imply that you're going to force "id" into 
the byte, overwriting what is already there.  Since this instead is 
adding "id" to the byte, perhaps a better name would include the word 
"offset", maybe like eth_hw_addr_set_port_offset(), to better imply the 
actual operation.

Personally, I think my name suggestion is too long, but it gets my 
thought across.

I started with eth_hw_addr_set_offset() my thought process was:

  .._set_offset() sounds like it's setting the offset

  dev_addr_mod() uses offset to modify just part of the address
  so we have two similar functions using 'offset' with different 
  meaning

  how about we name it after the most common use? -> .._port()

Thinking again maybe eth_hw_addr_gen()? We "generate" a port address
based on base address and port ID.

I can change if others agree that .._set_offset() is better.

On Fri, Oct 15, 2021 at 03:30:53PM -0700, Jakub Kicinski wrote:

Thinking again maybe eth_hw_addr_gen()? We "generate" a port address
based on base address and port ID.

I like eth_hw_addr_gen() better.

On Fri, Oct 15, 2021 at 12:38:45PM -0700, Jakub Kicinski wrote:

Instead of having yet another temporary copy, can't we zero out
sw->base_mac[ETH_ALEN - 1] in prestera_switch_set_base_mac_addr()?

 
 	err = prestera_hw_port_mac_set(port, dev->dev_addr);
 	if (err) {
-- 
2.31.1

On Sat, 16 Oct 2021 02:51:30 +0300 Vladimir Oltean wrote:

quoted

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
 	/* firmware requires that port's MAC address consist of the first
 	 * 5 bytes of the base MAC address
 	 */
-	memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-	dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+	memcpy(addr, sw->base_mac, dev->addr_len - 1);
+	eth_hw_addr_set_port(dev, addr, port->fp_id);  

Instead of having yet another temporary copy, can't we zero out
sw->base_mac[ETH_ALEN - 1] in prestera_switch_set_base_mac_addr()?

Will do unless Marvel & friends tell us FW cares about the last byte
(prestera_hw_switch_mac_set() send the whole thing).

On Fri, Oct 15, 2021 at 05:17:30PM -0700, Jakub Kicinski wrote:

On Sat, 16 Oct 2021 02:51:30 +0300 Vladimir Oltean wrote:

quoted

quoted

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
 	/* firmware requires that port's MAC address consist of the first
 	 * 5 bytes of the base MAC address
 	 */
-	memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-	dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+	memcpy(addr, sw->base_mac, dev->addr_len - 1);
+	eth_hw_addr_set_port(dev, addr, port->fp_id);  

Instead of having yet another temporary copy, can't we zero out
sw->base_mac[ETH_ALEN - 1] in prestera_switch_set_base_mac_addr()?

Will do unless Marvel & friends tell us FW cares about the last byte
(prestera_hw_switch_mac_set() send the whole thing).

You can always zero out the last byte after the call to
prestera_hw_switch_mac_set(), and then it shouldn't even matter.

On 10/15/21 12:38 PM, Jakub Kicinski wrote:

Notice in this case I think the original code is setting the last byte 
to port->fp_id, found I think by a call to their firmware, not by adding 
fp_id to the existing byte value.

This is an example of how I feel a bit queezy about this suggested 
helper: each driver that does something like this may need to do it 
slightly differently depending upon how their hardware/firmware works.  
We may be trying to help too much here.

As a potential consumer of these helpers, I'd rather do my own mac 
address byte twiddling and then use eth_hw_addr_set() to put it into place.

sln

  
  	err = prestera_hw_port_mac_set(port, dev->dev_addr);
  	if (err) {

On Fri, Oct 15, 2021 at 12:38:43PM -0700, Jakub Kicinski wrote:

+/**
+ * eth_hw_addr_set_port - Generate and assign Ethernet address to a port
+ * @dev: pointer to port's net_device structure
+ * @base_addr: base Ethernet address
+ * @id: offset to add to the base address
+ *
+ * Assign a MAC address to the net_device using a base address and an offset.
+ * Commonly used by switch drivers which need to compute addresses for all
+ * their ports. addr_assign_type is not changed.
+ */
+static inline void eth_hw_addr_set_port(struct net_device *dev,
+					const u8 *base_addr, u8 id)

If necessary, would it be possible to change 'id' to u16?

I'm asking because currently in mlxsw we set the MAC of each netdev to
'base_mac + local_port' where 'local_port' is u8. In Spectrum-4 we are
going to have more than 256 logical ports, so 'local_port' becomes u16.

Regarding the naming, eth_hw_addr_gen() sounds good to me.

Thanks for working on this

+{
+	u64 u = ether_addr_to_u64(base_addr);
+	u8 addr[ETH_ALEN];
+
+	u += id;
+	u64_to_ether_addr(u, addr);
+	eth_hw_addr_set(dev, addr);
+}
+
 /**
  * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
  * @skb: Buffer to pad
-- 
2.31.1

On Sun, 17 Oct 2021 18:06:17 +0300 Ido Schimmel wrote:

On Fri, Oct 15, 2021 at 12:38:43PM -0700, Jakub Kicinski wrote:

quoted

+/**
+ * eth_hw_addr_set_port - Generate and assign Ethernet address to a port
+ * @dev: pointer to port's net_device structure
+ * @base_addr: base Ethernet address
+ * @id: offset to add to the base address
+ *
+ * Assign a MAC address to the net_device using a base address and an offset.
+ * Commonly used by switch drivers which need to compute addresses for all
+ * their ports. addr_assign_type is not changed.
+ */
+static inline void eth_hw_addr_set_port(struct net_device *dev,
+					const u8 *base_addr, u8 id)  

If necessary, would it be possible to change 'id' to u16?

Let me make it an unsigned int, I had u8 initially because I wasn't
planning on doing the wrapping and wanted the compiler to warn.

I'm asking because currently in mlxsw we set the MAC of each netdev to
'base_mac + local_port' where 'local_port' is u8. In Spectrum-4 we are
going to have more than 256 logical ports, so 'local_port' becomes u16.

Regarding the naming, eth_hw_addr_gen() sounds good to me.

On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:

On 10/15/21 12:38 PM, Jakub Kicinski wrote:

quoted

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

We need to make sure the last byte is zeroed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

quoted

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
  	/* firmware requires that port's MAC address consist of the first
  	 * 5 bytes of the base MAC address
  	 */
-	memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-	dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+	memcpy(addr, sw->base_mac, dev->addr_len - 1);
+	eth_hw_addr_set_port(dev, addr, port->fp_id);  

Notice in this case I think the original code is setting the last byte 
to port->fp_id, found I think by a call to their firmware, not by adding 
fp_id to the existing byte value.

Yeah, as mentioned in the commit message and discussed with Vladimir.
Notice that the memcpy is (,, size - 1) and the initial buf is zeroed.

This is an example of how I feel a bit queezy about this suggested 
helper: each driver that does something like this may need to do it 
slightly differently depending upon how their hardware/firmware works.  
We may be trying to help too much here.

As a potential consumer of these helpers, I'd rather do my own mac 
address byte twiddling and then use eth_hw_addr_set() to put it into place.

This is disproved by many upstream drivers, I only converted the ones
that jumped out at me on Friday, but I'm sure there is more. If your
driver is _really_ doing something questionable^W I mean "special"
nothing is stopping you from open coding it. For others the helper will
be useful. 

IOW I don't understand your comment.

In fact if someone is "afraid" of others refactoring their driver or
can't provide timely feedback (ekhm, prestera people) maybe the driver
doesn't belong upstream.

On 10/18/21 7:19 AM, Jakub Kicinski wrote:

On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:

quoted

On 10/15/21 12:38 PM, Jakub Kicinski wrote:

quoted

Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.

We need to make sure the last byte is zeroed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
   	/* firmware requires that port's MAC address consist of the first
   	 * 5 bytes of the base MAC address
   	 */
-	memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-	dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+	memcpy(addr, sw->base_mac, dev->addr_len - 1);
+	eth_hw_addr_set_port(dev, addr, port->fp_id);

Notice in this case I think the original code is setting the last byte
to port->fp_id, found I think by a call to their firmware, not by adding
fp_id to the existing byte value.

Yeah, as mentioned in the commit message and discussed with Vladimir.
Notice that the memcpy is (,, size - 1) and the initial buf is zeroed.

quoted

This is an example of how I feel a bit queezy about this suggested
helper: each driver that does something like this may need to do it
slightly differently depending upon how their hardware/firmware works.
We may be trying to help too much here.

As a potential consumer of these helpers, I'd rather do my own mac
address byte twiddling and then use eth_hw_addr_set() to put it into place.

This is disproved by many upstream drivers, I only converted the ones
that jumped out at me on Friday, but I'm sure there is more. If your
driver is _really_ doing something questionable^W I mean "special"
nothing is stopping you from open coding it. For others the helper will
be useful.

IOW I don't understand your comment.

To try to answer your RFC more clearly: I feel that this particular 
helper obfuscates the operation more than it helps.

sln

----------------------------------------------------------------------
On Fri, Oct 15, 2021 at 12:38:45PM -0700, Jakub Kicinski wrote:

This code is a bit buggy.  We do care about the last byte of the base mac address.
For example if base mac is xx:xx:xx:xx:xx:10 first port mac should be  xx:xx:xx:xx:xx:11

+     eth_hw_addr_set_port(dev, addr, port->fp_id);

Instead of having yet another temporary copy, can't we zero out
sw->base_mac[ETH_ALEN - 1] in prestera_switch_set_base_mac_addr()?

      err = prestera_hw_port_mac_set(port, dev->dev_addr);
      if (err) {
--
2.31.1

On Mon, 18 Oct 2021 16:54:00 +0000 Taras Chornyi [C] wrote:

quoted

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
      /* firmware requires that port's MAC address consist of the first
       * 5 bytes of the base MAC address
       */
-     memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-     dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+     memcpy(addr, sw->base_mac, dev->addr_len - 1);  

This code is a bit buggy.  We do care about the last byte of the base mac address.
For example if base mac is xx:xx:xx:xx:xx:10 first port mac should be  xx:xx:xx:xx:xx:11

Thanks for the reply, does it mean we can assume base_mac will be valid
or should we add a check like below?

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index b667f560b931..966f94c6c7a6 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c

@@ -338,11 +338,14 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
                goto err_port_init;
        }
 
-       /* firmware requires that port's MAC address consist of the first
-        * 5 bytes of the base MAC address
-        */
-       memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-       dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+       eth_hw_addr_set_port(dev, sw->base_mac, port->fp_id);
+       if (memcmp(dev->dev_addr, sw->base_mac, ETH_ALEN - 1)) {
+               /* firmware requires that port's MAC address consists
+                * of the first 5 bytes of the base MAC address
+                */
+               dev_warn(prestera_dev(sw), "Port MAC address overflows the base for port(%u)\n", id);
+               dev_addr_mod(dev, 0, sw->base_mac, ETH_ALEN - 1);
+       }
 
        err = prestera_hw_port_mac_set(port, dev->dev_addr);
        if (err) {

On Mon, Oct 18, 2021 at 09:26:21AM -0700, Shannon Nelson wrote:

On 10/18/21 7:19 AM, Jakub Kicinski wrote:

quoted

On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:

quoted

As a potential consumer of these helpers, I'd rather do my own mac
address byte twiddling and then use eth_hw_addr_set() to put it into place.

This is disproved by many upstream drivers, I only converted the ones
that jumped out at me on Friday, but I'm sure there is more. If your
driver is _really_ doing something questionable^W I mean "special"
nothing is stopping you from open coding it. For others the helper will
be useful.

IOW I don't understand your comment.

To try to answer your RFC more clearly: I feel that this particular helper
obfuscates the operation more than it helps.

FWIW, it at least helped me realize that we are going to have a bug with
Spectrum-4. Currently we have:

ether_addr_copy(addr, mlxsw_sp->base_mac);
addr[ETH_ALEN - 1] += mlxsw_sp_port->local_port;

As a preparation for Spectrum-4 we are promoting 'local_port' to u16
since at least 257 and 258 are valid local port values.

With the current code, the netdev corresponding to local port 1 will
have the same MAC as the netdev corresponding to local port 257.

After Jakub's conversion and changing the 'id' argument to 'unsigned
int' [1], it should work correctly.

[1] https://lore.kernel.org/netdev/20211018070845.68ba815d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

On 10/18/21 10:33 AM, Ido Schimmel wrote:

On Mon, Oct 18, 2021 at 09:26:21AM -0700, Shannon Nelson wrote:

quoted

On 10/18/21 7:19 AM, Jakub Kicinski wrote:

quoted

On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:

quoted

As a potential consumer of these helpers, I'd rather do my own mac
address byte twiddling and then use eth_hw_addr_set() to put it into place.

This is disproved by many upstream drivers, I only converted the ones
that jumped out at me on Friday, but I'm sure there is more. If your
driver is _really_ doing something questionable^W I mean "special"
nothing is stopping you from open coding it. For others the helper will
be useful.

IOW I don't understand your comment.

To try to answer your RFC more clearly: I feel that this particular helper
obfuscates the operation more than it helps.

FWIW, it at least helped me realize that we are going to have a bug with
Spectrum-4. Currently we have:

ether_addr_copy(addr, mlxsw_sp->base_mac);
addr[ETH_ALEN - 1] += mlxsw_sp_port->local_port;

As a preparation for Spectrum-4 we are promoting 'local_port' to u16
since at least 257 and 258 are valid local port values.

With the current code, the netdev corresponding to local port 1 will
have the same MAC as the netdev corresponding to local port 257.

After Jakub's conversion and changing the 'id' argument to 'unsigned
int' [1], it should work correctly.

[1] https://lore.kernel.org/netdev/20211018070845.68ba815d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

I would think that it might be clearer to do something like

     u64 addr64;

     addr64 = ether_addr_to_64(mlxsw_sp->base_mac);
     addr64 += mlxsw_sp_port->local_port;
     u64_to_ether_addr(addr64, addr);
     eth_hw_addr_set(dev, addr);

This uses our helpers to keep common actions safe, but also keeps the 
vendor specific logic (add N to the base_mac) in the driver.

sln

----------------------------------------------------------------------
On Mon, 18 Oct 2021 16:54:00 +0000 Taras Chornyi [C] wrote:

quoted

quoted

@@ -341,8 +342,8 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
      /* firmware requires that port's MAC address consist of the first
       * 5 bytes of the base MAC address
       */
-     memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
-     dev->dev_addr[dev->addr_len - 1] = port->fp_id;
+     memcpy(addr, sw->base_mac, dev->addr_len - 1);  

This code is a bit buggy.  We do care about the last byte of the base mac address.
For example if base mac is xx:xx:xx:xx:xx:10 first port mac should be  xx:xx:xx:xx:xx:11

Thanks for the reply, does it mean we can assume base_mac will be valid
or should we add a check like below?

We can assume that base mac is always valid in production environment(stored in eeprom),
however if can we can not get base mac it will be generated.

On Mon, Oct 18, 2021 at 10:54:56AM -0700, Shannon Nelson wrote:

On 10/18/21 10:33 AM, Ido Schimmel wrote:

quoted

On Mon, Oct 18, 2021 at 09:26:21AM -0700, Shannon Nelson wrote:

quoted

On 10/18/21 7:19 AM, Jakub Kicinski wrote:

quoted

On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:

quoted

As a potential consumer of these helpers, I'd rather do my own mac
address byte twiddling and then use eth_hw_addr_set() to put it into place.

This is disproved by many upstream drivers, I only converted the ones
that jumped out at me on Friday, but I'm sure there is more. If your
driver is _really_ doing something questionable^W I mean "special"
nothing is stopping you from open coding it. For others the helper will
be useful.

IOW I don't understand your comment.

To try to answer your RFC more clearly: I feel that this particular helper
obfuscates the operation more than it helps.

FWIW, it at least helped me realize that we are going to have a bug with
Spectrum-4. Currently we have:

ether_addr_copy(addr, mlxsw_sp->base_mac);
addr[ETH_ALEN - 1] += mlxsw_sp_port->local_port;

As a preparation for Spectrum-4 we are promoting 'local_port' to u16
since at least 257 and 258 are valid local port values.

With the current code, the netdev corresponding to local port 1 will
have the same MAC as the netdev corresponding to local port 257.

After Jakub's conversion and changing the 'id' argument to 'unsigned
int' [1], it should work correctly.

[1] https://lore.kernel.org/netdev/20211018070845.68ba815d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

I would think that it might be clearer to do something like

    u64 addr64;

    addr64 = ether_addr_to_64(mlxsw_sp->base_mac);
    addr64 += mlxsw_sp_port->local_port;
    u64_to_ether_addr(addr64, addr);
    eth_hw_addr_set(dev, addr);

This is basically what Jakub's helper is doing...

I don't know how to argue with "clearer", but the fact is that we are
not doing what you suggested right now (hindsight is always 20/20) and
that it would have taken me time to debug it.

The suggested helper already helped to avoid one bug and it's not even
merged yet, so it's safe to assume it will help to avoid more bugs in
the future.