Many embedded devices have information such as MAC addresses stored
inside NVMEMs like EEPROMs and so on. Currently there are only two
drivers in the tree which benefit from NVMEM bindings.
Adding support for NVMEM into every other driver would mean adding a lot
of repetitive code. This patch allows us to configure MAC addresses in
various devices like ethernet and wireless adapters directly from
of_get_mac_address, which is already used by almost every driver in the
tree.
Predecessor of this patch which used directly MTD layer has originated
in OpenWrt some time ago and supports already about 497 use cases in 357
device tree files.
Cc: Alban Bedel <[email protected]>
Signed-off-by: Felix Fietkau <[email protected]>
Signed-off-by: John Crispin <[email protected]>
Signed-off-by: Petr Štetiar <[email protected]>
---
Changes since v1:
* moved handling of nvmem after mac-address and local-mac-address properties
Changes since v2:
* moved of_get_mac_addr_nvmem after of_get_mac_addr(np, "address") call
* replaced kzalloc, kmemdup and kfree with it's devm variants
* introduced of_has_nvmem_mac_addr helper which checks if DT node has nvmem
cell with `mac-address`
* of_get_mac_address now returns ERR_PTR encoded error value
drivers/of/of_net.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 62 insertions(+), 3 deletions(-)
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..258ceb8 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -8,8 +8,10 @@
#include <linux/etherdevice.h>
#include <linux/kernel.h>
#include <linux/of_net.h>
+#include <linux/of_platform.h>
#include <linux/phy.h>
#include <linux/export.h>
+#include <linux/device.h>
/**
* of_get_phy_mode - Get phy mode for given device_node
@@ -47,12 +49,59 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
return NULL;
}
+static const void *of_get_mac_addr_nvmem(struct device_node *np)
+{
+ int ret;
+ u8 mac[ETH_ALEN];
+ struct property *pp;
+ struct platform_device *pdev = of_find_device_by_node(np);
+
+ if (!pdev)
+ return ERR_PTR(-ENODEV);
+
+ ret = nvmem_get_mac_address(&pdev->dev, &mac);
+ if (ret)
+ return ERR_PTR(ret);
+
+ pp = devm_kzalloc(&pdev->dev, sizeof(*pp), GFP_KERNEL);
+ if (!pp)
+ return ERR_PTR(-ENOMEM);
+
+ pp->name = "nvmem-mac-address";
+ pp->length = ETH_ALEN;
+ pp->value = devm_kmemdup(&pdev->dev, mac, ETH_ALEN, GFP_KERNEL);
+ if (!pp->value) {
+ ret = -ENOMEM;
+ goto free;
+ }
+
+ ret = of_add_property(np, pp);
+ if (ret)
+ goto free;
+
+ return pp->value;
+free:
+ devm_kfree(&pdev->dev, pp->value);
+ devm_kfree(&pdev->dev, pp);
+
+ return ERR_PTR(ret);
+}
+
+static inline bool of_has_nvmem_mac_addr(struct device_node *np)
+{
+ int index = of_property_match_string(np, "nvmem-cell-names",
+ "mac-address");
+ return of_parse_phandle(np, "nvmem-cells", index) != NULL;
+}
+
/**
* Search the device tree for the best MAC address to use. 'mac-address' is
* checked first, because that is supposed to contain to "most recent" MAC
* address. If that isn't set, then 'local-mac-address' is checked next,
- * because that is the default address. If that isn't set, then the obsolete
- * 'address' is checked, just in case we're using an old device tree.
+ * because that is the default address. If that isn't set, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree. If any
+ * of the above isn't set, then try to get MAC address from nvmem cell named
+ * 'mac-address'.
*
* Note that the 'address' property is supposed to contain a virtual address of
* the register set, but some DTS files have redefined that property to be the
@@ -64,6 +113,9 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
* addresses. Some older U-Boots only initialized 'local-mac-address'. In
* this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
* but is all zeros.
+ *
+ * Return: Will be a valid pointer on success, NULL in case there wasn't
+ * 'mac-address' nvmem cell node found, and ERR_PTR in case of error.
*/
const void *of_get_mac_address(struct device_node *np)
{
@@ -77,6 +129,13 @@ const void *of_get_mac_address(struct device_node *np)
if (addr)
return addr;
- return of_get_mac_addr(np, "address");
+ addr = of_get_mac_addr(np, "address");
+ if (addr)
+ return addr;
+
+ if (!of_has_nvmem_mac_addr(np))
+ return NULL;
+
+ return of_get_mac_addr_nvmem(np);
}
EXPORT_SYMBOL(of_get_mac_address);
--
1.9.1
Hello!
On 03.05.2019 10:55, Petr Štetiar wrote:
> Many embedded devices have information such as MAC addresses stored
> inside NVMEMs like EEPROMs and so on. Currently there are only two
> drivers in the tree which benefit from NVMEM bindings.
>
> Adding support for NVMEM into every other driver would mean adding a lot
> of repetitive code. This patch allows us to configure MAC addresses in
> various devices like ethernet and wireless adapters directly from
> of_get_mac_address, which is already used by almost every driver in the
> tree.
>
> Predecessor of this patch which used directly MTD layer has originated
> in OpenWrt some time ago and supports already about 497 use cases in 357
> device tree files.
>
> Cc: Alban Bedel <[email protected]>
> Signed-off-by: Felix Fietkau <[email protected]>
> Signed-off-by: John Crispin <[email protected]>
> Signed-off-by: Petr Štetiar <[email protected]>
> ---
>
> Changes since v1:
>
> * moved handling of nvmem after mac-address and local-mac-address properties
>
> Changes since v2:
>
> * moved of_get_mac_addr_nvmem after of_get_mac_addr(np, "address") call
> * replaced kzalloc, kmemdup and kfree with it's devm variants
> * introduced of_has_nvmem_mac_addr helper which checks if DT node has nvmem
> cell with `mac-address`
> * of_get_mac_address now returns ERR_PTR encoded error value
>
> drivers/of/of_net.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index d820f3e..258ceb8 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
[...]
> @@ -64,6 +113,9 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
> * addresses. Some older U-Boots only initialized 'local-mac-address'. In
> * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
> * but is all zeros.
> + *
> + * Return: Will be a valid pointer on success, NULL in case there wasn't
> + * 'mac-address' nvmem cell node found, and ERR_PTR in case of error.
Returning both NULL and error codes on failure is usually a sign of a
misdesigned API. Why not always return an error code?
[...]
MBR, Sergei
On Fri, May 03, 2019 at 11:15:42AM +0200, Petr Štetiar wrote:
> Sergei Shtylyov <[email protected]> [2019-05-03 11:44:54]:
>
> Hi Sergei,
>
> > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > > index d820f3e..258ceb8 100644
> > > --- a/drivers/of/of_net.c
> > > +++ b/drivers/of/of_net.c
> > [...]
> > > @@ -64,6 +113,9 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
> > > * addresses. Some older U-Boots only initialized 'local-mac-address'. In
> > > * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
> > > * but is all zeros.
> > > + *
> > > + * Return: Will be a valid pointer on success, NULL in case there wasn't
> > > + * 'mac-address' nvmem cell node found, and ERR_PTR in case of error.
> >
> > Returning both NULL and error codes on failure is usually a sign of a
> > misdesigned API.
>
> well, then there's a lot of misdesigned APIs in the tree already, as I've just
> grepped for IS_ERR_OR_NULL usage and found this pointer/NULL/ERR_PTR usage
> pretty legit.
That's not really an argument though.
> > Why not always return an error code?
>
> I've received following comment[1] from Andrew:
>
> "What you have to be careful of, is the return value from your new code
> looking in NVMEM. It should only return EPROBE_DEFER, or another error
> if there really is expected to be a value in NVMEM, or getting it from
> NVMEM resulted in an error."
>
> So in order to fullfil this remark, I can't simply use ENOENT instead of
> current NULL, as the caller couldn't distinguish between ENOENT from
> of_get_mac_address or ENOENT from NVMEM subsystem.
Does it matter? You're trying to put some specific code (nvmem lookup)
behind a generic API, so the fact that you get some nvmem related
errors is more an abstraction leakage than anything else.
And if you don't really like ENOENT, ENODEV is an option as well.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Sergei Shtylyov <[email protected]> [2019-05-03 11:44:54]:
Hi Sergei,
> > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > index d820f3e..258ceb8 100644
> > --- a/drivers/of/of_net.c
> > +++ b/drivers/of/of_net.c
> [...]
> > @@ -64,6 +113,9 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
> > * addresses. Some older U-Boots only initialized 'local-mac-address'. In
> > * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
> > * but is all zeros.
> > + *
> > + * Return: Will be a valid pointer on success, NULL in case there wasn't
> > + * 'mac-address' nvmem cell node found, and ERR_PTR in case of error.
>
> Returning both NULL and error codes on failure is usually a sign of a
> misdesigned API.
well, then there's a lot of misdesigned APIs in the tree already, as I've just
grepped for IS_ERR_OR_NULL usage and found this pointer/NULL/ERR_PTR usage
pretty legit.
> Why not always return an error code?
I've received following comment[1] from Andrew:
"What you have to be careful of, is the return value from your new code
looking in NVMEM. It should only return EPROBE_DEFER, or another error
if there really is expected to be a value in NVMEM, or getting it from
NVMEM resulted in an error."
So in order to fullfil this remark, I can't simply use ENOENT instead of
current NULL, as the caller couldn't distinguish between ENOENT from
of_get_mac_address or ENOENT from NVMEM subsystem.
1. https://patchwork.ozlabs.org/patch/1092243/#2161764
-- ynezz
On Fri, May 03, 2019 at 11:15:42AM +0200, Petr Štetiar wrote:
> Sergei Shtylyov <[email protected]> [2019-05-03 11:44:54]:
>
> Hi Sergei,
>
> > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > > index d820f3e..258ceb8 100644
> > > --- a/drivers/of/of_net.c
> > > +++ b/drivers/of/of_net.c
> > [...]
> > > @@ -64,6 +113,9 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
> > > * addresses. Some older U-Boots only initialized 'local-mac-address'. In
> > > * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
> > > * but is all zeros.
> > > + *
> > > + * Return: Will be a valid pointer on success, NULL in case there wasn't
> > > + * 'mac-address' nvmem cell node found, and ERR_PTR in case of error.
> >
> > Returning both NULL and error codes on failure is usually a sign of a
> > misdesigned API.
>
> well, then there's a lot of misdesigned APIs in the tree already, as I've just
> grepped for IS_ERR_OR_NULL usage and found this pointer/NULL/ERR_PTR usage
> pretty legit.
>
> > Why not always return an error code?
>
> I've received following comment[1] from Andrew:
>
> "What you have to be careful of, is the return value from your new code
> looking in NVMEM. It should only return EPROBE_DEFER, or another error
> if there really is expected to be a value in NVMEM, or getting it from
> NVMEM resulted in an error."
>
> So in order to fullfil this remark, I can't simply use ENOENT instead of
> current NULL, as the caller couldn't distinguish between ENOENT from
> of_get_mac_address or ENOENT from NVMEM subsystem.
ENOENT and its like have to be handled special by of_get_mac_address()
for all the different ways you can find the MAC address. It means that
method does not have a MAC address, try the next. And if at the end
you have not found a MAC address, ENOENT is a good return code, it
indicates none of the methods found a MAC address.
If you are using of_get_mac_address() you don't really care where the
MAC address came from, except you expect the documented search order.
So you don't need to know that NVMEM returned ENOENT. If you do care
about that, you would not use of_get_mac_address(), but directly go to
the NVMEM.
Andrew