[PATCH net-next v2 0/2] net: switch to scoped device_for_each_child_node()

[PATCH net-next v2 0/2] net: switch to scoped device_for_each_child_node()

[Javier Carrasco]

This series switches from the device_for_each_child_node() macro to its
scoped variant. This makes the code more robust if new early exits are
added to the loops, because there is no need for explicit calls to
fwnode_handle_put(), which also simplifies existing code.

The non-scoped macros to walk over nodes turn error-prone as soon as
the loop contains early exits (break, goto, return), and patches to
fix them show up regularly, sometimes due to new error paths in an
existing loop [1].

Note that the child node is now declared in the macro, and therefore the
explicit declaration is no longer required.

The general functionality should not be affected by this modification.
If functional changes are found, please report them back as errors.

Link:
https://lore.kernel.org/all/20240901160829.709296395@linuxfoundation.org/
[1]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- Rebase onto net-next.
- Fix commit messages (incomplete path, missing net-next prefix).
- Link to v1: https://lore.kernel.org/r/20240930-net-device_for_each_child_node_scoped-v1-0-bbdd7f9fd649@gmail.com

---
Javier Carrasco (2):
      net: mdio: thunder: switch to scoped device_for_each_child_node()
      net: hns: hisilicon: hns_dsaf_mac: switch to scoped device_for_each_child_node()

 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 10 +++-------
 drivers/net/mdio/mdio-thunder.c                   |  4 +---
 2 files changed, 4 insertions(+), 10 deletions(-)
---
base-commit: c824deb1a89755f70156b5cdaf569fca80698719
change-id: 20240930-net-device_for_each_child_node_scoped-ebe62f742847

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH net-next v2 1/2] net: mdio: thunder: switch to scoped device_for_each_child_node()

[Javier Carrasco]

There has already been an issue with the handling of early exits from
device_for_each_child() in this driver, and it was solved with commit
b1de5c78ebe9 ("net: mdio: thunder: Add missing fwnode_handle_put()") by
adding a call to fwnode_handle_put() right after the loop.

That solution is valid indeed, but if a new error path with a 'return'
is added to the loop, this solution will fail. A more secure approach
is using the scoped variant of the macro, which automatically
decrements the refcount of the child node when it goes out of scope,
removing the need for explicit calls to fwnode_handle_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/mdio/mdio-thunder.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
index 6067d96b2b7b..1e1aa72b1eff 100644
--- a/drivers/net/mdio/mdio-thunder.c
+++ b/drivers/net/mdio/mdio-thunder.c
@@ -23,7 +23,6 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 				     const struct pci_device_id *ent)
 {
 	struct device_node *node;
-	struct fwnode_handle *fwn;
 	struct thunder_mdiobus_nexus *nexus;
 	int err;
 	int i;
@@ -54,7 +53,7 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 	}
 
 	i = 0;
-	device_for_each_child_node(&pdev->dev, fwn) {
+	device_for_each_child_node_scoped(&pdev->dev, fwn) {
 		struct resource r;
 		struct mii_bus *mii_bus;
 		struct cavium_mdiobus *bus;
@@ -106,7 +105,6 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 		if (i >= ARRAY_SIZE(nexus->buses))
 			break;
 	}
-	fwnode_handle_put(fwn);
 	return 0;
 
 err_release_regions:

-- 
2.43.0

[PATCH net-next v2 2/2] net: hns: hisilicon: hns_dsaf_mac: switch to scoped device_for_each_child_node()

[Javier Carrasco]

Use device_for_each_child_node_scoped() to simplify the code by removing
the need for explicit calls to fwnode_handle_put() in every error path.
This approach also accounts for any error path that could be added.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index 58baac7103b3..5fa9b2eeb929 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -1090,28 +1090,24 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
 	u32 port_id;
 	int max_port_num = hns_mac_get_max_port_num(dsaf_dev);
 	struct hns_mac_cb *mac_cb;
-	struct fwnode_handle *child;
 
-	device_for_each_child_node(dsaf_dev->dev, child) {
+	device_for_each_child_node_scoped(dsaf_dev->dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &port_id);
 		if (ret) {
-			fwnode_handle_put(child);
 			dev_err(dsaf_dev->dev,
 				"get reg fail, ret=%d!\n", ret);
 			return ret;
 		}
 		if (port_id >= max_port_num) {
-			fwnode_handle_put(child);
 			dev_err(dsaf_dev->dev,
 				"reg(%u) out of range!\n", port_id);
 			return -EINVAL;
 		}
 		mac_cb = devm_kzalloc(dsaf_dev->dev, sizeof(*mac_cb),
 				      GFP_KERNEL);
-		if (!mac_cb) {
-			fwnode_handle_put(child);
+		if (!mac_cb)
 			return -ENOMEM;
-		}
+
 		mac_cb->fw_port = child;
 		mac_cb->mac_id = (u8)port_id;
 		dsaf_dev->mac_cb[port_id] = mac_cb;

-- 
2.43.0

Re: [PATCH net-next v2 0/2] net: switch to scoped device_for_each_child_node()

[Andrew Lunn]

On Mon, Sep 30, 2024 at 10:38:24PM +0200, Javier Carrasco wrote:
> This series switches from the device_for_each_child_node() macro to its
> scoped variant. This makes the code more robust if new early exits are
> added to the loops, because there is no need for explicit calls to
> fwnode_handle_put(), which also simplifies existing code.
> 
> The non-scoped macros to walk over nodes turn error-prone as soon as
> the loop contains early exits (break, goto, return), and patches to
> fix them show up regularly, sometimes due to new error paths in an
> existing loop [1].
> 
> Note that the child node is now declared in the macro, and therefore the
> explicit declaration is no longer required.
> 
> The general functionality should not be affected by this modification.
> If functional changes are found, please report them back as errors.
> 
> Link:
> https://lore.kernel.org/all/20240901160829.709296395@linuxfoundation.org/
> [1]
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> Changes in v2:
> - Rebase onto net-next.
> - Fix commit messages (incomplete path, missing net-next prefix).
> - Link to v1: https://lore.kernel.org/r/20240930-net-device_for_each_child_node_scoped-v1-0-bbdd7f9fd649@gmail.com

Much better.

Just watch out for the 24 hour rule between reposts. Reposting too
fast results in wasted time. Reviewers see you v1 and give comments on
it without knowing there is a v2 which might have the issues
fixed. And you might ignore those late comments on v1 ...

I will wait a day or two to review the actual patches, to give others
time to take a look.

  Andrew

Re: [PATCH net-next v2 0/2] net: switch to scoped device_for_each_child_node()

[Javier Carrasco]

On 30/09/2024 22:47, Andrew Lunn wrote:
> On Mon, Sep 30, 2024 at 10:38:24PM +0200, Javier Carrasco wrote:
>> This series switches from the device_for_each_child_node() macro to its
>> scoped variant. This makes the code more robust if new early exits are
>> added to the loops, because there is no need for explicit calls to
>> fwnode_handle_put(), which also simplifies existing code.
>>
>> The non-scoped macros to walk over nodes turn error-prone as soon as
>> the loop contains early exits (break, goto, return), and patches to
>> fix them show up regularly, sometimes due to new error paths in an
>> existing loop [1].
>>
>> Note that the child node is now declared in the macro, and therefore the
>> explicit declaration is no longer required.
>>
>> The general functionality should not be affected by this modification.
>> If functional changes are found, please report them back as errors.
>>
>> Link:
>> https://lore.kernel.org/all/20240901160829.709296395@linuxfoundation.org/
>> [1]
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>> Changes in v2:
>> - Rebase onto net-next.
>> - Fix commit messages (incomplete path, missing net-next prefix).
>> - Link to v1: https://lore.kernel.org/r/20240930-net-device_for_each_child_node_scoped-v1-0-bbdd7f9fd649@gmail.com
> 
> Much better.
> 
> Just watch out for the 24 hour rule between reposts. Reposting too
> fast results in wasted time. Reviewers see you v1 and give comments on
> it without knowing there is a v2 which might have the issues
> fixed. And you might ignore those late comments on v1 ...
> 
> I will wait a day or two to review the actual patches, to give others
> time to take a look.
> 
>   Andrew

Thanks again, the commit messages were so broken that I thought the
series would not be taken into account, especially after your reply. But
you are right, it could confuse reviewers and we are definitely not in a
hurry :)

Best regards,
Javier Carrasco

Re: [PATCH net-next v2 0/2] net: switch to scoped device_for_each_child_node()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 30 Sep 2024 22:38:24 +0200 you wrote:
> This series switches from the device_for_each_child_node() macro to its
> scoped variant. This makes the code more robust if new early exits are
> added to the loops, because there is no need for explicit calls to
> fwnode_handle_put(), which also simplifies existing code.
> 
> The non-scoped macros to walk over nodes turn error-prone as soon as
> the loop contains early exits (break, goto, return), and patches to
> fix them show up regularly, sometimes due to new error paths in an
> existing loop [1].
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/2] net: mdio: thunder: switch to scoped device_for_each_child_node()
    https://git.kernel.org/netdev/net-next/c/1d39d02a1535
  - [net-next,v2,2/2] net: hns: hisilicon: hns_dsaf_mac: switch to scoped device_for_each_child_node()
    https://git.kernel.org/netdev/net-next/c/e97dccd3e976

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html