| Message ID | 20210405164643.21130-3-michael@walle.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | of: net: support non-platform devices in of_get_mac_address() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi Michael > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > { > struct platform_device *pdev = of_find_device_by_node(np); > + struct nvmem_cell *cell; > + const void *mac; > + size_t len; > int ret; > > - if (!pdev) > - return -ENODEV; > + /* Try lookup by device first, there might be a nvmem_cell_lookup > + * associated with a given device. > + */ > + if (pdev) { > + ret = nvmem_get_mac_address(&pdev->dev, addr); > + put_device(&pdev->dev); > + return ret; > + } Can you think of any odd corner case where nvmem_get_mac_address() would fail, but of_nvmem_cell_get(np, "mac-address") would work? Andrew
Hi Andrew, Am 2021-04-05 23:34, schrieb Andrew Lunn: >> -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) >> +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) >> { >> struct platform_device *pdev = of_find_device_by_node(np); >> + struct nvmem_cell *cell; >> + const void *mac; >> + size_t len; >> int ret; >> >> - if (!pdev) >> - return -ENODEV; >> + /* Try lookup by device first, there might be a nvmem_cell_lookup >> + * associated with a given device. >> + */ >> + if (pdev) { >> + ret = nvmem_get_mac_address(&pdev->dev, addr); >> + put_device(&pdev->dev); >> + return ret; >> + } > > Can you think of any odd corner case where nvmem_get_mac_address() > would fail, but of_nvmem_cell_get(np, "mac-address") would work? You mean, it might make sense to just return here when nvmem_get_mac_address() will succeed and fall back to the of_nvmem_cell_get() in case of an error? nvmem_get_mac_address() will first try to do the lookup by the of_node of pdev->dev; and because np is used to find the pdev, it should work for the same cases where of_nvmem_cell_get(np) will work. I'm fine with either, maybe the fallback to of_nvmem_cell_get() is clearer. -michael
On Mon, Apr 05, 2021 at 11:46:04PM +0200, Michael Walle wrote: > Hi Andrew, > > Am 2021-04-05 23:34, schrieb Andrew Lunn: > > > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) > > > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > > > { > > > struct platform_device *pdev = of_find_device_by_node(np); > > > + struct nvmem_cell *cell; > > > + const void *mac; > > > + size_t len; > > > int ret; > > > > > > - if (!pdev) > > > - return -ENODEV; > > > + /* Try lookup by device first, there might be a nvmem_cell_lookup > > > + * associated with a given device. > > > + */ > > > + if (pdev) { > > > + ret = nvmem_get_mac_address(&pdev->dev, addr); > > > + put_device(&pdev->dev); > > > + return ret; > > > + } > > > > Can you think of any odd corner case where nvmem_get_mac_address() > > would fail, but of_nvmem_cell_get(np, "mac-address") would work? > > You mean, it might make sense to just return here when > nvmem_get_mac_address() will succeed and fall back to the > of_nvmem_cell_get() in case of an error? I've not read the documentation for nvmem_get_mac_address(). I was thinking we might want to return real errors, and -EPROBE_DEFER. But maybe with -ENODEV we should try of_nvmem_cell_get()? But i'm not sure if there are any real use cases? The only thing i can think of is if np points to something deeper inside the device tree than what pdev does? Andrew
Am 2021-04-06 00:13, schrieb Andrew Lunn: > On Mon, Apr 05, 2021 at 11:46:04PM +0200, Michael Walle wrote: >> Hi Andrew, >> >> Am 2021-04-05 23:34, schrieb Andrew Lunn: >> > > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) >> > > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) >> > > { >> > > struct platform_device *pdev = of_find_device_by_node(np); >> > > + struct nvmem_cell *cell; >> > > + const void *mac; >> > > + size_t len; >> > > int ret; >> > > >> > > - if (!pdev) >> > > - return -ENODEV; >> > > + /* Try lookup by device first, there might be a nvmem_cell_lookup >> > > + * associated with a given device. >> > > + */ >> > > + if (pdev) { >> > > + ret = nvmem_get_mac_address(&pdev->dev, addr); >> > > + put_device(&pdev->dev); >> > > + return ret; >> > > + } >> > >> > Can you think of any odd corner case where nvmem_get_mac_address() >> > would fail, but of_nvmem_cell_get(np, "mac-address") would work? >> >> You mean, it might make sense to just return here when >> nvmem_get_mac_address() will succeed and fall back to the >> of_nvmem_cell_get() in case of an error? > > I've not read the documentation for nvmem_get_mac_address(). I was > thinking we might want to return real errors, and -EPROBE_DEFER. I can't follow, nvmem_get_mac_address() should already return those. > But maybe with -ENODEV we should try of_nvmem_cell_get()? And if this happens - that is nvmem_get_mac_address(&pdev->dev) returns -ENODEV - then of_nvmem_cell_get(np) will also return -ENODEV. Because pdev->dev.of_node == np and nvmem_get_mac_address(&pdev->dev) tries of_nvmem_cell_get(pdev->dev.of_node) first. > But i'm not sure if there are any real use cases? The only thing i can > think of is if np points to something deeper inside the device tree > than what pdev does? But then pdev will be NULL and nvmem_get_mac_address() won't be called at all, no? -michael
> But then pdev will be NULL and nvmem_get_mac_address() won't be called > at all, no? Forget it, it can be added later if there is a real use case. Andrew
On Mon, Apr 5, 2021 at 11:47 AM Michael Walle <michael@walle.cc> wrote: > > of_get_mac_address() already supports fetching the MAC address by an > nvmem provider. But until now, it was just working for platform devices. > Esp. it was not working for DSA ports and PCI devices. It gets more > common that PCI devices have a device tree binding since SoCs contain > integrated root complexes. > > Use the nvmem of_* binding to fetch the nvmem cells by a struct > device_node. We still have to try to read the cell by device first > because there might be a nvmem_cell_lookup associated with that device. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > Please note, that I've kept the nvmem_get_mac_address() which operates > on a device. The new of_get_mac_addr_nvmem() is almost identical and > there are no users of the former function right now, but it seems to be > the "newer" version to get the MAC address for a "struct device". Thus > I've kept it. Please advise, if I should kill it though. It seems kind of backwards from how we normally design this type of API where the API with a struct device will call a firmware specific version if there's a firmware handle. But certainly, I don't think we should be operating on platform device if we can help it. > drivers/of/of_net.c | 37 +++++++++++++++++++++++++++++++------ > 1 file changed, 31 insertions(+), 6 deletions(-) > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c > index 2344ad7fff5e..2323c6063eaf 100644 > --- a/drivers/of/of_net.c > +++ b/drivers/of/of_net.c > @@ -11,6 +11,7 @@ > #include <linux/phy.h> > #include <linux/export.h> > #include <linux/device.h> > +#include <linux/nvmem-consumer.h> > > /** > * of_get_phy_mode - Get phy mode for given device_node > @@ -56,18 +57,42 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr) > return -ENODEV; > } > > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > { > struct platform_device *pdev = of_find_device_by_node(np); > + struct nvmem_cell *cell; > + const void *mac; > + size_t len; > int ret; > > - if (!pdev) > - return -ENODEV; > + /* Try lookup by device first, there might be a nvmem_cell_lookup > + * associated with a given device. > + */ > + if (pdev) { > + ret = nvmem_get_mac_address(&pdev->dev, addr); > + put_device(&pdev->dev); > + return ret; > + } > + > + cell = of_nvmem_cell_get(np, "mac-address"); > + if (IS_ERR(cell)) > + return PTR_ERR(cell); > + > + mac = nvmem_cell_read(cell, &len); > + nvmem_cell_put(cell); > + > + if (IS_ERR(mac)) > + return PTR_ERR(mac); > + > + if (len != ETH_ALEN || !is_valid_ether_addr(mac)) { > + kfree(mac); > + return -EINVAL; > + } > > - ret = nvmem_get_mac_address(&pdev->dev, addr); > - put_device(&pdev->dev); > + ether_addr_copy(addr, mac); > + kfree(mac); > > - return ret; > + return 0; > } > > /** > -- > 2.20.1 >
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c index 2344ad7fff5e..2323c6063eaf 100644 --- a/drivers/of/of_net.c +++ b/drivers/of/of_net.c @@ -11,6 +11,7 @@ #include <linux/phy.h> #include <linux/export.h> #include <linux/device.h> +#include <linux/nvmem-consumer.h> /** * of_get_phy_mode - Get phy mode for given device_node @@ -56,18 +57,42 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr) return -ENODEV; } -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) { struct platform_device *pdev = of_find_device_by_node(np); + struct nvmem_cell *cell; + const void *mac; + size_t len; int ret; - if (!pdev) - return -ENODEV; + /* Try lookup by device first, there might be a nvmem_cell_lookup + * associated with a given device. + */ + if (pdev) { + ret = nvmem_get_mac_address(&pdev->dev, addr); + put_device(&pdev->dev); + return ret; + } + + cell = of_nvmem_cell_get(np, "mac-address"); + if (IS_ERR(cell)) + return PTR_ERR(cell); + + mac = nvmem_cell_read(cell, &len); + nvmem_cell_put(cell); + + if (IS_ERR(mac)) + return PTR_ERR(mac); + + if (len != ETH_ALEN || !is_valid_ether_addr(mac)) { + kfree(mac); + return -EINVAL; + } - ret = nvmem_get_mac_address(&pdev->dev, addr); - put_device(&pdev->dev); + ether_addr_copy(addr, mac); + kfree(mac); - return ret; + return 0; } /**
of_get_mac_address() already supports fetching the MAC address by an nvmem provider. But until now, it was just working for platform devices. Esp. it was not working for DSA ports and PCI devices. It gets more common that PCI devices have a device tree binding since SoCs contain integrated root complexes. Use the nvmem of_* binding to fetch the nvmem cells by a struct device_node. We still have to try to read the cell by device first because there might be a nvmem_cell_lookup associated with that device. Signed-off-by: Michael Walle <michael@walle.cc> --- Please note, that I've kept the nvmem_get_mac_address() which operates on a device. The new of_get_mac_addr_nvmem() is almost identical and there are no users of the former function right now, but it seems to be the "newer" version to get the MAC address for a "struct device". Thus I've kept it. Please advise, if I should kill it though. drivers/of/of_net.c | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-)