diff mbox series

[net-next,v4,2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

Message ID 20210412174718.17382-3-michael@walle.cc
State Not Applicable
Headers show
Series of: net: support non-platform devices in of_get_mac_address() | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Michael Walle April 12, 2021, 5:47 p.m. UTC
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>
---
 drivers/of/of_net.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Andrew Lunn April 13, 2021, 12:57 a.m. UTC | #1
On Mon, Apr 12, 2021 at 07:47:18PM +0200, Michael Walle 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Benjamin Herrenschmidt April 16, 2021, 3:24 a.m. UTC | #2
On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
> 
>  /**
>   * of_get_phy_mode - Get phy mode for given device_node
> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, const char *name, 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;
> +       }
> +

This smells like the wrong band aid :)

Any struct device can contain an OF node pointer these days.

This seems all backwards. I think we are dealing with bad evolution.

We need to do a lookup for the device because we get passed an of_node.
We should just get passed a device here... or rather stop calling
of_get_mac_addr() from all those drivers and instead call
eth_platform_get_mac_address() which in turns calls of_get_mac_addr().

Then the nvmem stuff gets put in eth_platform_get_mac_address().

of_get_mac_addr() becomes a low-level thingy that most drivers don't
care about.

Cheers,
Ben.
Michael Walle April 16, 2021, 7:29 a.m. UTC | #3
Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
> On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
>> 
>>  /**
>>   * of_get_phy_mode - Get phy mode for given device_node
>> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, 
>> const char *name, 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;
>> +       }
>> +
> 
> This smells like the wrong band aid :)
> 
> Any struct device can contain an OF node pointer these days.

But not all nodes might have an associated device, see DSA for example.
And as the name suggests of_get_mac_address() operates on a node. So
if a driver calls of_get_mac_address() it should work on the node. What
is wrong IMHO, is that the ethernet drivers where the corresponding 
board
has a nvmem_cell_lookup registered is calling of_get_mac_address(node).
It should rather call eth_get_mac_address(dev) in the first place.

One would need to figure out if there is an actual device (with an
assiciated of_node), then call eth_get_mac_address(dev) and if there
isn't a device call of_get_mac_address(node).

But I don't know if that is easy to figure out. Well, one could start
with just the device where nvmem_cell_lookup is used. Then we could
drop the workaround above.

> This seems all backwards. I think we are dealing with bad evolution.
> 
> We need to do a lookup for the device because we get passed an of_node.
> We should just get passed a device here... or rather stop calling
> of_get_mac_addr() from all those drivers and instead call
> eth_platform_get_mac_address() which in turns calls of_get_mac_addr().
> 
> Then the nvmem stuff gets put in eth_platform_get_mac_address().
> 
> of_get_mac_addr() becomes a low-level thingy that most drivers don't
> care about.

The NVMEM thing is just another (optional) way how the MAC address
is fetched from the device tree. Thus, if the drivers have the
of_get_mac_address() call they should automatically get the NVMEM
method, too.

-michael
Rob Herring April 16, 2021, 3:19 p.m. UTC | #4
On Fri, Apr 16, 2021 at 2:30 AM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
> > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
> >>
> >>  /**
> >>   * of_get_phy_mode - Get phy mode for given device_node
> >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np,
> >> const char *name, 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;
> >> +       }
> >> +
> >
> > This smells like the wrong band aid :)
> >
> > Any struct device can contain an OF node pointer these days.
>
> But not all nodes might have an associated device, see DSA for example.

I believe what Ben is saying and what I said earlier is going from dev
-> OF node is right and OF node -> dev is wrong. If you only have an
OF node, then use an of_* function.

> And as the name suggests of_get_mac_address() operates on a node. So
> if a driver calls of_get_mac_address() it should work on the node. What
> is wrong IMHO, is that the ethernet drivers where the corresponding
> board
> has a nvmem_cell_lookup registered is calling of_get_mac_address(node).
> It should rather call eth_get_mac_address(dev) in the first place.
>
> One would need to figure out if there is an actual device (with an
> assiciated of_node), then call eth_get_mac_address(dev) and if there
> isn't a device call of_get_mac_address(node).

Yes, I think we're all in agreement.

> But I don't know if that is easy to figure out. Well, one could start
> with just the device where nvmem_cell_lookup is used. Then we could
> drop the workaround above.

Start with the ones just passing dev.of_node directly:

$ git grep 'of_get_mac_address(.*of_node)'
drivers/net/ethernet/aeroflex/greth.c:          addr =
of_get_mac_address(ofdev->dev.of_node);
drivers/net/ethernet/altera/altera_tse_main.c:  macaddr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/arc/emac_main.c:   mac_addr =
of_get_mac_address(dev->of_node);
drivers/net/ethernet/broadcom/bgmac-bcma.c:             mac =
of_get_mac_address(bgmac->dev->of_node);
drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:       mac =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/ethoc.c:           mac =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/ezchip/nps_enet.c: mac_addr =
of_get_mac_address(dev->of_node);
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:  mac_addr =
of_get_mac_address(ofdev->dev.of_node);
drivers/net/ethernet/marvell/pxa168_eth.c:              mac_addr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/marvell/sky2.c:    iap =
of_get_mac_address(hw->pdev->dev.of_node);
drivers/net/ethernet/mediatek/mtk_eth_soc.c:    mac_addr =
of_get_mac_address(mac->of_node);
drivers/net/ethernet/microchip/lan743x_main.c:  mac_addr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/qualcomm/qca_spi.c:        mac =
of_get_mac_address(spi->dev.of_node);
drivers/net/ethernet/qualcomm/qca_uart.c:       mac =
of_get_mac_address(serdev->dev.of_node);
drivers/net/ethernet/wiznet/w5100-spi.c:        const void *mac =
of_get_mac_address(spi->dev.of_node);
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:      mac_addr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/xilinx/xilinx_emaclite.c:  mac_address =
of_get_mac_address(ofdev->dev.of_node);
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c: mac_addr =
of_get_mac_address(rt2x00dev->dev->of_node);
drivers/staging/octeon/ethernet.c:              mac =
of_get_mac_address(priv->of_node);
drivers/staging/wfx/main.c:             macaddr =
of_get_mac_address(wdev->dev->of_node);
net/ethernet/eth.c:             addr = of_get_mac_address(dev->of_node);

Then this will find most of the rest:
git grep -W 'of_get_mac_address([a-z]*)'| grep -E '(node|np)'

Rob
Michael Walle April 26, 2021, 10:54 a.m. UTC | #5
Am 2021-04-16 17:19, schrieb Rob Herring:
> On Fri, Apr 16, 2021 at 2:30 AM Michael Walle <michael@walle.cc> wrote:
>> 
>> Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
>> > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
>> >>
>> >>  /**
>> >>   * of_get_phy_mode - Get phy mode for given device_node
>> >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np,
>> >> const char *name, 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;
>> >> +       }
>> >> +
>> >
>> > This smells like the wrong band aid :)
>> >
>> > Any struct device can contain an OF node pointer these days.
>> 
>> But not all nodes might have an associated device, see DSA for 
>> example.
> 
> I believe what Ben is saying and what I said earlier is going from dev
> -> OF node is right and OF node -> dev is wrong. If you only have an
> OF node, then use an of_* function.
> 
>> And as the name suggests of_get_mac_address() operates on a node. So
>> if a driver calls of_get_mac_address() it should work on the node. 
>> What
>> is wrong IMHO, is that the ethernet drivers where the corresponding
>> board
>> has a nvmem_cell_lookup registered is calling 
>> of_get_mac_address(node).
>> It should rather call eth_get_mac_address(dev) in the first place.
>> 
>> One would need to figure out if there is an actual device (with an
>> assiciated of_node), then call eth_get_mac_address(dev) and if there
>> isn't a device call of_get_mac_address(node).
> 
> Yes, I think we're all in agreement.
> 
>> But I don't know if that is easy to figure out. Well, one could start
>> with just the device where nvmem_cell_lookup is used. Then we could
>> drop the workaround above.
> 
> Start with the ones just passing dev.of_node directly:
> 
> $ git grep 'of_get_mac_address(.*of_node)'

[..]

Before I'll try to come up with a patch for this, I'd like to get
your opinion on it.

(1) replacing of_get_mac_address(node) with eth_get_mac_address(dev)
     might sometimes lead to confusing comments like in
     drivers/net/ethernet/allwinner/sun4i-emac.c:

     /* Read MAC-address from DT */
     ret = of_get_mac_address(np, ndev->dev_addr);

     Do we live with that or should the new name somehow reflect that
     it is taken from the device tree.

(2) What do you think of eth_get_mac_address(ndev). That is, the
     second argument is missing and ndev->dev_addr is used.
     I'm unsure about it. We'd still need a second function for drivers
     which don't write ndev->dev_addr directly, but have some custom
     logic in between. OTOH it would be like eth_hw_addr_random(ndev).

-michael
Benjamin Herrenschmidt April 26, 2021, 11:44 p.m. UTC | #6
On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote:
> Before I'll try to come up with a patch for this, I'd like to get
> your opinion on it.
> 
> (1) replacing of_get_mac_address(node) with eth_get_mac_address(dev)
>      might sometimes lead to confusing comments like in
>      drivers/net/ethernet/allwinner/sun4i-emac.c:
> 
>      /* Read MAC-address from DT */
>      ret = of_get_mac_address(np, ndev->dev_addr);

You could leave it or turn it into "from platform", doesn't matter...

> (2) What do you think of eth_get_mac_address(ndev). That is, the

Not sure what you mean, eth_platform_get_mac_address() takes the
address as an argument. I think what you want is a consolidated
nvmem_get_mac_address + eth_platform_get_mac_address that takes a
device, which would have no requirement of the bus_type at all.

Cheers,
Ben.
Michael Walle April 28, 2021, 8:09 a.m. UTC | #7
Am 2021-04-27 01:44, schrieb Benjamin Herrenschmidt:
> On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote:
>> (2) What do you think of eth_get_mac_address(ndev). That is, the
> 
> Not sure what you mean, eth_platform_get_mac_address() takes the
> address as an argument. I think what you want is a consolidated
> nvmem_get_mac_address + eth_platform_get_mac_address that takes a
> device, which would have no requirement of the bus_type at all.

Sure. What I meant was the following:

  eth_get_mac_address(struct net_device *ndev)
vs.
  eth_get_mac_address(struct device *dev, u8 *mac_buf)

The first would assume the destination is ndev->dev_addr (which
is true for most of the calls, but not all).

-michael
diff mbox series

Patch

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index cb77b774bf76..dbac3a172a11 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
@@ -59,15 +60,39 @@  static int of_get_mac_addr(struct device_node *np, const char *name, 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);
+	memcpy(addr, mac, ETH_ALEN);
+	kfree(mac);
 
-	return ret;
+	return 0;
 }
 
 /**