[PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Marco Felsch]

Some vendors like Polyhex store the MAC address ASCII encoded instead of
using the plain 6-byte MAC address. This commit adds the support to
decode the 12-byte ASCII encoded MAC addresses.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/of_net.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 75a24073da51..4e74986cdda8 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
 	return -ENODEV;
 }
 
+#define ETH_ALEN_ASCII	12
+
 int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
 {
 	struct nvmem_cell *cell;
@@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
 	if (IS_ERR(mac))
 		return PTR_ERR(mac);
 
+	if (len == ETH_ALEN_ASCII) {
+		u8 *mac_new;
+		int ret;
+
+		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
+		ret = hex2bin(mac_new, mac, ETH_ALEN);
+		if (ret) {
+			kfree(mac_new);
+			kfree(mac);
+			return ret;
+		}
+
+		kfree(mac);
+		mac = mac_new;
+		len = ETH_ALEN;
+	}
+
 	if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
 		kfree(mac);
 		return -EINVAL;
-- 
2.39.2

Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Sascha Hauer]

On Mon, Aug 07, 2023 at 07:07:43PM +0200, Marco Felsch wrote:
> Some vendors like Polyhex store the MAC address ASCII encoded instead of
> using the plain 6-byte MAC address. This commit adds the support to
> decode the 12-byte ASCII encoded MAC addresses.

The upstream i.MX8MP dtsi files have "mac-address" nvmem cells described
in the device trees, but they point to a 6-byte long cell in ocotp.
These cells are not overwritten in the Polyhex dts files. How can there
be a 12-byte ASCII stored?

> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/of/of_net.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 75a24073da51..4e74986cdda8 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
>  	return -ENODEV;
>  }
>  
> +#define ETH_ALEN_ASCII	12
> +
>  int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>  {
>  	struct nvmem_cell *cell;
> @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>  	if (IS_ERR(mac))
>  		return PTR_ERR(mac);
>  
> +	if (len == ETH_ALEN_ASCII) {

I don't like this heuristic very much. If I understand the nvmem stuff
correctly then parsing of properties in non standard formats should be
fixed in a struct nvmem_cell_info::read_post_process hook.

> +		u8 *mac_new;
> +		int ret;
> +
> +		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);

If anything, then sizeof("xxxxxxxxxxxx"), but what you want here is
ETH_ALEN.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Marco Felsch]

Hi Sascha,

On 23-08-08, Sascha Hauer wrote:
> On Mon, Aug 07, 2023 at 07:07:43PM +0200, Marco Felsch wrote:
> > Some vendors like Polyhex store the MAC address ASCII encoded instead of
> > using the plain 6-byte MAC address. This commit adds the support to
> > decode the 12-byte ASCII encoded MAC addresses.
> 
> The upstream i.MX8MP dtsi files have "mac-address" nvmem cells described
> in the device trees, but they point to a 6-byte long cell in ocotp.
> These cells are not overwritten in the Polyhex dts files. How can there
> be a 12-byte ASCII stored?

Please have a look at:

https://lore.kernel.org/all/20230807171513.156907-4-m.felsch@pengutronix.de/

and search for ethmac{1,2}. Once the devicetree is upstream I will sync
our internal -upstream variant.

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/of/of_net.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > index 75a24073da51..4e74986cdda8 100644
> > --- a/drivers/of/of_net.c
> > +++ b/drivers/of/of_net.c
> > @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
> >  	return -ENODEV;
> >  }
> >  
> > +#define ETH_ALEN_ASCII	12
> > +
> >  int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >  {
> >  	struct nvmem_cell *cell;
> > @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >  	if (IS_ERR(mac))
> >  		return PTR_ERR(mac);
> >  
> > +	if (len == ETH_ALEN_ASCII) {
> 
> I don't like this heuristic very much. If I understand the nvmem stuff
> correctly then parsing of properties in non standard formats should be
> fixed in a struct nvmem_cell_info::read_post_process hook.

IMHO there is no standard to store MAC addresses, there is an easy way
(raw address stored in 6-bytes in some nvmem reachable from the host)
and a vendor-know-it-better way. While coding I was thinking about a
property to indicate that the mac-address is stored in ascii like:

&eeprom {
	macaddr1: mac-address@0 {
		reg = <0 0xc>;
		barebox,ascii-mac-address;
	}
}

Then I thought, if someone stores the mac-address in a 12-byte field
this have to be an ascii encoded mac-address and I dropped the
barebox,ascii-mac-address property.

> > +		u8 *mac_new;
> > +		int ret;
> > +
> > +		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
> 
> If anything, then sizeof("xxxxxxxxxxxx"), but what you want here is
> ETH_ALEN.

You're right. Thanks.

Regards,
  Marco

Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Ahmad Fatoum]

Hello Marco,

On 07.08.23 19:07, Marco Felsch wrote:
> Some vendors like Polyhex store the MAC address ASCII encoded instead of
> using the plain 6-byte MAC address. This commit adds the support to
> decode the 12-byte ASCII encoded MAC addresses.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

FYI, the upstream device tree binding for this is NVMEM layout, which was only
recently added to Linux and for which barebox has no support yet.

I can understand that porting NVMEM layouts, just to get a MAC address assigned
might not be an attractive proposition, but I don't think that adding a new
barebox-specific binding is the right way here. I'd suggest, you get the
nvmem cell in board code and assign it there. There's readily available API
for that. If you are interested in a generic solution, NVMEM layouts are
the way to go IMO.

> ---
>  drivers/of/of_net.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 75a24073da51..4e74986cdda8 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
>  	return -ENODEV;
>  }
>  
> +#define ETH_ALEN_ASCII	12
> +
>  int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>  {
>  	struct nvmem_cell *cell;
> @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>  	if (IS_ERR(mac))
>  		return PTR_ERR(mac);
>  
> +	if (len == ETH_ALEN_ASCII) {
> +		u8 *mac_new;
> +		int ret;
> +
> +		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
> +		ret = hex2bin(mac_new, mac, ETH_ALEN);

Why not parse into a fixed size local buffer and then copy it? Would save
you the extra allocation.

> +		if (ret) {
> +			kfree(mac_new);
> +			kfree(mac);
> +			return ret;
> +		}
> +
> +		kfree(mac);
> +		mac = mac_new;
> +		len = ETH_ALEN;
> +	}
> +
>  	if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
>  		kfree(mac);
>  		return -EINVAL;

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Marco Felsch]

Hi Ahmad,

On 23-08-08, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 07.08.23 19:07, Marco Felsch wrote:
> > Some vendors like Polyhex store the MAC address ASCII encoded instead of
> > using the plain 6-byte MAC address. This commit adds the support to
> > decode the 12-byte ASCII encoded MAC addresses.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> FYI, the upstream device tree binding for this is NVMEM layout, which was only
> recently added to Linux and for which barebox has no support yet.

I know that, thanks for the info :) I thought that this is no "layout"
it's just the mac-address stored in ASCII instead of plain 6-byte
storage.

> I can understand that porting NVMEM layouts, just to get a MAC address assigned
> might not be an attractive proposition, but I don't think that adding a new
> barebox-specific binding is the right way here.

Me neither therefore I dropped the barebox specific binding and did just
do some heuristic.

> I'd suggest, you get the nvmem cell in board code and assign it there.
> There's readily available API for that. If you are interested in a
> generic solution, NVMEM layouts are the way to go IMO.

Thought about that too but went this way because it's much less code
than doing it in the board code. Also it allows to share the code with
others.

As said, I don't think that this is a layout. Of course there are more
ASCII strings to store the production test result but this is not
relevant. I really need to check which is more effort
board-code vs. layout-support if you think that this is layout.

> > ---
> >  drivers/of/of_net.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > index 75a24073da51..4e74986cdda8 100644
> > --- a/drivers/of/of_net.c
> > +++ b/drivers/of/of_net.c
> > @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
> >  	return -ENODEV;
> >  }
> >  
> > +#define ETH_ALEN_ASCII	12
> > +
> >  int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >  {
> >  	struct nvmem_cell *cell;
> > @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >  	if (IS_ERR(mac))
> >  		return PTR_ERR(mac);
> >  
> > +	if (len == ETH_ALEN_ASCII) {
> > +		u8 *mac_new;
> > +		int ret;
> > +
> > +		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
> > +		ret = hex2bin(mac_new, mac, ETH_ALEN);
> 
> Why not parse into a fixed size local buffer and then copy it? Would save
> you the extra allocation.

I went this way to keep the below free logic the same.

Regards,
  Marco

Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Ahmad Fatoum]

Hello Marco,

On 08.08.23 11:46, Marco Felsch wrote:
> Hi Ahmad,
> 
> On 23-08-08, Ahmad Fatoum wrote:
>> Hello Marco,
>>
>> On 07.08.23 19:07, Marco Felsch wrote:
>>> Some vendors like Polyhex store the MAC address ASCII encoded instead of
>>> using the plain 6-byte MAC address. This commit adds the support to
>>> decode the 12-byte ASCII encoded MAC addresses.
>>>
>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>
>> FYI, the upstream device tree binding for this is NVMEM layout, which was only
>> recently added to Linux and for which barebox has no support yet.
> 
> I know that, thanks for the info :) I thought that this is no "layout"
> it's just the mac-address stored in ASCII instead of plain 6-byte
> storage.

Sequential big-endian 6 bytes is the normal format. Anything else (ASCII
with nothing between it, ASCII with :, ASCII with -) is a different layout IMO.

>> I can understand that porting NVMEM layouts, just to get a MAC address assigned
>> might not be an attractive proposition, but I don't think that adding a new
>> barebox-specific binding is the right way here.
> 
> Me neither therefore I dropped the barebox specific binding and did just
> do some heuristic.

It's a binding, whether you use a boolean property, the size in the reg field
or add a nvmem-layout subnode.

>> I'd suggest, you get the nvmem cell in board code and assign it there.
>> There's readily available API for that. If you are interested in a
>> generic solution, NVMEM layouts are the way to go IMO.
> 
> Thought about that too but went this way because it's much less code
> than doing it in the board code. Also it allows to share the code with
> others.

How widespread is it to store MAC address that way? If it's just Debix doing
it this way, you are effectively adding a binding that's only useful to Debix
into common code.
 
> As said, I don't think that this is a layout. Of course there are more
> ASCII strings to store the production test result but this is not
> relevant. I really need to check which is more effort
> board-code vs. layout-support if you think that this is layout.

I'd be more amenable to this patch if there exists no way in upstream bindings
to represent this, which was for a long time the case. That's not the case any
more, so we should not add any new barebox-specific bindings for MAC addresses
that duplicate what's achievable by the upstream binding.

For an example of how to do this in board code, see rdu_eth_register_ethaddr().

Cheers,
Ahmad

> 
>>> ---
>>>  drivers/of/of_net.c | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
>>> index 75a24073da51..4e74986cdda8 100644
>>> --- a/drivers/of/of_net.c
>>> +++ b/drivers/of/of_net.c
>>> @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
>>>  	return -ENODEV;
>>>  }
>>>  
>>> +#define ETH_ALEN_ASCII	12
>>> +
>>>  int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>>>  {
>>>  	struct nvmem_cell *cell;
>>> @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>>>  	if (IS_ERR(mac))
>>>  		return PTR_ERR(mac);
>>>  
>>> +	if (len == ETH_ALEN_ASCII) {
>>> +		u8 *mac_new;
>>> +		int ret;
>>> +
>>> +		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
>>> +		ret = hex2bin(mac_new, mac, ETH_ALEN);
>>
>> Why not parse into a fixed size local buffer and then copy it? Would save
>> you the extra allocation.
> 
> I went this way to keep the below free logic the same.
> 
> Regards,
>   Marco
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Marco Felsch]

On 23-08-08, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 08.08.23 11:46, Marco Felsch wrote:
> > Hi Ahmad,
> > 
> > On 23-08-08, Ahmad Fatoum wrote:
> >> Hello Marco,
> >>
> >> On 07.08.23 19:07, Marco Felsch wrote:
> >>> Some vendors like Polyhex store the MAC address ASCII encoded instead of
> >>> using the plain 6-byte MAC address. This commit adds the support to
> >>> decode the 12-byte ASCII encoded MAC addresses.
> >>>
> >>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> >>
> >> FYI, the upstream device tree binding for this is NVMEM layout, which was only
> >> recently added to Linux and for which barebox has no support yet.
> > 
> > I know that, thanks for the info :) I thought that this is no "layout"
> > it's just the mac-address stored in ASCII instead of plain 6-byte
> > storage.
> 
> Sequential big-endian 6 bytes is the normal format. Anything else (ASCII
> with nothing between it, ASCII with :, ASCII with -) is a different layout IMO.

You're right.

> >> I can understand that porting NVMEM layouts, just to get a MAC address assigned
> >> might not be an attractive proposition, but I don't think that adding a new
> >> barebox-specific binding is the right way here.
> > 
> > Me neither therefore I dropped the barebox specific binding and did just
> > do some heuristic.
> 
> It's a binding, whether you use a boolean property, the size in the reg field
> or add a nvmem-layout subnode.

To be clear, the binding under
"Documentation/devicetree/bindings/net/ethernet-controller.yaml" just
says that the nvmem-cells is <1> and the nvmem-cells-names must be set to
"mac-address". The binding don't say how much space the mac-address
storage occupy.

> >> I'd suggest, you get the nvmem cell in board code and assign it there.
> >> There's readily available API for that. If you are interested in a
> >> generic solution, NVMEM layouts are the way to go IMO.
> > 
> > Thought about that too but went this way because it's much less code
> > than doing it in the board code. Also it allows to share the code with
> > others.
> 
> How widespread is it to store MAC address that way? If it's just Debix doing
> it this way, you are effectively adding a binding that's only useful to Debix
> into common code.

Debix is the first I'm aware of. I know that this is common code. As
said above the binding don't stop us from having a larger nvmem-cell
size.

> > As said, I don't think that this is a layout. Of course there are more
> > ASCII strings to store the production test result but this is not
> > relevant. I really need to check which is more effort
> > board-code vs. layout-support if you think that this is layout.
> 
> I'd be more amenable to this patch if there exists no way in upstream bindings
> to represent this, which was for a long time the case. That's not the case any
> more, so we should not add any new barebox-specific bindings for MAC addresses
> that duplicate what's achievable by the upstream binding.
> 
> For an example of how to do this in board code, see rdu_eth_register_ethaddr().

I will move it to the board code, thanks for the link.

Regards,
  Marco