[RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[Jakub Kicinski]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC net-next 6/6] ethernet: sparx5: use eth_hw_addr_set_port()

[Jakub Kicinski]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[Shannon Nelson]

On 10/15/21 12:38 PM, Jakub Kicinski wrote:
> 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)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[Jakub Kicinski]

On Fri, 15 Oct 2021 14:36:00 -0700 Shannon Nelson wrote:
> On 10/15/21 12:38 PM, Jakub Kicinski wrote:
> > 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!

> > +/**
> > + * 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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[Vladimir Oltean]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[Ido Schimmel]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC net-next 1/6] ethernet: add a helper for assigning port addresses

[Jakub Kicinski]

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:
> > +/**
> > + * 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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel