| Message ID | 20180718161035.7005-5-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> + if (!IS_ERR(nvmem)) { > + addr = nvmem_cell_read(nvmem, &alen); > + if (!IS_ERR(addr)) { > + from = "nvmem"; > + /* Don't use ether_addr_copy() in case we > + * didn't get the right size. > + */ Please verify the size. A short read can still give a valid MAC address, so the is_valid_ether_addr(addr) is not sufficient. > + memcpy(addrbuf, addr, alen); Another reason to check the length is that you appear to have a buffer overflow here, if alen > 6. Andrew > + kfree(addr); > + addr = addrbuf; > + } > + > + nvmem_cell_put(nvmem); > + } > + } > + > if (!addr || !is_valid_ether_addr(addr)) > return -ENODEV; > > -- > 2.17.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 18, 2018 at 06:10:34PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Many non-DT platforms read the MAC address from EEPROM. Usually it's > either done with callbacks defined in board files or from SoC-specific > ethernet drivers. > > In order to generalize this, try to read the MAC from nvmem in > eth_platform_get_mac_address() using a standard lookup name: > "mac-address". > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > net/ethernet/eth.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c > index 6b64586fd2af..adf5bd03851f 100644 > --- a/net/ethernet/eth.c > +++ b/net/ethernet/eth.c > @@ -54,6 +54,7 @@ > #include <linux/if_ether.h> > #include <linux/of_net.h> > #include <linux/pci.h> > +#include <linux/nvmem-consumer.h> > #include <net/dst.h> > #include <net/arp.h> > #include <net/sock.h> > @@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) > struct device_node *dp = dev_is_pci(dev) ? > pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node; > const unsigned char *addr = NULL; > + unsigned char addrbuf[ETH_ALEN]; > + struct nvmem_cell *nvmem; > const char *from = NULL; > + size_t alen; > > if (dp) { > addr = of_get_mac_address(dp); > @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) > from = "arch callback"; > } > > + if (!addr) { > + nvmem = nvmem_cell_get(dev, "mac-address"); > + if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER) How does EPROBE_DEFER work here? You say the use case is Non-DT. Without having DT, how do you know the cell should exist, but does not yet exist? I might be looking at old code, but i only see -EPROBE_DEFER inside the if (np) case. > + /* We may have a lookup registered for MAC address but > + * the corresponding nvmem provider hasn't been > + * registered yet. > + */ > + return -EPROBE_DEFER; You really should return real errors. If i'm reading __nvmem_device_get() right, it will return a NULL pointer when the cell does not exist. NULL is not an error, so IS_ERR() will return false. So you should return all errors from nvmem_cell_get(). Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-07-19 17:22 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: > On Wed, Jul 18, 2018 at 06:10:34PM +0200, Bartosz Golaszewski wrote: >> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> >> Many non-DT platforms read the MAC address from EEPROM. Usually it's >> either done with callbacks defined in board files or from SoC-specific >> ethernet drivers. >> >> In order to generalize this, try to read the MAC from nvmem in >> eth_platform_get_mac_address() using a standard lookup name: >> "mac-address". >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> --- >> net/ethernet/eth.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c >> index 6b64586fd2af..adf5bd03851f 100644 >> --- a/net/ethernet/eth.c >> +++ b/net/ethernet/eth.c >> @@ -54,6 +54,7 @@ >> #include <linux/if_ether.h> >> #include <linux/of_net.h> >> #include <linux/pci.h> >> +#include <linux/nvmem-consumer.h> >> #include <net/dst.h> >> #include <net/arp.h> >> #include <net/sock.h> >> @@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) >> struct device_node *dp = dev_is_pci(dev) ? >> pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node; >> const unsigned char *addr = NULL; >> + unsigned char addrbuf[ETH_ALEN]; >> + struct nvmem_cell *nvmem; >> const char *from = NULL; >> + size_t alen; >> >> if (dp) { >> addr = of_get_mac_address(dp); >> @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) >> from = "arch callback"; >> } >> >> + if (!addr) { >> + nvmem = nvmem_cell_get(dev, "mac-address"); >> + if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER) > > How does EPROBE_DEFER work here? You say the use case is > Non-DT. Without having DT, how do you know the cell should exist, but > does not yet exist? I might be looking at old code, but i only see > -EPROBE_DEFER inside the if (np) case. > >> + /* We may have a lookup registered for MAC address but >> + * the corresponding nvmem provider hasn't been >> + * registered yet. >> + */ >> + return -EPROBE_DEFER; > > You really should return real errors. If i'm reading > __nvmem_device_get() right, it will return a NULL pointer when the > cell does not exist. NULL is not an error, so IS_ERR() will return > false. So you should return all errors from nvmem_cell_get(). > > Andrew We have a patch queued for nvmem for 4.19 which adds a notion of nvmem cell lookups similar to gpio lookups: https://patchwork.kernel.org/patch/10496045/ This will work fine with probe deferral. Bart -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 18, 2018 at 06:10:34PM +0200, Bartosz Golaszewski wrote: > @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) > from = "arch callback"; > } > > + if (!addr) { > + nvmem = nvmem_cell_get(dev, "mac-address"); > + if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER) This is way too verbose. To quote Al Viro from earlier today: <viro> sigh... <viro> if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) <viro> what the hell is wrong with if (link == ERR_PTR(-EEXIST))? I wonder why so many people haven't heard of pointer comparison... ;) IS_ERR(ERR_PTR(-EPROBE_DEFER)) is always true - if it wasn't, then we'd be in for problems. So, if you're asserting that nvmem is ERR_PTR(-EPROBE_DEFER) then there's no need to do the IS_ERR(nvmem) must also be true. Hence, a simple pointer comparison is sufficient: if (nvmem == ERR_PTR(-EPROBE_DEFER)) return -EPROBE_DEFER;
2018-07-19 19:47 GMT+02:00 Russell King - ARM Linux <linux@armlinux.org.uk>: > On Wed, Jul 18, 2018 at 06:10:34PM +0200, Bartosz Golaszewski wrote: >> @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) >> from = "arch callback"; >> } >> >> + if (!addr) { >> + nvmem = nvmem_cell_get(dev, "mac-address"); >> + if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER) > > This is way too verbose. To quote Al Viro from earlier today: > > <viro> sigh... > <viro> if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) > <viro> what the hell is wrong with if (link == ERR_PTR(-EEXIST))? > > I wonder why so many people haven't heard of pointer comparison... ;) > > IS_ERR(ERR_PTR(-EPROBE_DEFER)) is always true - if it wasn't, then > we'd be in for problems. > > So, if you're asserting that nvmem is ERR_PTR(-EPROBE_DEFER) then > there's no need to do the IS_ERR(nvmem) must also be true. Hence, a > simple pointer comparison is sufficient: > > if (nvmem == ERR_PTR(-EPROBE_DEFER)) > return -EPROBE_DEFER; > Hi Russell, this issue is gone now in v3 but thanks for the example - I somehow never thought about it. Bart -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Hi Russell, > > this issue is gone now in v3 but thanks for the example - I somehow > never thought about it. Please don't send patchset so fast. It wastes people time, reviewing outdated versions. No more than one version of a patchset per day. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 6b64586fd2af..adf5bd03851f 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -54,6 +54,7 @@ #include <linux/if_ether.h> #include <linux/of_net.h> #include <linux/pci.h> +#include <linux/nvmem-consumer.h> #include <net/dst.h> #include <net/arp.h> #include <net/sock.h> @@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) struct device_node *dp = dev_is_pci(dev) ? pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node; const unsigned char *addr = NULL; + unsigned char addrbuf[ETH_ALEN]; + struct nvmem_cell *nvmem; const char *from = NULL; + size_t alen; if (dp) { addr = of_get_mac_address(dp); @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr) from = "arch callback"; } + if (!addr) { + nvmem = nvmem_cell_get(dev, "mac-address"); + if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER) + /* We may have a lookup registered for MAC address but + * the corresponding nvmem provider hasn't been + * registered yet. + */ + return -EPROBE_DEFER; + + if (!IS_ERR(nvmem)) { + addr = nvmem_cell_read(nvmem, &alen); + if (!IS_ERR(addr)) { + from = "nvmem"; + /* Don't use ether_addr_copy() in case we + * didn't get the right size. + */ + memcpy(addrbuf, addr, alen); + kfree(addr); + addr = addrbuf; + } + + nvmem_cell_put(nvmem); + } + } + if (!addr || !is_valid_ether_addr(addr)) return -ENODEV;