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

[PATCH 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>
---
Javier Carrasco (2):
      net: mdio: switch to scoped device_for_each_child_node()
      net: hns: 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: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20240930-net-device_for_each_child_node_scoped-ebe62f742847

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

[PATCH 1/2] net: mdio: 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

Re: [PATCH 1/2] net: mdio: switch to scoped device_for_each_child_node()

[Andrew Lunn]

On Mon, Sep 30, 2024 at 10:03:29PM +0200, Javier Carrasco wrote:
> 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().

Hi Javier

I know you are going across the whole tree, multiple sub systems, and
each has its own rules. I think naming patches is however pretty
uniform across the tree. Do what other patches did:

d84fe6dc7377 net: mdio: thunder: Add missing fwnode_handle_put()
a93a0a15876d net: mdio: thunder: Fix a double free issue in the .remove function

netdev has some additional documentation you should read:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

The change itself looks O.K, its just the processes which need work.

	Andrew

Re: [PATCH 1/2] net: mdio: switch to scoped device_for_each_child_node()

[Javier Carrasco]

On 30/09/2024 22:10, Andrew Lunn wrote:
> On Mon, Sep 30, 2024 at 10:03:29PM +0200, Javier Carrasco wrote:
>> 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().
> 
> Hi Javier
> 
> I know you are going across the whole tree, multiple sub systems, and
> each has its own rules. I think naming patches is however pretty
> uniform across the tree. Do what other patches did:
> 
> d84fe6dc7377 net: mdio: thunder: Add missing fwnode_handle_put()
> a93a0a15876d net: mdio: thunder: Fix a double free issue in the .remove function
> 
> netdev has some additional documentation you should read:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> The change itself looks O.K, its just the processes which need work.
> 
> 	Andrew


Hi Andrew,

thanks for your feedback. I used the same name I found in the first two
patches for that file, and I missed the 'thunder:' because probably
those patches applied to more files. And the same applies for
hns_dsaf_mac.c. I will fix that for v2 alongside anything I might find
in the netdev documentation that might be missing too.

Best regards,
Javier Carrasco

[PATCH 2/2] net: hns: 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