On Thu, May 2, 2019 at 4:05 AM Petr Štetiar [off-list ref] wrote:

Rob Herring [off-list ref] [2019-05-01 15:19:25]:

Hi Rob,

quoted

quoted

+   struct property *pp;

...

quoted

quoted

+   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()")[1] 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[2] currently produces
following runtime DT content:

 root@OpenWrt:/# find /sys/firmware/devicetree/ -name *nvmem* -o -name *addr@*
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/eth-mac-addr@0
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/eth-mac-addr@6
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/wifi-mac-addr@1002
 /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-mac-address
 /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-cell-names
 /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-mac-address
 /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-cell-names
 /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-mac-address
 /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-cell-names

'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.

Rob