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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |