| Message ID | 1556456002-13430-2-git-send-email-ynezz@true.cz |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | of_net: Add NVMEM support to of_get_mac_address | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Sun, Apr 28, 2019 at 02:53:19PM +0200, 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 <albeu@free.fr> > Signed-off-by: Felix Fietkau <nbd@nbd.name> > Signed-off-by: John Crispin <john@phrozen.org> > Signed-off-by: Petr Štetiar <ynezz@true.cz> > --- > > Changes since v1: > > * moved handling of nvmem after mac-address and local-mac-address properties > > drivers/of/of_net.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c > index d820f3e..8ce4f47 100644 > --- a/drivers/of/of_net.c > +++ b/drivers/of/of_net.c > @@ -8,6 +8,7 @@ > #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> > > @@ -47,12 +48,45 @@ 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 r; > + u8 mac[ETH_ALEN]; > + struct property *pp; > + struct platform_device *pdev = of_find_device_by_node(np); > + > + if (!pdev) > + return NULL; > + > + r = nvmem_get_mac_address(&pdev->dev, &mac); > + if (r < 0) > + return NULL; > + > + 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? You have the struct device ptr, so just use devm_kzalloc() if you need an allocation. > + > + return pp->value; > +free: > + kfree(pp->value); > + kfree(pp); > + > + return 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, try to get MAC > + * address from nvmem cell named 'mac-address'. If that isn't set, then the > + * obsolete 'address' is checked, just in case we're using an old device tree. > * > * 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 > @@ -77,6 +111,10 @@ const void *of_get_mac_address(struct device_node *np) > if (addr) > return addr; > > + addr = of_get_mac_addr_nvmem(np); > + if (addr) > + return addr; > + > return of_get_mac_addr(np, "address"); > } > EXPORT_SYMBOL(of_get_mac_address); > -- > 1.9.1 >
Rob Herring <robh@kernel.org> [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. 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. 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 root@OpenWrt:/# hexdump -C /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-mac-address 00000000 00 03 7f 11 52 da |....R.| 00000006 root@OpenWrt:/# ip addr show wlan0 4: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000 link/ether 00:03:7f:11:52:da brd ff:ff:ff:ff:ff:ff 1. https://patchwork.ozlabs.org/patch/1086628/ 2. https://git.openwrt.org/?p=openwrt/staging/ynezz.git;a=blob;f=target/linux/ath79/dts/ar9331_8dev_carambola2.dts;h=349c91e760ca5a56d65c587c949fed5fb6ea980e;hb=349c91e760ca5a56d65c587c949fed5fb6ea980e > You have the struct device ptr, so just use devm_kzalloc() if you need an > allocation. I'll address this in v3, thanks. -- ynezz
On Thu, May 2, 2019 at 4:05 AM Petr Štetiar <ynezz@true.cz> wrote: > > Rob Herring <robh@kernel.org> [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
Rob Herring <robh@kernel.org> [2019-05-07 11:06:43]: Hi, > > 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.) Ok, got it. > But really, my point with using devm_kzalloc() is just return the > data, not store in DT and free it when the driver unbinds. Ok, I've simply misunderstood your point, sorry, I'll handle it in the follow up fix series, along with the DT documentation update. > 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. Indeed, I did it wrong, will fix that. > '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. Ok, it makes sense now. Thanks for the detailed explanation. -- ynezz
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c index d820f3e..8ce4f47 100644 --- a/drivers/of/of_net.c +++ b/drivers/of/of_net.c @@ -8,6 +8,7 @@ #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> @@ -47,12 +48,45 @@ 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 r; + u8 mac[ETH_ALEN]; + struct property *pp; + struct platform_device *pdev = of_find_device_by_node(np); + + if (!pdev) + return NULL; + + r = nvmem_get_mac_address(&pdev->dev, &mac); + if (r < 0) + return NULL; + + 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; + + return pp->value; +free: + kfree(pp->value); + kfree(pp); + + return 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, try to get MAC + * address from nvmem cell named 'mac-address'. If that isn't set, then the + * obsolete 'address' is checked, just in case we're using an old device tree. * * 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 @@ -77,6 +111,10 @@ const void *of_get_mac_address(struct device_node *np) if (addr) return addr; + addr = of_get_mac_addr_nvmem(np); + if (addr) + return addr; + return of_get_mac_addr(np, "address"); } EXPORT_SYMBOL(of_get_mac_address);