[PATCH net-next v2 00/13] net: Simplified with scoped function

[PATCH net-next v2 00/13] net: Simplified with scoped function

[Jinjie Ruan]

Simplify with scoped for each OF child loop and __free(), as well as
dev_err_probe().

Changes in v2:
- Subject prefix: next -> net-next.
- Split __free() from scoped for each OF child loop clean.
- Fix use of_node_put() instead of __free() for the 5th patch.

Jinjie Ruan (13):
  net: stmmac: dwmac-sun8i: Use for_each_child_of_node_scoped()
  net: stmmac: dwmac-sun8i: Use __free() to simplify code
  net: dsa: realtek: Use for_each_child_of_node_scoped()
  net: dsa: realtek: Use __free() to simplify code
  net: phy: Fix missing of_node_put() for leds
  net: phy: Use for_each_available_child_of_node_scoped()
  net: mdio: mux-mmioreg: Simplified with scoped function
  net: mdio: mux-mmioreg: Simplified with dev_err_probe()
  net: mv643xx_eth: Simplify with scoped for each OF child loop
  net: dsa: microchip: Use scoped function to simplfy code
  net: dsa: microchip: Use __free() to simplfy code
  net: bcmasp: Simplify with scoped for each OF child loop
  net: bcmasp: Simplify with __free()

 drivers/net/dsa/microchip/ksz_common.c        | 12 ++---
 drivers/net/dsa/realtek/rtl8366rb.c           | 11 ++--
 drivers/net/ethernet/broadcom/asp2/bcmasp.c   | 18 +++----
 drivers/net/ethernet/marvell/mv643xx_eth.c    |  5 +-
 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 20 +++-----
 drivers/net/mdio/mdio-mux-mmioreg.c           | 51 ++++++++-----------
 drivers/net/phy/phy_device.c                  |  7 +--
 7 files changed, 48 insertions(+), 76 deletions(-)

-- 
2.34.1

[PATCH net-next v2 01/13] net: stmmac: dwmac-sun8i: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using
for_each_child_of_node_scoped(), which can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index cc93f73a380e..8c5b4e0c0976 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -774,7 +774,7 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
 static int get_ephy_nodes(struct stmmac_priv *priv)
 {
 	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
-	struct device_node *mdio_mux, *iphynode;
+	struct device_node *mdio_mux;
 	struct device_node *mdio_internal;
 	int ret;
 
@@ -793,7 +793,7 @@ static int get_ephy_nodes(struct stmmac_priv *priv)
 	}
 
 	/* Seek for internal PHY */
-	for_each_child_of_node(mdio_internal, iphynode) {
+	for_each_child_of_node_scoped(mdio_internal, iphynode) {
 		gmac->ephy_clk = of_clk_get(iphynode, 0);
 		if (IS_ERR(gmac->ephy_clk))
 			continue;
@@ -801,14 +801,12 @@ static int get_ephy_nodes(struct stmmac_priv *priv)
 		if (IS_ERR(gmac->rst_ephy)) {
 			ret = PTR_ERR(gmac->rst_ephy);
 			if (ret == -EPROBE_DEFER) {
-				of_node_put(iphynode);
 				of_node_put(mdio_internal);
 				return ret;
 			}
 			continue;
 		}
 		dev_info(priv->device, "Found internal PHY node\n");
-		of_node_put(iphynode);
 		of_node_put(mdio_internal);
 		return 0;
 	}
-- 
2.34.1

Re: [PATCH net-next v2 01/13] net: stmmac: dwmac-sun8i: Use for_each_child_of_node_scoped()

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:31 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoid need to manually handle of_node_put() by using
> for_each_child_of_node_scoped(), which can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH net-next v2 01/13] net: stmmac: dwmac-sun8i: Use for_each_child_of_node_scoped()

[Andrew Lunn]

On Wed, Aug 28, 2024 at 11:23:31AM +0800, Jinjie Ruan wrote:
> Avoid need to manually handle of_node_put() by using
> for_each_child_of_node_scoped(), which can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index cc93f73a380e..8c5b4e0c0976 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -774,7 +774,7 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
>  static int get_ephy_nodes(struct stmmac_priv *priv)
>  {
>  	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> -	struct device_node *mdio_mux, *iphynode;
> +	struct device_node *mdio_mux;
>  	struct device_node *mdio_internal;
>  	int ret;

Networking uses reverse Christmas tree. Variables are sorted, longest
first, shortest last. So you need to move mdio_mux after
mdio_internal.

The rest looks O.K.


    Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 01/13] net: stmmac: dwmac-sun8i: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

On 2024/8/28 22:17, Andrew Lunn wrote:
> On Wed, Aug 28, 2024 at 11:23:31AM +0800, Jinjie Ruan wrote:
>> Avoid need to manually handle of_node_put() by using
>> for_each_child_of_node_scoped(), which can simplfy code.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>> index cc93f73a380e..8c5b4e0c0976 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>> @@ -774,7 +774,7 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
>>  static int get_ephy_nodes(struct stmmac_priv *priv)
>>  {
>>  	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
>> -	struct device_node *mdio_mux, *iphynode;
>> +	struct device_node *mdio_mux;
>>  	struct device_node *mdio_internal;
>>  	int ret;
> 
> Networking uses reverse Christmas tree. Variables are sorted, longest
> first, shortest last. So you need to move mdio_mux after
> mdio_internal.

Right, it will look more clear.

> 
> The rest looks O.K.
> 
> 
>     Andrew
> 
> ---
> pw-bot: cr

[PATCH net-next v2 02/13] net: stmmac: dwmac-sun8i: Use __free() to simplify code

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using __free(), which
can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c    | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 8c5b4e0c0976..415a0d23b3a5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -774,19 +774,17 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
 static int get_ephy_nodes(struct stmmac_priv *priv)
 {
 	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
-	struct device_node *mdio_mux;
-	struct device_node *mdio_internal;
 	int ret;
 
-	mdio_mux = of_get_child_by_name(priv->device->of_node, "mdio-mux");
+	struct device_node *mdio_mux __free(device_node) =
+		of_get_child_by_name(priv->device->of_node, "mdio-mux");
 	if (!mdio_mux) {
 		dev_err(priv->device, "Cannot get mdio-mux node\n");
 		return -ENODEV;
 	}
 
-	mdio_internal = of_get_compatible_child(mdio_mux,
-						"allwinner,sun8i-h3-mdio-internal");
-	of_node_put(mdio_mux);
+	struct device_node *mdio_internal __free(device_node) =
+		of_get_compatible_child(mdio_mux, "allwinner,sun8i-h3-mdio-internal");
 	if (!mdio_internal) {
 		dev_err(priv->device, "Cannot get internal_mdio node\n");
 		return -ENODEV;
@@ -800,18 +798,14 @@ static int get_ephy_nodes(struct stmmac_priv *priv)
 		gmac->rst_ephy = of_reset_control_get_exclusive(iphynode, NULL);
 		if (IS_ERR(gmac->rst_ephy)) {
 			ret = PTR_ERR(gmac->rst_ephy);
-			if (ret == -EPROBE_DEFER) {
-				of_node_put(mdio_internal);
+			if (ret == -EPROBE_DEFER)
 				return ret;
-			}
 			continue;
 		}
 		dev_info(priv->device, "Found internal PHY node\n");
-		of_node_put(mdio_internal);
 		return 0;
 	}
 
-	of_node_put(mdio_internal);
 	return -ENODEV;
 }
 
-- 
2.34.1

Re: [PATCH net-next v2 02/13] net: stmmac: dwmac-sun8i: Use __free() to simplify code

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:32 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoid need to manually handle of_node_put() by using __free(), which
> can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 03/13] net: dsa: realtek: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using
for_each_child_of_node_scoped(), which can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/dsa/realtek/rtl8366rb.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 9e821b42e5f3..7001b8b1c028 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1009,7 +1009,7 @@ static int rtl8366rb_setup_all_leds_off(struct realtek_priv *priv)
 
 static int rtl8366rb_setup_leds(struct realtek_priv *priv)
 {
-	struct device_node *leds_np, *led_np;
+	struct device_node *leds_np;
 	struct dsa_switch *ds = &priv->ds;
 	struct dsa_port *dp;
 	int ret = 0;
@@ -1025,13 +1025,11 @@ static int rtl8366rb_setup_leds(struct realtek_priv *priv)
 			continue;
 		}
 
-		for_each_child_of_node(leds_np, led_np) {
+		for_each_child_of_node_scoped(leds_np, led_np) {
 			ret = rtl8366rb_setup_led(priv, dp,
 						  of_fwnode_handle(led_np));
-			if (ret) {
-				of_node_put(led_np);
+			if (ret)
 				break;
-			}
 		}
 
 		of_node_put(leds_np);
-- 
2.34.1

Re: [PATCH net-next v2 03/13] net: dsa: realtek: Use for_each_child_of_node_scoped()

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:33 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoid need to manually handle of_node_put() by using
> for_each_child_of_node_scoped(), which can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 04/13] net: dsa: realtek: Use __free() to simplify code

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using __free(), which
can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2
- Split into 2 patches.
---
 drivers/net/dsa/realtek/rtl8366rb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 7001b8b1c028..0acdcdd93ea2 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1009,7 +1009,6 @@ static int rtl8366rb_setup_all_leds_off(struct realtek_priv *priv)
 
 static int rtl8366rb_setup_leds(struct realtek_priv *priv)
 {
-	struct device_node *leds_np;
 	struct dsa_switch *ds = &priv->ds;
 	struct dsa_port *dp;
 	int ret = 0;
@@ -1018,7 +1017,8 @@ static int rtl8366rb_setup_leds(struct realtek_priv *priv)
 		if (!dp->dn)
 			continue;
 
-		leds_np = of_get_child_by_name(dp->dn, "leds");
+		struct device_node *leds_np __free(device_node) =
+			of_get_child_by_name(dp->dn, "leds");
 		if (!leds_np) {
 			dev_dbg(priv->dev, "No leds defined for port %d",
 				dp->index);
@@ -1032,7 +1032,6 @@ static int rtl8366rb_setup_leds(struct realtek_priv *priv)
 				break;
 		}
 
-		of_node_put(leds_np);
 		if (ret)
 			return ret;
 	}
-- 
2.34.1

Re: [PATCH net-next v2 04/13] net: dsa: realtek: Use __free() to simplify code

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:34 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoid need to manually handle of_node_put() by using __free(), which
> can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
One suggestion inline.

> ---
> v2
> - Split into 2 patches.
> ---
>  drivers/net/dsa/realtek/rtl8366rb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
> index 7001b8b1c028..0acdcdd93ea2 100644
> --- a/drivers/net/dsa/realtek/rtl8366rb.c
> +++ b/drivers/net/dsa/realtek/rtl8366rb.c
> @@ -1009,7 +1009,6 @@ static int rtl8366rb_setup_all_leds_off(struct realtek_priv *priv)
>  
>  static int rtl8366rb_setup_leds(struct realtek_priv *priv)
>  {
> -	struct device_node *leds_np;
>  	struct dsa_switch *ds = &priv->ds;
>  	struct dsa_port *dp;
>  	int ret = 0;
> @@ -1018,7 +1017,8 @@ static int rtl8366rb_setup_leds(struct realtek_priv *priv)
>  		if (!dp->dn)
>  			continue;
>  
> -		leds_np = of_get_child_by_name(dp->dn, "leds");
> +		struct device_node *leds_np __free(device_node) =
> +			of_get_child_by_name(dp->dn, "leds");
>  		if (!leds_np) {
>  			dev_dbg(priv->dev, "No leds defined for port %d",
>  				dp->index);
> @@ -1032,7 +1032,6 @@ static int rtl8366rb_setup_leds(struct realtek_priv *priv)
>  				break;
>  		}
>  
> -		of_node_put(leds_np);
>  		if (ret)
>  			return ret;

Move this return up to the only place it can come from which is
inside the loop where the break is.

You can then avoid initializing ret and indeed could bring it's
scope into the loop

		for_each_child_of_node(leds_np, led_np) {
			int ret = rtl8366rb_setup_led(priv, dp,
						      of_fwnode_handle(led_np));
			if (ret)
				return ret;
		}	

>  	}

[PATCH net-next v2 05/13] net: phy: Fix missing of_node_put() for leds

[Jinjie Ruan]

The call of of_get_child_by_name() will cause refcount incremented
for leds, if it succeeds, it should call of_node_put() to decrease
it, fix it.

Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
- Use of_node_put() rather than __free() to fix it.
---
 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8f5314c1fecc..243dae686992 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3424,11 +3424,13 @@ static int of_phy_leds(struct phy_device *phydev)
 		err = of_phy_led(phydev, led);
 		if (err) {
 			of_node_put(led);
+			of_node_put(leds);
 			phy_leds_unregister(phydev);
 			return err;
 		}
 	}
 
+	of_node_put(leds);
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH net-next v2 05/13] net: phy: Fix missing of_node_put() for leds

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:35 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> The call of of_get_child_by_name() will cause refcount incremented
> for leds, if it succeeds, it should call of_node_put() to decrease
> it, fix it.
> 
> Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH net-next v2 05/13] net: phy: Fix missing of_node_put() for leds

[Willem de Bruijn]

Jinjie Ruan wrote:
> The call of of_get_child_by_name() will cause refcount incremented
> for leds, if it succeeds, it should call of_node_put() to decrease
> it, fix it.
> 
> Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

Fixes should go to net. Should not be part of this series?

Re: [PATCH net-next v2 05/13] net: phy: Fix missing of_node_put() for leds

[Jinjie Ruan]

On 2024/8/28 23:45, Willem de Bruijn wrote:
> Jinjie Ruan wrote:
>> The call of of_get_child_by_name() will cause refcount incremented
>> for leds, if it succeeds, it should call of_node_put() to decrease
>> it, fix it.
>>
>> Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> Fixes should go to net. Should not be part of this series?

Thank you! I will separate it out.

Re: [PATCH net-next v2 05/13] net: phy: Fix missing of_node_put() for leds

[Jinjie Ruan]

On 2024/8/28 23:45, Willem de Bruijn wrote:
> Jinjie Ruan wrote:
>> The call of of_get_child_by_name() will cause refcount incremented
>> for leds, if it succeeds, it should call of_node_put() to decrease
>> it, fix it.
>>
>> Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> Fixes should go to net. Should not be part of this series?

However, they have context dependency. If one is merged, the other needs
to be rebased.

[PATCH net-next v2 06/13] net: phy: Use for_each_available_child_of_node_scoped()

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using
for_each_available_child_of_node_scoped(), which can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
- Split into 2 patches.
---
 drivers/net/phy/phy_device.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 243dae686992..560e338b307a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3407,7 +3407,7 @@ static int of_phy_led(struct phy_device *phydev,
 static int of_phy_leds(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
-	struct device_node *leds, *led;
+	struct device_node *leds;
 	int err;
 
 	if (!IS_ENABLED(CONFIG_OF_MDIO))
@@ -3420,10 +3420,9 @@ static int of_phy_leds(struct phy_device *phydev)
 	if (!leds)
 		return 0;
 
-	for_each_available_child_of_node(leds, led) {
+	for_each_available_child_of_node_scoped(leds, led) {
 		err = of_phy_led(phydev, led);
 		if (err) {
-			of_node_put(led);
 			of_node_put(leds);
 			phy_leds_unregister(phydev);
 			return err;
-- 
2.34.1

Re: [PATCH net-next v2 06/13] net: phy: Use for_each_available_child_of_node_scoped()

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:36 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoid need to manually handle of_node_put() by using
> for_each_available_child_of_node_scoped(), which can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Ah. I see Andrew mentioned he didn't like the __free change
hence you've only done this one. Fair enough I guess.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> - Split into 2 patches.
> ---
>  drivers/net/phy/phy_device.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 243dae686992..560e338b307a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3407,7 +3407,7 @@ static int of_phy_led(struct phy_device *phydev,
>  static int of_phy_leds(struct phy_device *phydev)
>  {
>  	struct device_node *node = phydev->mdio.dev.of_node;
> -	struct device_node *leds, *led;
> +	struct device_node *leds;
>  	int err;
>  
>  	if (!IS_ENABLED(CONFIG_OF_MDIO))
> @@ -3420,10 +3420,9 @@ static int of_phy_leds(struct phy_device *phydev)
>  	if (!leds)
>  		return 0;
>  
> -	for_each_available_child_of_node(leds, led) {
> +	for_each_available_child_of_node_scoped(leds, led) {
>  		err = of_phy_led(phydev, led);
>  		if (err) {
> -			of_node_put(led);
>  			of_node_put(leds);
>  			phy_leds_unregister(phydev);
>  			return err;

[PATCH net-next v2 07/13] net: mdio: mux-mmioreg: Simplified with scoped function

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop by using for_each_available_child_of_node_scoped().

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/mdio/mdio-mux-mmioreg.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mdio/mdio-mux-mmioreg.c b/drivers/net/mdio/mdio-mux-mmioreg.c
index de08419d0c98..4d87e61fec7b 100644
--- a/drivers/net/mdio/mdio-mux-mmioreg.c
+++ b/drivers/net/mdio/mdio-mux-mmioreg.c
@@ -96,7 +96,7 @@ static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
 
 static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
 {
-	struct device_node *np2, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	struct mdio_mux_mmioreg_state *s;
 	struct resource res;
 	const __be32 *iprop;
@@ -139,20 +139,18 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
 	 * Verify that the 'reg' property of each child MDIO bus does not
 	 * set any bits outside of the 'mask'.
 	 */
-	for_each_available_child_of_node(np, np2) {
+	for_each_available_child_of_node_scoped(np, np2) {
 		u64 reg;
 
 		if (of_property_read_reg(np2, 0, &reg, NULL)) {
 			dev_err(&pdev->dev, "mdio-mux child node %pOF is "
 				"missing a 'reg' property\n", np2);
-			of_node_put(np2);
 			return -ENODEV;
 		}
 		if ((u32)reg & ~s->mask) {
 			dev_err(&pdev->dev, "mdio-mux child node %pOF has "
 				"a 'reg' value with unmasked bits\n",
 				np2);
-			of_node_put(np2);
 			return -ENODEV;
 		}
 	}
-- 
2.34.1

Re: [PATCH net-next v2 07/13] net: mdio: mux-mmioreg: Simplified with scoped function

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:37 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoids the need for manual cleanup of_node_put() in early exits
> from the loop by using for_each_available_child_of_node_scoped().
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Looks fine, though maybe driver would also benefit from dev_err_probe()
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 08/13] net: mdio: mux-mmioreg: Simplified with dev_err_probe()

[Jinjie Ruan]

Use the dev_err_probe() helper to simplify code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/mdio/mdio-mux-mmioreg.c | 45 ++++++++++++-----------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/drivers/net/mdio/mdio-mux-mmioreg.c b/drivers/net/mdio/mdio-mux-mmioreg.c
index 4d87e61fec7b..08c484ccdcbe 100644
--- a/drivers/net/mdio/mdio-mux-mmioreg.c
+++ b/drivers/net/mdio/mdio-mux-mmioreg.c
@@ -109,30 +109,26 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ret = of_address_to_resource(np, 0, &res);
-	if (ret) {
-		dev_err(&pdev->dev, "could not obtain memory map for node %pOF\n",
-			np);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "could not obtain memory map for node %pOF\n", np);
 	s->phys = res.start;
 
 	s->iosize = resource_size(&res);
 	if (s->iosize != sizeof(uint8_t) &&
 	    s->iosize != sizeof(uint16_t) &&
 	    s->iosize != sizeof(uint32_t)) {
-		dev_err(&pdev->dev, "only 8/16/32-bit registers are supported\n");
-		return -EINVAL;
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "only 8/16/32-bit registers are supported\n");
 	}
 
 	iprop = of_get_property(np, "mux-mask", &len);
-	if (!iprop || len != sizeof(uint32_t)) {
-		dev_err(&pdev->dev, "missing or invalid mux-mask property\n");
-		return -ENODEV;
-	}
-	if (be32_to_cpup(iprop) >= BIT(s->iosize * 8)) {
-		dev_err(&pdev->dev, "only 8/16/32-bit registers are supported\n");
-		return -EINVAL;
-	}
+	if (!iprop || len != sizeof(uint32_t))
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "missing or invalid mux-mask property\n");
+	if (be32_to_cpup(iprop) >= BIT(s->iosize * 8))
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "only 8/16/32-bit registers are supported\n");
 	s->mask = be32_to_cpup(iprop);
 
 	/*
@@ -142,17 +138,14 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
 	for_each_available_child_of_node_scoped(np, np2) {
 		u64 reg;
 
-		if (of_property_read_reg(np2, 0, &reg, NULL)) {
-			dev_err(&pdev->dev, "mdio-mux child node %pOF is "
-				"missing a 'reg' property\n", np2);
-			return -ENODEV;
-		}
-		if ((u32)reg & ~s->mask) {
-			dev_err(&pdev->dev, "mdio-mux child node %pOF has "
-				"a 'reg' value with unmasked bits\n",
-				np2);
-			return -ENODEV;
-		}
+		if (of_property_read_reg(np2, 0, &reg, NULL))
+			return dev_err_probe(&pdev->dev, -ENODEV,
+					     "mdio-mux child node %pOF is missing a 'reg' property\n",
+					     np2);
+		if ((u32)reg & ~s->mask)
+			return dev_err_probe(&pdev->dev, -ENODEV,
+					     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
+					     np2);
 	}
 
 	ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
-- 
2.34.1

Re: [PATCH net-next v2 08/13] net: mdio: mux-mmioreg: Simplified with dev_err_probe()

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:38 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Use the dev_err_probe() helper to simplify code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Ah. I should have read next patch. Sorry!

Might be worth breaking from rule of aligning parameters
after brackets to keep the longest line lengths down.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> v2:
> - Split into 2 patches.
> ---
>  drivers/net/mdio/mdio-mux-mmioreg.c | 45 ++++++++++++-----------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-mux-mmioreg.c b/drivers/net/mdio/mdio-mux-mmioreg.c
> index 4d87e61fec7b..08c484ccdcbe 100644
> --- a/drivers/net/mdio/mdio-mux-mmioreg.c
> +++ b/drivers/net/mdio/mdio-mux-mmioreg.c
> @@ -109,30 +109,26 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	ret = of_address_to_resource(np, 0, &res);
> -	if (ret) {
> -		dev_err(&pdev->dev, "could not obtain memory map for node %pOF\n",
> -			np);
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "could not obtain memory map for node %pOF\n", np);
>  	s->phys = res.start;
>  
>  	s->iosize = resource_size(&res);
>  	if (s->iosize != sizeof(uint8_t) &&
>  	    s->iosize != sizeof(uint16_t) &&
>  	    s->iosize != sizeof(uint32_t)) {
> -		dev_err(&pdev->dev, "only 8/16/32-bit registers are supported\n");
> -		return -EINVAL;
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "only 8/16/32-bit registers are supported\n");
>  	}
>  
>  	iprop = of_get_property(np, "mux-mask", &len);
> -	if (!iprop || len != sizeof(uint32_t)) {
> -		dev_err(&pdev->dev, "missing or invalid mux-mask property\n");
> -		return -ENODEV;
> -	}
> -	if (be32_to_cpup(iprop) >= BIT(s->iosize * 8)) {
> -		dev_err(&pdev->dev, "only 8/16/32-bit registers are supported\n");
> -		return -EINVAL;
> -	}
> +	if (!iprop || len != sizeof(uint32_t))
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "missing or invalid mux-mask property\n");
> +	if (be32_to_cpup(iprop) >= BIT(s->iosize * 8))
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "only 8/16/32-bit registers are supported\n");
>  	s->mask = be32_to_cpup(iprop);
>  
>  	/*
> @@ -142,17 +138,14 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
>  	for_each_available_child_of_node_scoped(np, np2) {
>  		u64 reg;
>  
> -		if (of_property_read_reg(np2, 0, &reg, NULL)) {
> -			dev_err(&pdev->dev, "mdio-mux child node %pOF is "
> -				"missing a 'reg' property\n", np2);
> -			return -ENODEV;
> -		}
> -		if ((u32)reg & ~s->mask) {
> -			dev_err(&pdev->dev, "mdio-mux child node %pOF has "
> -				"a 'reg' value with unmasked bits\n",
> -				np2);
> -			return -ENODEV;
> -		}
> +		if (of_property_read_reg(np2, 0, &reg, NULL))
> +			return dev_err_probe(&pdev->dev, -ENODEV,
> +					     "mdio-mux child node %pOF is missing a 'reg' property\n",
> +					     np2);
> +		if ((u32)reg & ~s->mask)
> +			return dev_err_probe(&pdev->dev, -ENODEV,
> +					     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
I'd align these super long ones as. 
			     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
It is ugly but then so are > 100 char lines.
> +					     np2);
>  	}
>  
>  	ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,

Re: [PATCH net-next v2 08/13] net: mdio: mux-mmioreg: Simplified with dev_err_probe()

[Jinjie Ruan]

On 2024/8/28 20:48, Jonathan Cameron wrote:
> On Wed, 28 Aug 2024 11:23:38 +0800
> Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> 
>> Use the dev_err_probe() helper to simplify code.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Ah. I should have read next patch. Sorry!
> 
> Might be worth breaking from rule of aligning parameters
> after brackets to keep the longest line lengths down.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>> v2:
>> - Split into 2 patches.
>> ---
>>  drivers/net/mdio/mdio-mux-mmioreg.c | 45 ++++++++++++-----------------
>>  1 file changed, 19 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/mdio/mdio-mux-mmioreg.c b/drivers/net/mdio/mdio-mux-mmioreg.c
>> index 4d87e61fec7b..08c484ccdcbe 100644
>> --- a/drivers/net/mdio/mdio-mux-mmioreg.c
>> +++ b/drivers/net/mdio/mdio-mux-mmioreg.c
>> @@ -109,30 +109,26 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
>>  		return -ENOMEM;
>>  
>>  	ret = of_address_to_resource(np, 0, &res);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "could not obtain memory map for node %pOF\n",
>> -			np);
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret,
>> +				     "could not obtain memory map for node %pOF\n", np);
>>  	s->phys = res.start;
>>  
>>  	s->iosize = resource_size(&res);
>>  	if (s->iosize != sizeof(uint8_t) &&
>>  	    s->iosize != sizeof(uint16_t) &&
>>  	    s->iosize != sizeof(uint32_t)) {
>> -		dev_err(&pdev->dev, "only 8/16/32-bit registers are supported\n");
>> -		return -EINVAL;
>> +		return dev_err_probe(&pdev->dev, -EINVAL,
>> +				     "only 8/16/32-bit registers are supported\n");
>>  	}
>>  
>>  	iprop = of_get_property(np, "mux-mask", &len);
>> -	if (!iprop || len != sizeof(uint32_t)) {
>> -		dev_err(&pdev->dev, "missing or invalid mux-mask property\n");
>> -		return -ENODEV;
>> -	}
>> -	if (be32_to_cpup(iprop) >= BIT(s->iosize * 8)) {
>> -		dev_err(&pdev->dev, "only 8/16/32-bit registers are supported\n");
>> -		return -EINVAL;
>> -	}
>> +	if (!iprop || len != sizeof(uint32_t))
>> +		return dev_err_probe(&pdev->dev, -ENODEV,
>> +				     "missing or invalid mux-mask property\n");
>> +	if (be32_to_cpup(iprop) >= BIT(s->iosize * 8))
>> +		return dev_err_probe(&pdev->dev, -EINVAL,
>> +				     "only 8/16/32-bit registers are supported\n");
>>  	s->mask = be32_to_cpup(iprop);
>>  
>>  	/*
>> @@ -142,17 +138,14 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
>>  	for_each_available_child_of_node_scoped(np, np2) {
>>  		u64 reg;
>>  
>> -		if (of_property_read_reg(np2, 0, &reg, NULL)) {
>> -			dev_err(&pdev->dev, "mdio-mux child node %pOF is "
>> -				"missing a 'reg' property\n", np2);
>> -			return -ENODEV;
>> -		}
>> -		if ((u32)reg & ~s->mask) {
>> -			dev_err(&pdev->dev, "mdio-mux child node %pOF has "
>> -				"a 'reg' value with unmasked bits\n",
>> -				np2);
>> -			return -ENODEV;
>> -		}
>> +		if (of_property_read_reg(np2, 0, &reg, NULL))
>> +			return dev_err_probe(&pdev->dev, -ENODEV,
>> +					     "mdio-mux child node %pOF is missing a 'reg' property\n",
>> +					     np2);
>> +		if ((u32)reg & ~s->mask)
>> +			return dev_err_probe(&pdev->dev, -ENODEV,
>> +					     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
> I'd align these super long ones as. 
> 			     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
> It is ugly but then so are > 100 char lines.

It seems that this kind string > 100 char is fine for patch check script.

>> +					     np2);
>>  	}
>>  
>>  	ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> 

Re: [PATCH net-next v2 08/13] net: mdio: mux-mmioreg: Simplified with dev_err_probe()

[Andrew Lunn]

> >> +		if ((u32)reg & ~s->mask)
> >> +			return dev_err_probe(&pdev->dev, -ENODEV,
> >> +					     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
> > I'd align these super long ones as. 
> > 			     "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n",
> > It is ugly but then so are > 100 char lines.
> 
> It seems that this kind string > 100 char is fine for patch check script.

Strings like this can ignore the 80 char limit because developers are
going to grep for it when it shows up in their kernel log. If it gets
broken in odd places, grep will not find it.

I would also say the indentation is correct as is, level with &pdev.

	Andrew

[PATCH net-next v2 09/13] net: mv643xx_eth: Simplify with scoped for each OF child loop

[Jinjie Ruan]

Use scoped for_each_available_child_of_node_scoped() when iterating
over device nodes to make code a bit simpler.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index f35ae2c88091..9e80899546d9 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2802,7 +2802,7 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
 static int mv643xx_eth_shared_of_probe(struct platform_device *pdev)
 {
 	struct mv643xx_eth_shared_platform_data *pd;
-	struct device_node *pnp, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
 	/* bail out if not registered from DT */
@@ -2816,10 +2816,9 @@ static int mv643xx_eth_shared_of_probe(struct platform_device *pdev)
 
 	mv643xx_eth_property(np, "tx-checksum-limit", pd->tx_csum_limit);
 
-	for_each_available_child_of_node(np, pnp) {
+	for_each_available_child_of_node_scoped(np, pnp) {
 		ret = mv643xx_eth_shared_of_add_port(pdev, pnp);
 		if (ret) {
-			of_node_put(pnp);
 			mv643xx_eth_shared_of_remove();
 			return ret;
 		}
-- 
2.34.1

Re: [PATCH net-next v2 09/13] net: mv643xx_eth: Simplify with scoped for each OF child loop

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:39 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Use scoped for_each_available_child_of_node_scoped() when iterating
> over device nodes to make code a bit simpler.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 10/13] net: dsa: microchip: Use scoped function to simplfy code

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop by using for_each_available_child_of_node_scoped().

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/dsa/microchip/ksz_common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index cd3991792b69..86ed563938f6 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -4595,7 +4595,7 @@ static int ksz_parse_drive_strength(struct ksz_device *dev)
 int ksz_switch_register(struct ksz_device *dev)
 {
 	const struct ksz_chip_data *info;
-	struct device_node *port, *ports;
+	struct device_node *ports;
 	phy_interface_t interface;
 	unsigned int port_num;
 	int ret;
@@ -4681,12 +4681,11 @@ int ksz_switch_register(struct ksz_device *dev)
 		if (!ports)
 			ports = of_get_child_by_name(dev->dev->of_node, "ports");
 		if (ports) {
-			for_each_available_child_of_node(ports, port) {
+			for_each_available_child_of_node_scoped(ports, port) {
 				if (of_property_read_u32(port, "reg",
 							 &port_num))
 					continue;
 				if (!(dev->port_mask & BIT(port_num))) {
-					of_node_put(port);
 					of_node_put(ports);
 					return -EINVAL;
 				}
-- 
2.34.1

Re: [PATCH net-next v2 10/13] net: dsa: microchip: Use scoped function to simplfy code

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:40 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoids the need for manual cleanup of_node_put() in early exits
> from the loop by using for_each_available_child_of_node_scoped().
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 11/13] net: dsa: microchip: Use __free() to simplfy code

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() by using __free().

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/dsa/microchip/ksz_common.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 86ed563938f6..8058a0b7c161 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -4595,7 +4595,6 @@ static int ksz_parse_drive_strength(struct ksz_device *dev)
 int ksz_switch_register(struct ksz_device *dev)
 {
 	const struct ksz_chip_data *info;
-	struct device_node *ports;
 	phy_interface_t interface;
 	unsigned int port_num;
 	int ret;
@@ -4677,7 +4676,8 @@ int ksz_switch_register(struct ksz_device *dev)
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
 			dev->compat_interface = interface;
-		ports = of_get_child_by_name(dev->dev->of_node, "ethernet-ports");
+		struct device_node *ports __free(device_node) =
+			of_get_child_by_name(dev->dev->of_node, "ethernet-ports");
 		if (!ports)
 			ports = of_get_child_by_name(dev->dev->of_node, "ports");
 		if (ports) {
@@ -4685,16 +4685,13 @@ int ksz_switch_register(struct ksz_device *dev)
 				if (of_property_read_u32(port, "reg",
 							 &port_num))
 					continue;
-				if (!(dev->port_mask & BIT(port_num))) {
-					of_node_put(ports);
+				if (!(dev->port_mask & BIT(port_num)))
 					return -EINVAL;
-				}
 				of_get_phy_mode(port,
 						&dev->ports[port_num].interface);
 
 				ksz_parse_rgmii_delay(dev, port_num, port);
 			}
-			of_node_put(ports);
 		}
 		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
 							 "microchip,synclko-125");
-- 
2.34.1

Re: [PATCH net-next v2 11/13] net: dsa: microchip: Use __free() to simplfy code

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:41 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoids the need for manual cleanup of_node_put() by using __free().
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 12/13] net: bcmasp: Simplify with scoped for each OF child loop

[Jinjie Ruan]

Use scoped for_each_available_child_of_node_scoped() when
iterating over device nodes to make code a bit simpler.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index 20c6529ec135..70219094b7f6 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -1300,7 +1300,7 @@ static void bcmasp_remove_intfs(struct bcmasp_priv *priv)
 
 static int bcmasp_probe(struct platform_device *pdev)
 {
-	struct device_node *ports_node, *intf_node;
+	struct device_node *ports_node;
 	const struct bcmasp_plat_data *pdata;
 	struct device *dev = &pdev->dev;
 	struct bcmasp_priv *priv;
@@ -1374,12 +1374,11 @@ static int bcmasp_probe(struct platform_device *pdev)
 	}
 
 	i = 0;
-	for_each_available_child_of_node(ports_node, intf_node) {
+	for_each_available_child_of_node_scoped(ports_node, intf_node) {
 		intf = bcmasp_interface_create(priv, intf_node, i);
 		if (!intf) {
 			dev_err(dev, "Cannot create eth interface %d\n", i);
 			bcmasp_remove_intfs(priv);
-			of_node_put(intf_node);
 			ret = -ENOMEM;
 			goto of_put_exit;
 		}
-- 
2.34.1

Re: [PATCH net-next v2 12/13] net: bcmasp: Simplify with scoped for each OF child loop

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:42 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Use scoped for_each_available_child_of_node_scoped() when
> iterating over device nodes to make code a bit simpler.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH net-next v2 13/13] net: bcmasp: Simplify with __free()

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using __free(), which
can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index 70219094b7f6..73e767aada2f 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -1300,7 +1300,6 @@ static void bcmasp_remove_intfs(struct bcmasp_priv *priv)
 
 static int bcmasp_probe(struct platform_device *pdev)
 {
-	struct device_node *ports_node;
 	const struct bcmasp_plat_data *pdata;
 	struct device *dev = &pdev->dev;
 	struct bcmasp_priv *priv;
@@ -1367,7 +1366,8 @@ static int bcmasp_probe(struct platform_device *pdev)
 	bcmasp_core_init(priv);
 	bcmasp_core_init_filters(priv);
 
-	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
+	struct device_node *ports_node __free(device_node) =
+		of_find_node_by_name(dev->of_node, "ethernet-ports");
 	if (!ports_node) {
 		dev_warn(dev, "No ports found\n");
 		return -EINVAL;
@@ -1377,10 +1377,9 @@ static int bcmasp_probe(struct platform_device *pdev)
 	for_each_available_child_of_node_scoped(ports_node, intf_node) {
 		intf = bcmasp_interface_create(priv, intf_node, i);
 		if (!intf) {
-			dev_err(dev, "Cannot create eth interface %d\n", i);
 			bcmasp_remove_intfs(priv);
-			ret = -ENOMEM;
-			goto of_put_exit;
+			return dev_err_probe(dev, -ENOMEM,
+					     "Cannot create eth interface %d\n", i);
 		}
 		list_add_tail(&intf->list, &priv->intfs);
 		i++;
@@ -1406,16 +1405,14 @@ static int bcmasp_probe(struct platform_device *pdev)
 				   "failed to register net_device: %d\n", ret);
 			priv->destroy_wol(priv);
 			bcmasp_remove_intfs(priv);
-			goto of_put_exit;
+			return ret;
 		}
 		count++;
 	}
 
 	dev_info(dev, "Initialized %d port(s)\n", count);
 
-of_put_exit:
-	of_node_put(ports_node);
-	return ret;
+	return 0;
 }
 
 static void bcmasp_remove(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH net-next v2 13/13] net: bcmasp: Simplify with __free()

[Jonathan Cameron]

On Wed, 28 Aug 2024 11:23:43 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoid need to manually handle of_node_put() by using __free(), which
> can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Longer term I'd prefer to see a lot of this stuff replaced
with firmware independent calls from property.h but in meantime
these are nice cleanup.

Re: [PATCH net-next v2 00/13] net: Simplified with scoped function

[Andrew Lunn]

On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote:
> Simplify with scoped for each OF child loop and __free(), as well as
> dev_err_probe().
> 
> Changes in v2:
> - Subject prefix: next -> net-next.
> - Split __free() from scoped for each OF child loop clean.
> - Fix use of_node_put() instead of __free() for the 5th patch.

I personally think all these __free() are ugly and magical. Can it
somehow be made part of of_get_child_by_name()? Add an
of_get_child_by_name_func_ref() which holds a reference to the node
for the scope of the function?

for_each_available_child_of_node_scoped() is fine. Once you have fixed
all the reverse christmas tree, please submit them. But i would like
to see alternatives to __free(), once which are less ugly.

	Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 00/13] net: Simplified with scoped function

[Krzysztof Kozlowski]

On 28/08/2024 16:32, Andrew Lunn wrote:
> On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote:
>> Simplify with scoped for each OF child loop and __free(), as well as
>> dev_err_probe().
>>
>> Changes in v2:
>> - Subject prefix: next -> net-next.
>> - Split __free() from scoped for each OF child loop clean.
>> - Fix use of_node_put() instead of __free() for the 5th patch.
> 
> I personally think all these __free() are ugly and magical. Can it

It is code readability so quite subjective. Jakub also rejected one of
such __free() changes, so maybe all these cases here should be rejected?

> somehow be made part of of_get_child_by_name()? Add an
> of_get_child_by_name_func_ref() which holds a reference to the node
> for the scope of the function?

That's interesting, scoped-wrapper. I am afraid we would need quite a
lot of them, though, for every of_get_xxx call.

Best regards,
Krzysztof

Re: [PATCH net-next v2 00/13] net: Simplified with scoped function

[Andrew Lunn]

On Wed, Aug 28, 2024 at 04:45:32PM +0200, Krzysztof Kozlowski wrote:
> On 28/08/2024 16:32, Andrew Lunn wrote:
> > On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote:
> >> Simplify with scoped for each OF child loop and __free(), as well as
> >> dev_err_probe().
> >>
> >> Changes in v2:
> >> - Subject prefix: next -> net-next.
> >> - Split __free() from scoped for each OF child loop clean.
> >> - Fix use of_node_put() instead of __free() for the 5th patch.
> > 
> > I personally think all these __free() are ugly and magical. Can it
> 
> It is code readability so quite subjective.

Try.

But the __ is also a red flag. Anything starting with _ or __ in
general should not be used in common code. That prefix is supposed to
indicate it is internal plumbing which should be hidden away, out of
sight, not to be used directly. Yet here it is, being scattered
everywhere.

I also wounder if this is lipstick on a pig. I suspect the reference
counting on DT object is broken everywhere, because it is almost never
used. In general, DT blobs exist from boot to shutdown. They don't go
away, so these reference counts are never used. DT overlays do exist,
but account for what, 1% of DT objects? And how often does an overlay
actually get unloaded? Has anybody written a fuzzer to try unloading
parts of DT blobs? I suspect we would quickly drown in bug reports.

Adding missing of_node_put() seems to be high on the list of bot
driven patches, which cause a lot of maintainer effort for no real
gain. And those submitting the patches probably have little
understanding of what they are doing, other than making the bot happy.
Do we really want to be adding ugly code, probably with a few
additional bugs thrown in, just to make a bot happy, but probably no
real benefit?

	Andrew

Re: [PATCH net-next v2 00/13] net: Simplified with scoped function

[Jakub Kicinski]

On Wed, 28 Aug 2024 16:45:32 +0200 Krzysztof Kozlowski wrote:
> On 28/08/2024 16:32, Andrew Lunn wrote:
> > On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote:  
> >> Simplify with scoped for each OF child loop and __free(), as well as
> >> dev_err_probe().
> >>
> >> Changes in v2:
> >> - Subject prefix: next -> net-next.
> >> - Split __free() from scoped for each OF child loop clean.
> >> - Fix use of_node_put() instead of __free() for the 5th patch.  
> > 
> > I personally think all these __free() are ugly and magical. Can it  
> 
> It is code readability so quite subjective. Jakub also rejected one of
> such __free() changes, so maybe all these cases here should be rejected?

Andrew's comments on refcounting on DT objects notwithstanding --
I do like the _scoped() iterator quite a bit, FWIW. I think it's one 
of the better uses of the auto-cleanup and local variable declarations.

Direct uses of __free() are more questionable in my opinion.

Using advanced constructs to build clean subsystem APIs is great,
sprinkling unreadable constructs to save LoC is what C++ is for. 
(Lets see how many people this offends ;))

Re: [PATCH net-next v2 00/13] net: Simplified with scoped function

[Jinjie Ruan]

On 2024/8/28 22:32, Andrew Lunn wrote:
> On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote:
>> Simplify with scoped for each OF child loop and __free(), as well as
>> dev_err_probe().
>>
>> Changes in v2:
>> - Subject prefix: next -> net-next.
>> - Split __free() from scoped for each OF child loop clean.
>> - Fix use of_node_put() instead of __free() for the 5th patch.
> 
> I personally think all these __free() are ugly and magical. Can it
> somehow be made part of of_get_child_by_name()? Add an
> of_get_child_by_name_func_ref() which holds a reference to the node
> for the scope of the function?

Yes, that is a good idea, and the __free() doesn't look as readable as
or_each_*child_of_node_scoped().

> 
> for_each_available_child_of_node_scoped() is fine. Once you have fixed
> all the reverse christmas tree, please submit them. But i would like
> to see alternatives to __free(), once which are less ugly.

Thank you!

> 
> 	Andrew
> 
> ---
> pw-bot: cr
>