Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 23-03-18 20:33, Florian Fainelli wrote:
> On 03/23/2018 12:20 PM, Mike Looijmans wrote:
> > On 23-3-2018 16:11, Andrew Lunn wrote:
> > > On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
> > > > It's common practice to store MAC addresses for network interfaces into
> > > > nvmem devices. However the code to actually do this in the kernel lacks,
> > > > so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
> > > > address from an nvmem cell provider.
> > > >
> > > > This is particulary useful on devices where the ethernet interface
> > > > cannot
> > > > be configured by the bootloader, for example because it's in an FPGA.
> > > >
> > > > Tested by adapting the cadence macb driver to call this instead of
> > > > of_get_mac_address().
> > >
> > > Hi Mike
> > >
> > > Please can you document the device tree binding. I assume you are
> > > adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
> > > device tree.
> >
> > Indeed. I'll add my settings as an example. Where should I put this
> > documentation, in the commit comment or somewhere in
> > Documents/devicetree/bindings?
> >
> > > > +/**
> > > > + * Search the device tree for a MAC address, by calling
> > > > of_get_mac_address
> > > > + * and if that doesn't provide an address, fetch it from an nvmem
> > > > provider
> > > > + * using the name 'mac-address'.
> > > > + * On success, copies the new address is into memory pointed to by
> > > > addr and
> > > > + * returns 0. Returns a negative error code otherwise.
> > > > + * @dev:ÂÂÂ Pointer to the device containing the device_node
> > > > + * @addr:ÂÂÂ Pointer to receive the MAC address using ether_addr_copy()
> > > > + */
> > > > +int of_get_nvmem_mac_address(struct device *dev, char *addr)
> > > > +{
> > > > +ÂÂÂ const char *mac;
> > > > +ÂÂÂ struct nvmem_cell *cell;
> > > > +ÂÂÂ size_t len;
> > > > +ÂÂÂ int ret;
> > > > +
> > > > +ÂÂÂ mac = of_get_mac_address(dev->of_node);
> > > > +ÂÂÂ if (mac) {
> > > > +ÂÂÂÂÂÂÂ ether_addr_copy(addr, mac);
> > > > +ÂÂÂÂÂÂÂ return 0;
> > > > +ÂÂÂ }
> > >
> > > Is there a need to add a new API? Could of_get_mac_address() be
> > > extended to look in NVMEM? The MAC driver does not care. It is saying,
> > > using OF get me a MAC address. One API seems sufficient, and would
> > > mean you don't need to change the MAC drivers.
> >
> > It's what I intended to do, but there were two problems with that:
> > - of_get_mac_address() returns a pointer to constant data in memory, but
> > the nvmem functions return an allocated memory object that must be freed
> > after use. This changes the way the call is to be made.
>
> Yeah...
>
> > - The nvmem functions need the "struct device" pointer as well, while
> > of_get_mac_address() only gets passed the DT node.
>
> Bummer, you can't assume there is always a struct device associated with
> a struct device_node. Also, bigger question is, how can we make this
> work, for e.g: ACPI systems and therefore use an abstract fw_node handle?

Andrew Lunn's suggestion of using "of_nvmem_cell_get()" should solve this.

> > One approach would be to deprecate the of_get_mac_address() interface
> > and migrate existing drivers to the of_get_nvmem_mac_address() interface.
>
> Humm maybe, but clearly making of_get_mac_address() look for a nvmem is
> less error prone and does not require people to opt-in for the new
> helper, that seems beneficial to me.

Totally agree. But I can't think of a way that doesn't change the 
method's signature. At some point the allocated nvmem buffer must be freed.

A quick survey for the of_get_mac_address users learns that most of them 
do a memcpy (or similar) right after it, so for these drivers the 
"of_get_nvmem_mac_address" style signature that performs the memcpy (or 
better, ether_addr_copy) is a better fit, e.g.:

int of_get_mac_address(struct device_node *np, void *addr)


--
Mike Looijmans