of: net: support NVMEM cells with MAC in text format

Commit Message

Rafał Miłecki Dec. 23, 2021, 12:27 p.m. UTC

From: Rafał Miłecki <[e-mail]>

Some NVMEM devices have text based cells. In such cases MAC is stored in
a XX:XX:XX:XX:XX:XX format. Use mac_pton() to parse such data and
support those NVMEM cells. This is required to support e.g. a very
popular U-Boot and its environment variables.

Signed-off-by: Rafał Miłecki <[e-mail]>
---
Please let me know if checking NVMEM cell length (6 B vs. 17 B) can be
considered a good enough solution. Alternatively we could use some DT
property to make it explicity, e.g. something like:

ethernet@18024000 {
	compatible = "brcm,amac";
	reg = <0x18024000 0x800>;

	nvmem-cells = <&mac_addr>;
	nvmem-cell-names = "mac-address";
	nvmem-mac-format = "text";
};
---
 net/core/of_net.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

Comments

[e-mail] Dec. 29, 2021, 11:40 a.m. UTC | #1

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <[e-mail]>:

On Thu, 23 Dec 2021 13:27:47 +0100 you wrote:
> From: Rafał Miłecki <[e-mail]>
> 
> Some NVMEM devices have text based cells. In such cases MAC is stored in
> a XX:XX:XX:XX:XX:XX format. Use mac_pton() to parse such data and
> support those NVMEM cells. This is required to support e.g. a very
> popular U-Boot and its environment variables.
> 
> [...]

Here is the summary with links:
  - of: net: support NVMEM cells with MAC in text format
    https://git.kernel.org/netdev/net-next/c/9ed319e41191

You are awesome, thank you!

Michael Walle Dec. 29, 2021, 12:40 p.m. UTC | #2

Hi,

> Some NVMEM devices have text based cells. In such cases MAC is stored in
> a XX:XX:XX:XX:XX:XX format. Use mac_pton() to parse such data and
> support those NVMEM cells. This is required to support e.g. a very
> popular U-Boot and its environment variables.
> 
> Signed-off-by: Rafał Miłecki <[e-mail]>
> ---
> Please let me know if checking NVMEM cell length (6 B vs. 17 B) can be
> considered a good enough solution. Alternatively we could use some DT
> property to make it explicity, e.g. something like:
> 
> ethernet@18024000 {
> 	compatible = "brcm,amac";
> 	reg = <0x18024000 0x800>;
> 
> 	nvmem-cells = <&mac_addr>;
> 	nvmem-cell-names = "mac-address";
> 	nvmem-mac-format = "text";
> };

Please note, that there is also this proposal, which had such a conversion
in mind:
https://lore.kernel.org/linux-devicetree/[e-mail]/

With this patch, there are now two different places where a mac address
format is converted. In of_get_mac_addr_nvmem() and in the imx otp driver.
And both have their shortcomings and aren't really flexible. Eg. this one
magically detects the format by comparing the length, but can't be used for
to swap bytes (because the length is also ETH_ALEN), which apparently is a
use case in the imx otp driver. And having the conversion in an nvmem
provider device driver is still a bad thing IMHO.

I'd really like to see all these kind of transformations in one place.

-michael

Jakub Kicinski Dec. 29, 2021, 6:18 p.m. UTC | #3

On Wed, 29 Dec 2021 13:40:47 +0100 Michael Walle wrote:
> > Some NVMEM devices have text based cells. In such cases MAC is stored in
> > a XX:XX:XX:XX:XX:XX format. Use mac_pton() to parse such data and
> > support those NVMEM cells. This is required to support e.g. a very
> > popular U-Boot and its environment variables.
> > 
> > Signed-off-by: Rafał Miłecki <[e-mail]>
> > ---
> > Please let me know if checking NVMEM cell length (6 B vs. 17 B) can be
> > considered a good enough solution. Alternatively we could use some DT
> > property to make it explicity, e.g. something like:
> > 
> > ethernet@18024000 {
> > 	compatible = "brcm,amac";
> > 	reg = <0x18024000 0x800>;
> > 
> > 	nvmem-cells = <&mac_addr>;
> > 	nvmem-cell-names = "mac-address";
> > 	nvmem-mac-format = "text";
> > };  
> 
> Please note, that there is also this proposal, which had such a conversion
> in mind:
> https://lore.kernel.org/linux-devicetree/[e-mail]/
> 
> With this patch, there are now two different places where a mac address
> format is converted. In of_get_mac_addr_nvmem() and in the imx otp driver.
> And both have their shortcomings and aren't really flexible. Eg. this one
> magically detects the format by comparing the length, but can't be used for
> to swap bytes (because the length is also ETH_ALEN), which apparently is a
> use case in the imx otp driver. And having the conversion in an nvmem
> provider device driver is still a bad thing IMHO.
> 
> I'd really like to see all these kind of transformations in one place.

FWIW offsetting from a common base address is relatively common, that's
why we have:

/**
 * eth_hw_addr_gen - 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
 *
 * Generate a MAC address using a base address and an offset and assign it
 * to a net_device. 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_gen(struct net_device *dev, const u8 *base_addr,
				   unsigned int id)

Michael Walle Dec. 29, 2021, 10:04 p.m. UTC | #4

Am 2021-12-29 19:18, schrieb Jakub Kicinski:
> On Wed, 29 Dec 2021 13:40:47 +0100 Michael Walle wrote:
>> > Some NVMEM devices have text based cells. In such cases MAC is stored in
>> > a XX:XX:XX:XX:XX:XX format. Use mac_pton() to parse such data and
>> > support those NVMEM cells. This is required to support e.g. a very
>> > popular U-Boot and its environment variables.
>> >
>> > Signed-off-by: Rafał Miłecki <[e-mail]>
>> > ---
>> > Please let me know if checking NVMEM cell length (6 B vs. 17 B) can be
>> > considered a good enough solution. Alternatively we could use some DT
>> > property to make it explicity, e.g. something like:
>> >
>> > ethernet@18024000 {
>> > 	compatible = "brcm,amac";
>> > 	reg = <0x18024000 0x800>;
>> >
>> > 	nvmem-cells = <&mac_addr>;
>> > 	nvmem-cell-names = "mac-address";
>> > 	nvmem-mac-format = "text";
>> > };
>> 
>> Please note, that there is also this proposal, which had such a 
>> conversion
>> in mind:
>> https://lore.kernel.org/linux-devicetree/[e-mail]/
>> 
>> With this patch, there are now two different places where a mac 
>> address
>> format is converted. In of_get_mac_addr_nvmem() and in the imx otp 
>> driver.
>> And both have their shortcomings and aren't really flexible. Eg. this 
>> one
>> magically detects the format by comparing the length, but can't be 
>> used for
>> to swap bytes (because the length is also ETH_ALEN), which apparently 
>> is a
>> use case in the imx otp driver. And having the conversion in an nvmem
>> provider device driver is still a bad thing IMHO.
>> 
>> I'd really like to see all these kind of transformations in one place.
> 
> FWIW offsetting from a common base address is relatively common, that's
> why we have:
> 
> /**
>  * eth_hw_addr_gen - 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
>  *
>  * Generate a MAC address using a base address and an offset and assign 
> it
>  * to a net_device. 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_gen(struct net_device *dev, const u8 
> *base_addr,
> 				   unsigned int id)

I didn't know that. But it doesn't help me that much because it mostly
used for switches, but in my case, I also have up to four network
cards (enetc) on the SoC; besides a network switch (felix). But
only one source for the base mac address.

-michael

Michael Walle Jan. 6, 2022, 9:23 a.m. UTC | #5

Am 2021-12-29 13:40, schrieb Michael Walle:
>> Some NVMEM devices have text based cells. In such cases MAC is stored 
>> in
>> a XX:XX:XX:XX:XX:XX format. Use mac_pton() to parse such data and
>> support those NVMEM cells. This is required to support e.g. a very
>> popular U-Boot and its environment variables.
>> 
>> Signed-off-by: Rafał Miłecki <[e-mail]>
>> ---
>> Please let me know if checking NVMEM cell length (6 B vs. 17 B) can be
>> considered a good enough solution. Alternatively we could use some DT
>> property to make it explicity, e.g. something like:
>> 
>> ethernet@18024000 {
>> 	compatible = "brcm,amac";
>> 	reg = <0x18024000 0x800>;
>> 
>> 	nvmem-cells = <&mac_addr>;
>> 	nvmem-cell-names = "mac-address";
>> 	nvmem-mac-format = "text";
>> };
> 
> Please note, that there is also this proposal, which had such a 
> conversion
> in mind:
> https://lore.kernel.org/linux-devicetree/[e-mail]/
> 
> With this patch, there are now two different places where a mac address
> format is converted. In of_get_mac_addr_nvmem() and in the imx otp 
> driver.
> And both have their shortcomings and aren't really flexible. Eg. this 
> one
> magically detects the format by comparing the length, but can't be used 
> for
> to swap bytes (because the length is also ETH_ALEN), which apparently 
> is a
> use case in the imx otp driver. And having the conversion in an nvmem
> provider device driver is still a bad thing IMHO.
> 
> I'd really like to see all these kind of transformations in one place.

Unfortunately, there were no replies yet. Can we revert this patch
until there was a discussion and before there are any users of it.
Esp. the latter is hard to track and then it might be impossible
to change them to a better solution.

Any optionions?

-michael

Patch

diff --git a/net/core/of_net.c b/net/core/of_net.c
index f1a9bf7578e7..95a64c813ae5 100644
--- a/net/core/of_net.c
+++ b/net/core/of_net.c
@@ -61,7 +61,7 @@  static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
 {
 	struct platform_device *pdev = of_find_device_by_node(np);
 	struct nvmem_cell *cell;
-	const void *mac;
+	const void *buf;
 	size_t len;
 	int ret;
 
@@ -78,21 +78,32 @@  static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
 	if (IS_ERR(cell))
 		return PTR_ERR(cell);
 
-	mac = nvmem_cell_read(cell, &len);
+	buf = nvmem_cell_read(cell, &len);
 	nvmem_cell_put(cell);
 
-	if (IS_ERR(mac))
-		return PTR_ERR(mac);
-
-	if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
-		kfree(mac);
-		return -EINVAL;
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	ret = 0;
+	if (len == ETH_ALEN) {
+		if (is_valid_ether_addr(buf))
+			memcpy(addr, buf, ETH_ALEN);
+		else
+			ret = -EINVAL;
+	} else if (len == 3 * ETH_ALEN - 1) {
+		u8 mac[ETH_ALEN];
+
+		if (mac_pton(buf, mac))
+			memcpy(addr, mac, ETH_ALEN);
+		else
+			ret = -EINVAL;
+	} else {
+		ret = -EINVAL;
 	}
 
-	memcpy(addr, mac, ETH_ALEN);
-	kfree(mac);
+	kfree(buf);
 
-	return 0;
+	return ret;
 }
 
 /**