Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address
From: Rob Herring
Date: Tue May 07 2019 - 12:07:52 EST
On Thu, May 2, 2019 at 4:05 AM Petr Åtetiar <ynezz@xxxxxxx> wrote:
> Rob Herring <robh@xxxxxxxxxx> [2019-05-01 15:19:25]:
> Hi Rob,
> > > + struct property *pp;
> > > + pp = kzalloc(sizeof(*pp), GFP_KERNEL);
> > > + if (!pp)
> > > + return NULL;
> > > +
> > > + pp->name = "nvmem-mac-address";
> > > + pp->length = ETH_ALEN;
> > > + pp->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
> > > + if (!pp->value || of_add_property(np, pp))
> > > + goto free;
> > Why add this to the DT?
> I've just carried it over from v1 ("of_net: add mtd-mac-address support to
> of_get_mac_address()") as nobody objected about this so far.
That's not really a reason...
> Honestly I don't know if it's necessary to have it, but so far address,
> mac-address and local-mac-address properties provide this DT nodes, so I've
> simply thought, that it would be good to have it for MAC address from NVMEM as
> well in order to stay consistent.
If you want to be consistent, then fill in 'local-mac-address' with
the value from nvmem. We don't need the same thing with a new name
added to DT. (TBC, I'm not suggesting you do that here.)
But really, my point with using devm_kzalloc() is just return the
data, not store in DT and free it when the driver unbinds. Allocating
it with devm_kzalloc AND adding it to DT as you've done in v4 leads to
2 entities refcounting the allocation. If the driver unbinds, the
buffer is freed, but DT code is still referencing that memory.
Also, what happens the 2 time a driver binds? The property would
already be in the DT.
> Just FYI, my testing ar9331_8dev_carambola2.dts currently produces
> following runtime DT content:
> root@OpenWrt:/# find /sys/firmware/devicetree/ -name *nvmem* -o -name *addr@*
'nvmem-mac-address' is not a documented property. That would need to
be documented before using upstream. Though, for reasons above, I
don't think it should be.