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()")[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
