| Message ID | 1521815074-30424-1-git-send-email-mike.looijmans@topic.nl |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | of_net: Implement of_get_nvmem_mac_address helper | expand |
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. > +/** > + * 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. Andrew
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. - The nvmem functions need the "struct device" pointer as well, while of_get_mac_address() only gets passed the DT node. One approach would be to deprecate the of_get_mac_address() interface and migrate existing drivers to the of_get_nvmem_mac_address() interface.
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? > > 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.
> 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? Documention/devicetree/bindings/net/ethernet.txt > 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. > - The nvmem functions need the "struct device" pointer as well, while > of_get_mac_address() only gets passed the DT node. Does of_nvmem_cell_get() help? Andrew
On 23-03-18 20:42, Andrew Lunn wrote: >> 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? > > Documention/devicetree/bindings/net/ethernet.txt Ok >> 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. >> - The nvmem functions need the "struct device" pointer as well, while >> of_get_mac_address() only gets passed the DT node. > > Does of_nvmem_cell_get() help? Yes, looking at the interface, it would.
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)
> 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) Hi Mike This is a nicer solution, but it is quite a lot of work, there are a lot of users. Maybe Coccinelle can help? Andrew
On 24-03-18 19:53, Andrew Lunn wrote: >> 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) > > Hi Mike > > This is a nicer solution, but it is quite a lot of work, there are a > lot of users. Maybe Coccinelle can help? About 58 of them, yeah. And this looked like such a simple thing when I started it... https://elixir.bootlin.com/linux/v4.16-rc6/ident/of_get_mac_address I have no experience with Coccinelle though.
> I have no experience with Coccinelle though.
Hi Mike
I've very little either. But all the interactions i've had with
Coccinelle people have been very friendly and helpful. It could be, if
you can describe in words what you need help with, they can write the
script to do it.
Andrew
On 25-03-18 10:17, Mike Looijmans wrote: > On 24-03-18 19:53, Andrew Lunn wrote: >>> 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) >> >> Hi Mike >> >> This is a nicer solution, but it is quite a lot of work, there are a >> lot of users. Maybe Coccinelle can help? > > About 58 of them, yeah. And this looked like such a simple thing when I > started it... > https://elixir.bootlin.com/linux/v4.16-rc6/ident/of_get_mac_address > I have no experience with Coccinelle though. > I'll at least post a v2 patch with what we've discussed so far. Another thing that just occurred to me is that the nvmem interface would also allow a MAC address to be written to NV storage. Haven't looked at that path, but it would be nice to be able to permanently program the MAC address using standard interfaces. Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c index d820f3e..316a537 100644 --- a/drivers/of/of_net.c +++ b/drivers/of/of_net.c @@ -7,6 +7,7 @@ */ #include <linux/etherdevice.h> #include <linux/kernel.h> +#include <linux/nvmem-consumer.h> #include <linux/of_net.h> #include <linux/phy.h> #include <linux/export.h> @@ -80,3 +81,50 @@ const void *of_get_mac_address(struct device_node *np) return of_get_mac_addr(np, "address"); } EXPORT_SYMBOL(of_get_mac_address); + +/** + * 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; + } + + cell = nvmem_cell_get(dev, "mac-address"); + if (IS_ERR(cell)) + return PTR_ERR(cell); + + mac = (const char *)nvmem_cell_read(cell, &len); + + nvmem_cell_put(cell); + + if (IS_ERR(mac)) + return PTR_ERR(mac); + + if (len < 6 || !is_valid_ether_addr(mac)) { + dev_err(dev, "MAC address from NVMEM is invalid\n"); + ret = -EINVAL; + } else { + ether_addr_copy(addr, mac); + ret = 0; + } + + kfree(mac); + + return ret; +} +EXPORT_SYMBOL(of_get_nvmem_mac_address); diff --git a/include/linux/of_net.h b/include/linux/of_net.h index 9cd72aa..0d52e1d 100644 --- a/include/linux/of_net.h +++ b/include/linux/of_net.h @@ -13,6 +13,7 @@ struct net_device; extern int of_get_phy_mode(struct device_node *np); extern const void *of_get_mac_address(struct device_node *np); +extern int of_get_nvmem_mac_address(struct device *dev, char *addr); extern struct net_device *of_find_net_device_by_node(struct device_node *np); #else static inline int of_get_phy_mode(struct device_node *np) @@ -25,6 +26,11 @@ static inline const void *of_get_mac_address(struct device_node *np) return NULL; } +static inline int of_get_nvmem_mac_address(struct device *dev, char *addr) +{ + return -ENODEV; +} + static inline struct net_device *of_find_net_device_by_node(struct device_node *np) { return NULL;
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(). Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- drivers/of/of_net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_net.h | 6 ++++++ 2 files changed, 54 insertions(+)