[PATCH v3 0/4] use device_for_each_child_node() to access device child nodes

[PATCH v3 0/4] use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

This series aims to clarify the use cases of:

- device_for_each_child_node[_scoped]()
- fwnode_for_each_available_child_node[_scoped]()

to access firmware nodes.

There have been multiple discussions [1][2] about what the first macro
implies in the sense of availability, and a number of users have opted
for the second macro in cases where the first one should have been
preferred.

The second macro is intended to be used over child nodes of a firmware
node, not direct child nodes of the device node. Instead, those users
retrieve the fwnode member from the device struct just to have access to
a macro that explicitly indicates node availability.

That workaround is not necessary because `device_for_each_child_node()`
implies availability for the existing backends (ACPI, DT, swnode).

This series does not cover other points discussed in [2] like addressing
uses of `fwnode_for_each_child_node()` where `device_*` should have been
used, using the `_avaialble_` variant of the fwnode loop whenever
possible, or adding new `_scoped` macros. Such points will be covered by
subsequent series to keep focus on the "availability" issue.

The conversion has been validated with an LTC2992 hwmon sensor, which is
one of the affected drivers. The rest of the drivers could only be
compiled and checked with static-analysis tools.

Link: https://lore.kernel.org/all/20211205190101.26de4a57@jic23-huawei/ [1]
Link: https://lore.kernel.org/all/20240523-fwnode_for_each_available_child_node_scoped-v2-0-701f3a03f2fb@gmail.com/ [2]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v3:
- Rebase to next/20240805 and drop applied patches.
- mvpp2: fix typos in commit description.
- mvpp2: add patch to use port_count instead of node iteration.
- Link to v2: https://lore.kernel.org/r/20240721-device_for_each_child_node-available-v2-0-f33748fd8b2d@gmail.com

Changes in v2:
- [1/6] property.h: drop "if found" from the description of
  device_for_each_child_node()
- [3/6] bd2607mvv.c: fix child node usage.
- Link to v1: https://lore.kernel.org/r/20240706-device_for_each_child_node-available-v1-0-8a3f7615e41c@gmail.com

---
Javier Carrasco (4):
      device property: document device_for_each_child_node macro
      leds: pca995x: use device_for_each_child_node() to access device child nodes
      net: mvpp2: use port_count to remove ports
      net: mvpp2: use device_for_each_child_node() to access device child nodes

 drivers/leds/leds-pca995x.c                     | 15 ++++--------
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 31 +++++++------------------
 include/linux/property.h                        | 10 ++++++++
 3 files changed, 24 insertions(+), 32 deletions(-)
---
base-commit: d6dbc9f56c3a70e915625b6f1887882c23dc5c91
change-id: 20240701-device_for_each_child_node-available-1c1eca4b6495

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

[PATCH v3 1/4] device property: document device_for_each_child_node macro

[Javier Carrasco]

There have been some misconceptions about this macro, which iterates
over available child nodes from different backends.

As that is not obvious by its name, some users have opted for the
`fwnode_for_each_available_child_node()` macro instead.
That requires an unnecessary, explicit access to the fwnode member
of the device structure.

Passing the device to `device_for_each_child_node()` is shorter,
reflects more clearly the nature of the child nodes, and renders the
same result.

In general, `fwnode_for_each_available_child_node()` should be used
whenever the parent node of the children to iterate over is a firmware
node, and not the device itself.

Document the `device_for_each_child node(dev, child)` macro to clarify
its functionality.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 include/linux/property.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 61fc20e5f81f..da8f1208de38 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
 struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 						 struct fwnode_handle *child);
 
+/**
+ * device_for_each_child_node - iterate over available child nodes of a device
+ * @dev: Pointer to the struct device
+ * @child: Pointer to an available child node in each loop iteration
+ *
+ * Unavailable nodes are skipped i.e. this macro is implicitly _available_.
+ * The reference to the child node must be dropped on early exits.
+ * See fwnode_handle_put().
+ * For a scoped version of this macro, use device_for_each_child_node_scoped().
+ */
 #define device_for_each_child_node(dev, child)				\
 	for (child = device_get_next_child_node(dev, NULL); child;	\
 	     child = device_get_next_child_node(dev, child))

-- 
2.43.0

Re: [PATCH v3 1/4] device property: document device_for_each_child_node macro

[Sakari Ailus]

Hi Javier,

Thanks for the patch.

On Mon, Aug 05, 2024 at 04:49:44PM +0200, Javier Carrasco wrote:
> There have been some misconceptions about this macro, which iterates
> over available child nodes from different backends.
> 
> As that is not obvious by its name, some users have opted for the
> `fwnode_for_each_available_child_node()` macro instead.
> That requires an unnecessary, explicit access to the fwnode member
> of the device structure.
> 
> Passing the device to `device_for_each_child_node()` is shorter,
> reflects more clearly the nature of the child nodes, and renders the
> same result.
> 
> In general, `fwnode_for_each_available_child_node()` should be used
> whenever the parent node of the children to iterate over is a firmware
> node, and not the device itself.
> 
> Document the `device_for_each_child node(dev, child)` macro to clarify
> its functionality.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  include/linux/property.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 61fc20e5f81f..da8f1208de38 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>  						 struct fwnode_handle *child);
>  
> +/**
> + * device_for_each_child_node - iterate over available child nodes of a device
> + * @dev: Pointer to the struct device
> + * @child: Pointer to an available child node in each loop iteration
> + *
> + * Unavailable nodes are skipped i.e. this macro is implicitly _available_.

Is it?

I think this was brought up previously but I understand there's an
available check on OF but not on ACPI, so on OF the availability-agnostic
and availability-aware variants are the same.

Both in OF and ACPI a node that's been marked not available simply isn't
there so as far as I understand, the semantics are the same. There's a
difference though: in OF every node can be tested for availability whereas
on ACPI only device nodes can (there are data nodes, too, for which the
availability test is always false as it's pointless to test them in the
first place).

So overall I guess the code is mostly ok be but the caller does need to be
careful with the differences.

> + * The reference to the child node must be dropped on early exits.
> + * See fwnode_handle_put().
> + * For a scoped version of this macro, use device_for_each_child_node_scoped().
> + */
>  #define device_for_each_child_node(dev, child)				\
>  	for (child = device_get_next_child_node(dev, NULL); child;	\
>  	     child = device_get_next_child_node(dev, child))
> 

-- 
Kind regards,

Sakari Ailus

[PATCH v3 2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

Use `device_for_each_child_node()` to indicate device's direct child
nodes.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-pca995x.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
index 686b77772cce..83bc9669544c 100644
--- a/drivers/leds/leds-pca995x.c
+++ b/drivers/leds/leds-pca995x.c
@@ -120,7 +120,7 @@ static const struct regmap_config pca995x_regmap = {
 static int pca995x_probe(struct i2c_client *client)
 {
 	struct fwnode_handle *led_fwnodes[PCA995X_MAX_OUTPUTS] = { 0 };
-	struct fwnode_handle *np, *child;
+	struct fwnode_handle *child;
 	struct device *dev = &client->dev;
 	const struct pca995x_chipdef *chipdef;
 	struct pca995x_chip *chip;
@@ -129,8 +129,7 @@ static int pca995x_probe(struct i2c_client *client)
 
 	chipdef = device_get_match_data(&client->dev);
 
-	np = dev_fwnode(dev);
-	if (!np)
+	if (!dev_fwnode(dev))
 		return -ENODEV;
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
@@ -144,17 +143,13 @@ static int pca995x_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, chip);
 
-	fwnode_for_each_available_child_node(np, child) {
+	device_for_each_child_node(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &reg);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
-		if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg]) {
-			fwnode_handle_put(child);
+		if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg])
 			return -EINVAL;
-		}
 
 		led = &chip->leds[reg];
 		led_fwnodes[reg] = child;

-- 
2.43.0

Re: (subset) [PATCH v3 2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes

[Lee Jones]

On Mon, 05 Aug 2024 16:49:45 +0200, Javier Carrasco wrote:
> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> [...]

Applied, thanks!

[2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes
      commit: 6eefd65ba6ae29ab801f6461e59c10f93dd496f8

--
Lee Jones [李琼斯]

Re: (subset) [PATCH v3 2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes

[Lee Jones]

On Mon, 05 Aug 2024, Lee Jones wrote:

> On Mon, 05 Aug 2024 16:49:45 +0200, Javier Carrasco wrote:
> > The iterated nodes are direct children of the device node, and the
> > `device_for_each_child_node()` macro accounts for child node
> > availability.
> > 
> > `fwnode_for_each_available_child_node()` is meant to access the child
> > nodes of an fwnode, and therefore not direct child nodes of the device
> > node.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes
>       commit: 6eefd65ba6ae29ab801f6461e59c10f93dd496f8

I'm not sure what you rebased onto, but it wasn't LEDs or -next.

Anyway, I fixed-up the conflicts and pushed.

The patch should be in -next by tomorrow.

Please check it to ensure I didn't make any mistakes.

-- 
Lee Jones [李琼斯]

Re: (subset) [PATCH v3 2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

On 05/08/2024 18:01, Lee Jones wrote:
> On Mon, 05 Aug 2024, Lee Jones wrote:
> 
>> On Mon, 05 Aug 2024 16:49:45 +0200, Javier Carrasco wrote:
>>> The iterated nodes are direct children of the device node, and the
>>> `device_for_each_child_node()` macro accounts for child node
>>> availability.
>>>
>>> `fwnode_for_each_available_child_node()` is meant to access the child
>>> nodes of an fwnode, and therefore not direct child nodes of the device
>>> node.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [2/4] leds: pca995x: use device_for_each_child_node() to access device child nodes
>>       commit: 6eefd65ba6ae29ab801f6461e59c10f93dd496f8
> 
> I'm not sure what you rebased onto, but it wasn't LEDs or -next.
> 
> Anyway, I fixed-up the conflicts and pushed.
> 
> The patch should be in -next by tomorrow.
> 
> Please check it to ensure I didn't make any mistakes.
> 

Hi, I rebased onto next-20240805, and its commit ID matches the
base-commit provided in the cover letter (generated by b4). I wonder why
it did not work on your side, but thanks for fixing the conflicts and
applying (I checked it and it looks fine).

Best regards,
Javier Carrasco

[PATCH v3 3/4] net: mvpp2: use port_count to remove ports

[Javier Carrasco]

As discussed in [1], there is no need to iterate over child nodes to
remove the list of ports. Instead, a loop up to `port_count` ports can
be used, and is in fact more reliable in case the child node
availability changes.

The suggested approach removes the need for the `fwnode` and
`port_fwnode` variables in mvpp2_remove() as well.

Link: https://lore.kernel.org/all/ZqdRgDkK1PzoI2Pf@shell.armlinux.org.uk/ [1]
Suggested-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 0d62a33afa80..0b5b2425de12 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -7655,12 +7655,8 @@ static int mvpp2_probe(struct platform_device *pdev)
 err_port_probe:
 	fwnode_handle_put(port_fwnode);
 
-	i = 0;
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
-		if (priv->port_list[i])
-			mvpp2_port_remove(priv->port_list[i]);
-		i++;
-	}
+	for (i = 0; i < priv->port_count; i++)
+		mvpp2_port_remove(priv->port_list[i]);
 err_axi_clk:
 	clk_disable_unprepare(priv->axi_clk);
 err_mg_core_clk:
@@ -7677,18 +7673,13 @@ static int mvpp2_probe(struct platform_device *pdev)
 static void mvpp2_remove(struct platform_device *pdev)
 {
 	struct mvpp2 *priv = platform_get_drvdata(pdev);
-	struct fwnode_handle *fwnode = pdev->dev.fwnode;
-	int i = 0, poolnum = MVPP2_BM_POOLS_NUM;
-	struct fwnode_handle *port_fwnode;
+	int i, poolnum = MVPP2_BM_POOLS_NUM;
 
 	mvpp2_dbgfs_cleanup(priv);
 
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
-		if (priv->port_list[i]) {
-			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
-			mvpp2_port_remove(priv->port_list[i]);
-		}
-		i++;
+	for (i = 0; i < priv->port_count; i++) {
+		mutex_destroy(&priv->port_list[i]->gather_stats_lock);
+		mvpp2_port_remove(priv->port_list[i]);
 	}
 
 	destroy_workqueue(priv->stats_queue);
@@ -7711,7 +7702,7 @@ static void mvpp2_remove(struct platform_device *pdev)
 				  aggr_txq->descs_dma);
 	}
 
-	if (is_acpi_node(port_fwnode))
+	if (!dev_of_node(&pdev->dev))
 		return;
 
 	clk_disable_unprepare(priv->axi_clk);

-- 
2.43.0

[PATCH v3 4/4] net: mvpp2: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

The child nodes within mvpp2_probe are not accessed outside the loops,
and the scoped version of the macro can be used to automatically
decrement the refcount on early exits.

Use `device_for_each_child_node()` and its scoped variant to indicate
device's direct child nodes.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 0b5b2425de12..216cc7b860d6 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -7417,8 +7417,6 @@ static int mvpp2_get_sram(struct platform_device *pdev,
 
 static int mvpp2_probe(struct platform_device *pdev)
 {
-	struct fwnode_handle *fwnode = pdev->dev.fwnode;
-	struct fwnode_handle *port_fwnode;
 	struct mvpp2 *priv;
 	struct resource *res;
 	void __iomem *base;
@@ -7591,7 +7589,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 	}
 
 	/* Map DTS-active ports. Should be done before FIFO mvpp2_init */
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
 		if (!fwnode_property_read_u32(port_fwnode, "port-id", &i))
 			priv->port_map |= BIT(i);
 	}
@@ -7614,7 +7612,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 		goto err_axi_clk;
 
 	/* Initialize ports */
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
 		err = mvpp2_port_probe(pdev, port_fwnode, priv);
 		if (err < 0)
 			goto err_port_probe;
@@ -7653,8 +7651,6 @@ static int mvpp2_probe(struct platform_device *pdev)
 	return 0;
 
 err_port_probe:
-	fwnode_handle_put(port_fwnode);
-
 	for (i = 0; i < priv->port_count; i++)
 		mvpp2_port_remove(priv->port_list[i]);
 err_axi_clk:

-- 
2.43.0

Re: [PATCH v3 0/4] use device_for_each_child_node() to access device child nodes

[Jakub Kicinski]

On Mon, 05 Aug 2024 16:49:43 +0200 Javier Carrasco wrote:
>       net: mvpp2: use port_count to remove ports
>       net: mvpp2: use device_for_each_child_node() to access device child nodes

Please repost these two separately so we can take the whole series 
via networking. As is the series doesn't apply so it's too much
manual twiddling to fit into our tree.