diff mbox series

of_net: Implement of_get_nvmem_mac_address helper

Message ID 1521815074-30424-1-git-send-email-mike.looijmans@topic.nl
State Changes Requested, archived
Headers show
Series of_net: Implement of_get_nvmem_mac_address helper | expand

Commit Message

Mike Looijmans March 23, 2018, 2:24 p.m. UTC
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(+)

Comments

Andrew Lunn March 23, 2018, 3:11 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Looijmans March 23, 2018, 7:20 p.m. UTC | #2
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.
Florian Fainelli March 23, 2018, 7:33 p.m. UTC | #3
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.
Andrew Lunn March 23, 2018, 7:42 p.m. UTC | #4
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Looijmans March 24, 2018, 4:03 p.m. UTC | #5
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.
Mike Looijmans March 24, 2018, 4:17 p.m. UTC | #6
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)
Andrew Lunn March 24, 2018, 6:53 p.m. UTC | #7
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Looijmans March 25, 2018, 8:17 a.m. UTC | #8
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.
Andrew Lunn March 25, 2018, 9:04 p.m. UTC | #9
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Looijmans March 26, 2018, 6:54 a.m. UTC | #10
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



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

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;