From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-15 19:40:36
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(+)
From: Shannon Nelson <hidden> Date: 2021-10-15 21:37:34
On 10/15/21 12:38 PM, Jakub Kicinski wrote:
quoted hunk
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(+)
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-15 22:32:16
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-18 14:17:58
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.