[PATCHv6 net-next 0/7] ibm: emac: more cleanups

[PATCHv6 net-next 0/7] ibm: emac: more cleanups

[Rosen Penev]

Tested on Cisco MX60W.

v2: fixed build errors. Also added extra commits to clean the driver up
further.
v3: Added tested message. Removed bad alloc_netdev_dummy commit.
v4: removed modules changes from patchset. Added fix for if MAC not
found.
v5: added of_find_matching_node commit.
v6: resend after net-next merge.

Rosen Penev (7):
  net: ibm: emac: use netif_receive_skb_list
  net: ibm: emac: remove custom init/exit functions
  net: ibm: emac: use devm_platform_ioremap_resource
  net: ibm: emac: use platform_get_irq
  net: ibm: emac: use devm for mutex_init
  net: ibm: emac: generate random MAC if not found
  net: ibm: emac: use of_find_matching_node

 drivers/net/ethernet/ibm/emac/core.c  | 91 ++++++++-------------------
 drivers/net/ethernet/ibm/emac/mal.c   | 10 +--
 drivers/net/ethernet/ibm/emac/mal.h   |  4 --
 drivers/net/ethernet/ibm/emac/rgmii.c | 10 +--
 drivers/net/ethernet/ibm/emac/rgmii.h |  4 --
 drivers/net/ethernet/ibm/emac/tah.c   | 10 +--
 drivers/net/ethernet/ibm/emac/tah.h   |  4 --
 drivers/net/ethernet/ibm/emac/zmii.c  | 10 +--
 drivers/net/ethernet/ibm/emac/zmii.h  |  4 --
 9 files changed, 30 insertions(+), 117 deletions(-)

-- 
2.47.0

[PATCHv6 net-next 1/7] net: ibm: emac: use netif_receive_skb_list

[Rosen Penev]

Small rx improvement. Would use napi_gro_receive instead but that's a
lot more involved than netif_receive_skb_list because of how the
function is implemented.

Before:

> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 51556 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.04 sec   559 MBytes   467 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 48228 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.03 sec   558 MBytes   467 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 47600 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.04 sec   557 MBytes   466 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 37252 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.05 sec   559 MBytes   467 Mbits/sec

After:

> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 40786 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.05 sec   572 MBytes   478 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 52482 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.04 sec   571 MBytes   477 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 48370 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.04 sec   572 MBytes   478 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 46086 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.05 sec   571 MBytes   476 Mbits/sec
> iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  1] local 192.168.1.101 port 46062 connected with 192.168.1.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  1] 0.00-10.04 sec   572 MBytes   478 Mbits/sec

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index dadd987efb6b..0edcb435e62f 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -1727,6 +1727,7 @@ static inline int emac_rx_sg_append(struct emac_instance *dev, int slot)
 /* NAPI poll context */
 static int emac_poll_rx(void *param, int budget)
 {
+	LIST_HEAD(rx_list);
 	struct emac_instance *dev = param;
 	int slot = dev->rx_slot, received = 0;
 
@@ -1783,8 +1784,7 @@ static int emac_poll_rx(void *param, int budget)
 		skb->protocol = eth_type_trans(skb, dev->ndev);
 		emac_rx_csum(dev, skb, ctrl);
 
-		if (unlikely(netif_receive_skb(skb) == NET_RX_DROP))
-			++dev->estats.rx_dropped_stack;
+		list_add_tail(&skb->list, &rx_list);
 	next:
 		++dev->stats.rx_packets;
 	skip:
@@ -1828,6 +1828,8 @@ static int emac_poll_rx(void *param, int budget)
 		goto next;
 	}
 
+	netif_receive_skb_list(&rx_list);
+
 	if (received) {
 		DBG2(dev, "rx %d BDs" NL, received);
 		dev->rx_slot = slot;
-- 
2.47.0

Re: [PATCHv6 net-next 1/7] net: ibm: emac: use netif_receive_skb_list

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:16PM -0700, Rosen Penev wrote:
> Small rx improvement. Would use napi_gro_receive instead but that's a
> lot more involved than netif_receive_skb_list because of how the
> function is implemented.

...

> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCHv6 net-next 2/7] net: ibm: emac: remove custom init/exit functions

[Rosen Penev]

c092d0be38f4f754cdbdc76dc6df628ca48ac0eb introduced EPROBE_DEFER
support. Because of that, we can defer initialization until all modules
are ready instead of handling it explicitly with custom init/exit
functions.

As a consequence of removing explicit module initialization and
deferring probe until everything is ready, there's no need for custom
init and exit functions.

There are now module_init and module_exit calls but no real change in
functionality as these init and exit functions are no longer directly
called by core.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c  | 39 +--------------------------
 drivers/net/ethernet/ibm/emac/mal.c   | 10 +------
 drivers/net/ethernet/ibm/emac/mal.h   |  4 ---
 drivers/net/ethernet/ibm/emac/rgmii.c | 10 +------
 drivers/net/ethernet/ibm/emac/rgmii.h |  4 ---
 drivers/net/ethernet/ibm/emac/tah.c   | 10 +------
 drivers/net/ethernet/ibm/emac/tah.h   |  4 ---
 drivers/net/ethernet/ibm/emac/zmii.c  | 10 +------
 drivers/net/ethernet/ibm/emac/zmii.h  |  4 ---
 9 files changed, 5 insertions(+), 90 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 0edcb435e62f..644abd37cfb4 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3283,42 +3283,10 @@ static void __init emac_make_bootlist(void)
 
 static int __init emac_init(void)
 {
-	int rc;
-
-	printk(KERN_INFO DRV_DESC ", version " DRV_VERSION "\n");
-
 	/* Build EMAC boot list */
 	emac_make_bootlist();
 
-	/* Init submodules */
-	rc = mal_init();
-	if (rc)
-		goto err;
-	rc = zmii_init();
-	if (rc)
-		goto err_mal;
-	rc = rgmii_init();
-	if (rc)
-		goto err_zmii;
-	rc = tah_init();
-	if (rc)
-		goto err_rgmii;
-	rc = platform_driver_register(&emac_driver);
-	if (rc)
-		goto err_tah;
-
-	return 0;
-
- err_tah:
-	tah_exit();
- err_rgmii:
-	rgmii_exit();
- err_zmii:
-	zmii_exit();
- err_mal:
-	mal_exit();
- err:
-	return rc;
+	return platform_driver_register(&emac_driver);
 }
 
 static void __exit emac_exit(void)
@@ -3327,11 +3295,6 @@ static void __exit emac_exit(void)
 
 	platform_driver_unregister(&emac_driver);
 
-	tah_exit();
-	rgmii_exit();
-	zmii_exit();
-	mal_exit();
-
 	/* Destroy EMAC boot list */
 	for (i = 0; i < EMAC_BOOT_LIST_SIZE; i++)
 		of_node_put(emac_boot_list[i]);
diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
index c634534710d9..c66adb7f4e7a 100644
--- a/drivers/net/ethernet/ibm/emac/mal.c
+++ b/drivers/net/ethernet/ibm/emac/mal.c
@@ -781,12 +781,4 @@ static struct platform_driver mal_of_driver = {
 	.remove = mal_remove,
 };
 
-int __init mal_init(void)
-{
-	return platform_driver_register(&mal_of_driver);
-}
-
-void mal_exit(void)
-{
-	platform_driver_unregister(&mal_of_driver);
-}
+module_platform_driver(mal_of_driver);
diff --git a/drivers/net/ethernet/ibm/emac/mal.h b/drivers/net/ethernet/ibm/emac/mal.h
index e0ddc41186a2..2963b36be6f5 100644
--- a/drivers/net/ethernet/ibm/emac/mal.h
+++ b/drivers/net/ethernet/ibm/emac/mal.h
@@ -252,10 +252,6 @@ static inline int mal_has_feature(struct mal_instance *dev,
 		(MAL_FTRS_POSSIBLE & dev->features & feature);
 }
 
-/* Register MAL devices */
-int mal_init(void);
-void mal_exit(void);
-
 int mal_register_commac(struct mal_instance *mal,
 			struct mal_commac *commac);
 void mal_unregister_commac(struct mal_instance *mal,
diff --git a/drivers/net/ethernet/ibm/emac/rgmii.c b/drivers/net/ethernet/ibm/emac/rgmii.c
index 317c22d09172..f275ebeb7158 100644
--- a/drivers/net/ethernet/ibm/emac/rgmii.c
+++ b/drivers/net/ethernet/ibm/emac/rgmii.c
@@ -303,12 +303,4 @@ static struct platform_driver rgmii_driver = {
 	.remove = rgmii_remove,
 };
 
-int __init rgmii_init(void)
-{
-	return platform_driver_register(&rgmii_driver);
-}
-
-void rgmii_exit(void)
-{
-	platform_driver_unregister(&rgmii_driver);
-}
+module_platform_driver(rgmii_driver);
diff --git a/drivers/net/ethernet/ibm/emac/rgmii.h b/drivers/net/ethernet/ibm/emac/rgmii.h
index 8e4e36eed172..170bcd35039b 100644
--- a/drivers/net/ethernet/ibm/emac/rgmii.h
+++ b/drivers/net/ethernet/ibm/emac/rgmii.h
@@ -52,8 +52,6 @@ struct rgmii_instance {
 
 #ifdef CONFIG_IBM_EMAC_RGMII
 
-int rgmii_init(void);
-void rgmii_exit(void);
 int rgmii_attach(struct platform_device *ofdev, int input, int mode);
 void rgmii_detach(struct platform_device *ofdev, int input);
 void rgmii_get_mdio(struct platform_device *ofdev, int input);
@@ -64,8 +62,6 @@ void *rgmii_dump_regs(struct platform_device *ofdev, void *buf);
 
 #else
 
-# define rgmii_init()		0
-# define rgmii_exit()		do { } while(0)
 # define rgmii_attach(x,y,z)	(-ENXIO)
 # define rgmii_detach(x,y)	do { } while(0)
 # define rgmii_get_mdio(o,i)	do { } while (0)
diff --git a/drivers/net/ethernet/ibm/emac/tah.c b/drivers/net/ethernet/ibm/emac/tah.c
index c605c8ff933e..77e881efa598 100644
--- a/drivers/net/ethernet/ibm/emac/tah.c
+++ b/drivers/net/ethernet/ibm/emac/tah.c
@@ -161,12 +161,4 @@ static struct platform_driver tah_driver = {
 	.remove = tah_remove,
 };
 
-int __init tah_init(void)
-{
-	return platform_driver_register(&tah_driver);
-}
-
-void tah_exit(void)
-{
-	platform_driver_unregister(&tah_driver);
-}
+module_platform_driver(tah_driver);
diff --git a/drivers/net/ethernet/ibm/emac/tah.h b/drivers/net/ethernet/ibm/emac/tah.h
index 86c2b6b9d460..60c16cf7a41a 100644
--- a/drivers/net/ethernet/ibm/emac/tah.h
+++ b/drivers/net/ethernet/ibm/emac/tah.h
@@ -68,8 +68,6 @@ struct tah_instance {
 
 #ifdef CONFIG_IBM_EMAC_TAH
 
-int tah_init(void);
-void tah_exit(void);
 int tah_attach(struct platform_device *ofdev, int channel);
 void tah_detach(struct platform_device *ofdev, int channel);
 void tah_reset(struct platform_device *ofdev);
@@ -78,8 +76,6 @@ void *tah_dump_regs(struct platform_device *ofdev, void *buf);
 
 #else
 
-# define tah_init()		0
-# define tah_exit()		do { } while(0)
 # define tah_attach(x,y)	(-ENXIO)
 # define tah_detach(x,y)	do { } while(0)
 # define tah_reset(x)		do { } while(0)
diff --git a/drivers/net/ethernet/ibm/emac/zmii.c b/drivers/net/ethernet/ibm/emac/zmii.c
index 03bab3f95fe4..211e843fdc7e 100644
--- a/drivers/net/ethernet/ibm/emac/zmii.c
+++ b/drivers/net/ethernet/ibm/emac/zmii.c
@@ -309,12 +309,4 @@ static struct platform_driver zmii_driver = {
 	.remove = zmii_remove,
 };
 
-int __init zmii_init(void)
-{
-	return platform_driver_register(&zmii_driver);
-}
-
-void zmii_exit(void)
-{
-	platform_driver_unregister(&zmii_driver);
-}
+module_platform_driver(zmii_driver);
diff --git a/drivers/net/ethernet/ibm/emac/zmii.h b/drivers/net/ethernet/ibm/emac/zmii.h
index 65daedc78594..213de06d8ea2 100644
--- a/drivers/net/ethernet/ibm/emac/zmii.h
+++ b/drivers/net/ethernet/ibm/emac/zmii.h
@@ -48,8 +48,6 @@ struct zmii_instance {
 
 #ifdef CONFIG_IBM_EMAC_ZMII
 
-int zmii_init(void);
-void zmii_exit(void);
 int zmii_attach(struct platform_device *ofdev, int input,
 		phy_interface_t *mode);
 void zmii_detach(struct platform_device *ofdev, int input);
@@ -60,8 +58,6 @@ int zmii_get_regs_len(struct platform_device *ocpdev);
 void *zmii_dump_regs(struct platform_device *ofdev, void *buf);
 
 #else
-# define zmii_init()		0
-# define zmii_exit()		do { } while(0)
 # define zmii_attach(x,y,z)	(-ENXIO)
 # define zmii_detach(x,y)	do { } while(0)
 # define zmii_get_mdio(x,y)	do { } while(0)
-- 
2.47.0

Re: [PATCHv6 net-next 2/7] net: ibm: emac: remove custom init/exit functions

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:17PM -0700, Rosen Penev wrote:
> c092d0be38f4f754cdbdc76dc6df628ca48ac0eb introduced EPROBE_DEFER

The preferred way to cite commits in patch descriptions is like this:

commit c092d0be38f4 ("net: ibm: emac: remove all waiting code")

Something like this in gitconfig can be helpful.

[core]
	abbrev = 12
[pretty]
	quote = commit %h (\"%s\")
[alias]
	quote = log -1 --pretty=quote

Then the following should work:

$ git quote c092d0be38f4f754cdbdc76dc6df628ca48ac0eb
commit 71eb7f699755 ("net: ibm: emac: use netif_receive_skb_list")

> support. Because of that, we can defer initialization until all modules
> are ready instead of handling it explicitly with custom init/exit
> functions.
> 
> As a consequence of removing explicit module initialization and
> deferring probe until everything is ready, there's no need for custom
> init and exit functions.
> 
> There are now module_init and module_exit calls but no real change in
> functionality as these init and exit functions are no longer directly
> called by core.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Otherwise, LGTM.

[PATCHv6 net-next 3/7] net: ibm: emac: use devm_platform_ioremap_resource

[Rosen Penev]

No need to have a struct resource. Gets rid of the TODO.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 644abd37cfb4..438b08e8e956 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3050,12 +3050,10 @@ static int emac_probe(struct platform_device *ofdev)
 
 	ndev->irq = dev->emac_irq;
 
-	/* Map EMAC regs */
-	// TODO : platform_get_resource() and devm_ioremap_resource()
-	dev->emacp = devm_of_iomap(&ofdev->dev, np, 0, NULL);
-	if (!dev->emacp) {
+	dev->emacp = devm_platform_ioremap_resource(ofdev, 0);
+	if (IS_ERR(dev->emacp)) {
 		dev_err(&ofdev->dev, "can't map device registers");
-		err = -ENOMEM;
+		err = PTR_ERR(dev->emacp);
 		goto err_gone;
 	}
 
-- 
2.47.0

Re: [PATCHv6 net-next 3/7] net: ibm: emac: use devm_platform_ioremap_resource

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:18PM -0700, Rosen Penev wrote:
> No need to have a struct resource. Gets rid of the TODO.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCHv6 net-next 4/7] net: ibm: emac: use platform_get_irq

[Rosen Penev]

No need for irq_of_parse_and_map since we have platform_device.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 438b08e8e956..f8478f0026af 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3031,15 +3031,8 @@ static int emac_probe(struct platform_device *ofdev)
 	if (err)
 		goto err_gone;
 
-	/* Get interrupts. EMAC irq is mandatory */
-	dev->emac_irq = irq_of_parse_and_map(np, 0);
-	if (!dev->emac_irq) {
-		printk(KERN_ERR "%pOF: Can't map main interrupt\n", np);
-		err = -ENODEV;
-		goto err_gone;
-	}
-
 	/* Setup error IRQ handler */
+	dev->emac_irq = platform_get_irq(ofdev, 0);
 	err = devm_request_irq(&ofdev->dev, dev->emac_irq, emac_irq, 0, "EMAC",
 			       dev);
 	if (err) {
-- 
2.47.0

Re: [PATCHv6 net-next 4/7] net: ibm: emac: use platform_get_irq

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:19PM -0700, Rosen Penev wrote:
> No need for irq_of_parse_and_map since we have platform_device.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCHv6 net-next 5/7] net: ibm: emac: use devm for mutex_init

[Rosen Penev]

It seems since inception that mutex_destroy was never called for these
in _remove. Instead of handling this manually, just use devm for
simplicity.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index f8478f0026af..b9ccaae61c48 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3021,8 +3021,14 @@ static int emac_probe(struct platform_device *ofdev)
 	SET_NETDEV_DEV(ndev, &ofdev->dev);
 
 	/* Initialize some embedded data structures */
-	mutex_init(&dev->mdio_lock);
-	mutex_init(&dev->link_lock);
+	err = devm_mutex_init(&ofdev->dev, &dev->mdio_lock);
+	if (err)
+		return err;
+
+	err = devm_mutex_init(&ofdev->dev, &dev->link_lock);
+	if (err)
+		return err;
+
 	spin_lock_init(&dev->lock);
 	INIT_WORK(&dev->reset_work, emac_reset_work);
 
-- 
2.47.0

Re: [PATCHv6 net-next 5/7] net: ibm: emac: use devm for mutex_init

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:20PM -0700, Rosen Penev wrote:
> It seems since inception that mutex_destroy was never called for these
> in _remove. Instead of handling this manually, just use devm for
> simplicity.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/ibm/emac/core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index f8478f0026af..b9ccaae61c48 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -3021,8 +3021,14 @@ static int emac_probe(struct platform_device *ofdev)
>  	SET_NETDEV_DEV(ndev, &ofdev->dev);
>  
>  	/* Initialize some embedded data structures */
> -	mutex_init(&dev->mdio_lock);
> -	mutex_init(&dev->link_lock);
> +	err = devm_mutex_init(&ofdev->dev, &dev->mdio_lock);
> +	if (err)
> +		return err;

Hi Rosen,

It looks like this should be:

		goto err_gone;

> +
> +	err = devm_mutex_init(&ofdev->dev, &dev->link_lock);
> +	if (err)
> +		return err;

Ditto.

> +
>  	spin_lock_init(&dev->lock);
>  	INIT_WORK(&dev->reset_work, emac_reset_work);

-- 
pw-bot: cr

[PATCHv6 net-next 6/7] net: ibm: emac: generate random MAC if not found

[Rosen Penev]

On this Cisco MX60W, u-boot sets the local-mac-address property.
Unfortunately by default, the MAC is wrong and is actually located on a
UBI partition. Which means nvmem needs to be used to grab it.

In the case where that fails, EMAC fails to initialize instead of
generating a random MAC as many other drivers do.

Match behavior with other drivers to have a working ethernet interface.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index b9ccaae61c48..faa483790b29 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -2937,9 +2937,12 @@ static int emac_init_config(struct emac_instance *dev)
 
 	/* Read MAC-address */
 	err = of_get_ethdev_address(np, dev->ndev);
-	if (err)
-		return dev_err_probe(&dev->ofdev->dev, err,
-				     "Can't get valid [local-]mac-address from OF !\n");
+	if (err == -EPROBE_DEFER)
+		return err;
+	if (err) {
+		dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
+		eth_hw_addr_random(dev->ndev);
+	}
 
 	/* IAHT and GAHT filter parameterization */
 	if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC)) {
-- 
2.47.0

Re: [PATCHv6 net-next 6/7] net: ibm: emac: generate random MAC if not found

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:21PM -0700, Rosen Penev wrote:
> On this Cisco MX60W, u-boot sets the local-mac-address property.
> Unfortunately by default, the MAC is wrong and is actually located on a
> UBI partition. Which means nvmem needs to be used to grab it.
> 
> In the case where that fails, EMAC fails to initialize instead of
> generating a random MAC as many other drivers do.
> 
> Match behavior with other drivers to have a working ethernet interface.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/ibm/emac/core.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index b9ccaae61c48..faa483790b29 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -2937,9 +2937,12 @@ static int emac_init_config(struct emac_instance *dev)
>  
>  	/* Read MAC-address */
>  	err = of_get_ethdev_address(np, dev->ndev);
> -	if (err)
> -		return dev_err_probe(&dev->ofdev->dev, err,
> -				     "Can't get valid [local-]mac-address from OF !\n");
> +	if (err == -EPROBE_DEFER)
> +		return err;
> +	if (err) {
> +		dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
> +		eth_hw_addr_random(dev->ndev);
> +	}

The above seems to take the random path for all errors other than
-EPROBE_DEFER. That seems too broad to me, and perhaps it would
be better to be more specific. Assuming the case that needs
to be covered is -EINVAL (a guess on my part), perhaps something like this
would work? (Completely untested!)

	err = of_get_ethdev_address(np, dev->ndev);
	if (err == -EINVAL) {
		/* An explanation should go here, mentioning Cisco MX60W
		 * Maybe the logic should even be specific to that hw?
		 */
		dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
		eth_hw_addr_random(dev->ndev);
	} else if (err) {
		return dev_err_probe(&dev->ofdev->dev, err,
				     "Can't get valid [local-]mac-address from OF !\n");
	}

Also, should this be a bug fix with a Fixes tag for net?

>  
>  	/* IAHT and GAHT filter parameterization */
>  	if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC)) {
> -- 
> 2.47.0
> 

Re: [PATCHv6 net-next 6/7] net: ibm: emac: generate random MAC if not found

[Rosen Penev]

On Sat, Oct 12, 2024 at 6:16 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Oct 11, 2024 at 12:56:21PM -0700, Rosen Penev wrote:
> > On this Cisco MX60W, u-boot sets the local-mac-address property.
> > Unfortunately by default, the MAC is wrong and is actually located on a
> > UBI partition. Which means nvmem needs to be used to grab it.
> >
> > In the case where that fails, EMAC fails to initialize instead of
> > generating a random MAC as many other drivers do.
> >
> > Match behavior with other drivers to have a working ethernet interface.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/ibm/emac/core.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index b9ccaae61c48..faa483790b29 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -2937,9 +2937,12 @@ static int emac_init_config(struct emac_instance *dev)
> >
> >       /* Read MAC-address */
> >       err = of_get_ethdev_address(np, dev->ndev);
> > -     if (err)
> > -             return dev_err_probe(&dev->ofdev->dev, err,
> > -                                  "Can't get valid [local-]mac-address from OF !\n");
> > +     if (err == -EPROBE_DEFER)
> > +             return err;
> > +     if (err) {
> > +             dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
> > +             eth_hw_addr_random(dev->ndev);
> > +     }
>
> The above seems to take the random path for all errors other than
> -EPROBE_DEFER. That seems too broad to me, and perhaps it would
> be better to be more specific. Assuming the case that needs
> to be covered is -EINVAL (a guess on my part), perhaps something like this
> would work? (Completely untested!)
>
>         err = of_get_ethdev_address(np, dev->ndev);
>         if (err == -EINVAL) {
>                 /* An explanation should go here, mentioning Cisco MX60W
>                  * Maybe the logic should even be specific to that hw?
>                  */
>                 dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
>                 eth_hw_addr_random(dev->ndev);
>         } else if (err) {
>                 return dev_err_probe(&dev->ofdev->dev, err,
>                                      "Can't get valid [local-]mac-address from OF !\n");
>         }
That's just yak shaving. besides 0 and EPROBE_DEFER,
of_get_ethdev_address returns ENODEV, EINVAL, and maybe something
else. I don't see a good enough reason to diverge from convention
here. This same pattern is present in other drivers.
>
> Also, should this be a bug fix with a Fixes tag for net?
No. It's more of a feature honestly.
>
> >
> >       /* IAHT and GAHT filter parameterization */
> >       if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC)) {
> > --
> > 2.47.0
> >

Re: [PATCHv6 net-next 6/7] net: ibm: emac: generate random MAC if not found

[Simon Horman]

On Tue, Oct 15, 2024 at 12:44:52PM -0700, Rosen Penev wrote:
> On Sat, Oct 12, 2024 at 6:16 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Fri, Oct 11, 2024 at 12:56:21PM -0700, Rosen Penev wrote:
> > > On this Cisco MX60W, u-boot sets the local-mac-address property.
> > > Unfortunately by default, the MAC is wrong and is actually located on a
> > > UBI partition. Which means nvmem needs to be used to grab it.
> > >
> > > In the case where that fails, EMAC fails to initialize instead of
> > > generating a random MAC as many other drivers do.
> > >
> > > Match behavior with other drivers to have a working ethernet interface.
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  drivers/net/ethernet/ibm/emac/core.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > > index b9ccaae61c48..faa483790b29 100644
> > > --- a/drivers/net/ethernet/ibm/emac/core.c
> > > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > > @@ -2937,9 +2937,12 @@ static int emac_init_config(struct emac_instance *dev)
> > >
> > >       /* Read MAC-address */
> > >       err = of_get_ethdev_address(np, dev->ndev);
> > > -     if (err)
> > > -             return dev_err_probe(&dev->ofdev->dev, err,
> > > -                                  "Can't get valid [local-]mac-address from OF !\n");
> > > +     if (err == -EPROBE_DEFER)
> > > +             return err;
> > > +     if (err) {
> > > +             dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
> > > +             eth_hw_addr_random(dev->ndev);
> > > +     }
> >
> > The above seems to take the random path for all errors other than
> > -EPROBE_DEFER. That seems too broad to me, and perhaps it would
> > be better to be more specific. Assuming the case that needs
> > to be covered is -EINVAL (a guess on my part), perhaps something like this
> > would work? (Completely untested!)
> >
> >         err = of_get_ethdev_address(np, dev->ndev);
> >         if (err == -EINVAL) {
> >                 /* An explanation should go here, mentioning Cisco MX60W
> >                  * Maybe the logic should even be specific to that hw?
> >                  */
> >                 dev_warn(&dev->ofdev->dev, "Can't get valid mac-address. Generating random.");
> >                 eth_hw_addr_random(dev->ndev);
> >         } else if (err) {
> >                 return dev_err_probe(&dev->ofdev->dev, err,
> >                                      "Can't get valid [local-]mac-address from OF !\n");
> >         }
> That's just yak shaving. besides 0 and EPROBE_DEFER,
> of_get_ethdev_address returns ENODEV, EINVAL, and maybe something
> else. I don't see a good enough reason to diverge from convention
> here. This same pattern is present in other drivers.

I assumed that more specific error detection would be appropriate, because
it seemed to be a rather specific case.  But if the pattern is present in
other drivers, then that is fine.

> >
> > Also, should this be a bug fix with a Fixes tag for net?
> No. It's more of a feature honestly.
> >
> > >
> > >       /* IAHT and GAHT filter parameterization */
> > >       if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC)) {
> > > --
> > > 2.47.0
> > >
> 

[PATCHv6 net-next 7/7] net: ibm: emac: use of_find_matching_node

[Rosen Penev]

Cleaner than using of_find_all_nodes and then of_match_node.

Also modified EMAC_BOOT_LIST_SIZE check to run before of_node_get to
avoid having to call of_node_put on failure.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index faa483790b29..5265616400c2 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3253,21 +3253,17 @@ static void __init emac_make_bootlist(void)
 	int cell_indices[EMAC_BOOT_LIST_SIZE];
 
 	/* Collect EMACs */
-	while((np = of_find_all_nodes(np)) != NULL) {
+	while((np = of_find_matching_node(np, emac_match))) {
 		u32 idx;
 
-		if (of_match_node(emac_match, np) == NULL)
-			continue;
 		if (of_property_read_bool(np, "unused"))
 			continue;
 		if (of_property_read_u32(np, "cell-index", &idx))
 			continue;
 		cell_indices[i] = idx;
-		emac_boot_list[i++] = of_node_get(np);
-		if (i >= EMAC_BOOT_LIST_SIZE) {
-			of_node_put(np);
+		if (i >= EMAC_BOOT_LIST_SIZE)
 			break;
-		}
+		emac_boot_list[i++] = of_node_get(np);
 	}
 	max = i;
 
-- 
2.47.0

Re: [PATCHv6 net-next 7/7] net: ibm: emac: use of_find_matching_node

[Simon Horman]

On Fri, Oct 11, 2024 at 12:56:22PM -0700, Rosen Penev wrote:
> Cleaner than using of_find_all_nodes and then of_match_node.
> 
> Also modified EMAC_BOOT_LIST_SIZE check to run before of_node_get to
> avoid having to call of_node_put on failure.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/ibm/emac/core.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index faa483790b29..5265616400c2 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -3253,21 +3253,17 @@ static void __init emac_make_bootlist(void)
>  	int cell_indices[EMAC_BOOT_LIST_SIZE];
>  
>  	/* Collect EMACs */
> -	while((np = of_find_all_nodes(np)) != NULL) {
> +	while((np = of_find_matching_node(np, emac_match))) {
>  		u32 idx;
>  
> -		if (of_match_node(emac_match, np) == NULL)
> -			continue;
>  		if (of_property_read_bool(np, "unused"))
>  			continue;
>  		if (of_property_read_u32(np, "cell-index", &idx))
>  			continue;
>  		cell_indices[i] = idx;
> -		emac_boot_list[i++] = of_node_get(np);
> -		if (i >= EMAC_BOOT_LIST_SIZE) {
> -			of_node_put(np);
> +		if (i >= EMAC_BOOT_LIST_SIZE)
>  			break;
> -		}
> +		emac_boot_list[i++] = of_node_get(np);

Reading the Kernel doc for of_find_matching_node() it seems
that of_node_put() needs to called each time it (and thus
of_find_matching_node() returns a np. But that doesn't seem
to be the case here. Am I mistaken?


>  	}
>  	max = i;
>  
> -- 
> 2.47.0
> 

Re: [PATCHv6 net-next 7/7] net: ibm: emac: use of_find_matching_node

[Rosen Penev]

On Sat, Oct 12, 2024 at 6:21 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Oct 11, 2024 at 12:56:22PM -0700, Rosen Penev wrote:
> > Cleaner than using of_find_all_nodes and then of_match_node.
> >
> > Also modified EMAC_BOOT_LIST_SIZE check to run before of_node_get to
> > avoid having to call of_node_put on failure.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/ibm/emac/core.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index faa483790b29..5265616400c2 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -3253,21 +3253,17 @@ static void __init emac_make_bootlist(void)
> >       int cell_indices[EMAC_BOOT_LIST_SIZE];
> >
> >       /* Collect EMACs */
> > -     while((np = of_find_all_nodes(np)) != NULL) {
> > +     while((np = of_find_matching_node(np, emac_match))) {
> >               u32 idx;
> >
> > -             if (of_match_node(emac_match, np) == NULL)
> > -                     continue;
> >               if (of_property_read_bool(np, "unused"))
> >                       continue;
> >               if (of_property_read_u32(np, "cell-index", &idx))
> >                       continue;
> >               cell_indices[i] = idx;
> > -             emac_boot_list[i++] = of_node_get(np);
> > -             if (i >= EMAC_BOOT_LIST_SIZE) {
> > -                     of_node_put(np);
> > +             if (i >= EMAC_BOOT_LIST_SIZE)
> >                       break;
> > -             }
> > +             emac_boot_list[i++] = of_node_get(np);
>
> Reading the Kernel doc for of_find_matching_node() it seems
> that of_node_put() needs to called each time it (and thus
> of_find_matching_node() returns a np. But that doesn't seem
> to be the case here. Am I mistaken?
Bad change. Will remove.
>
>
> >       }
> >       max = i;
> >
> > --
> > 2.47.0
> >