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

[Marco Felsch <m.felsch@pengutronix.de>]

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