diff mbox series

[6/6] net: mvpp2: use device_for_each_child_node() to access device child nodes

Message ID 20240706-device_for_each_child_node-available-v1-6-8a3f7615e41c@gmail.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series use device_for_each_child_node() to access device child nodes | expand

Commit Message

Javier Carrasco July 6, 2024, 3:23 p.m. UTC
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 lopps,
and the socped 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.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Jonathan Cameron July 7, 2024, 5:01 p.m. UTC | #1
On Sat, 06 Jul 2024 17:23:38 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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.
> 
> The child nodes within mvpp2_probe are not accessed outside the lopps,
> and the socped 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.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Russell King (Oracle) July 29, 2024, 8:23 a.m. UTC | #2
On Sat, Jul 06, 2024 at 05:23:38PM +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.
> 
> The child nodes within mvpp2_probe are not accessed outside the lopps,

"lopps" ?

> and the socped version of the macro can be used to automatically

"socped" ?

> decrement the refcount on early exits.
> 
> Use `device_for_each_child_node()` and its scoped variant to indicate
> device's direct child nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 9adf4301c9b1..97f1faab6f28 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,10 +7651,8 @@ static int mvpp2_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_port_probe:
> -	fwnode_handle_put(port_fwnode);
> -
>  	i = 0;
> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>  		if (priv->port_list[i])
>  			mvpp2_port_remove(priv->port_list[i]);
>  		i++;
> @@ -7677,13 +7673,12 @@ 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;
>  
>  	mvpp2_dbgfs_cleanup(priv);
>  
> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> +	device_for_each_child_node(&pdev->dev, port_fwnode) {
>  		if (priv->port_list[i]) {
>  			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
>  			mvpp2_port_remove(priv->port_list[i]);

This loop is just silly. There is no need to iterate the child nodes.
port_fwnode is not used, and the loop boils down to:

	for (i = 0; i < priv->port_count; i++) {
		mutex_destroy(&priv->port_list[i]->gather_stats_lock);
		mvpp2_port_remove(priv->port_list[i]);
	}

Not only is walking the child nodes not necessary, but checking whether
the pointer is NULL is also unnecessary. mvpp2_port_probe() populates
the array using:

        priv->port_list[priv->port_count++] = port;

and "port" can not be NULL here, so we're guaranteed that all port_list
entries for 0..priv->port_count will be non-NULL, and the driver makes
this assumption in multiple places.

In fact, I'd say that using fwnode_for_each_available_child_node() or
device_for_each_child_node() is buggy here if the availability of the
children change - it could leave ports not cleaned up.
Javier Carrasco July 29, 2024, 9:23 a.m. UTC | #3
On 29/07/2024 10:23, Russell King (Oracle) wrote:
> On Sat, Jul 06, 2024 at 05:23:38PM +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.
>>
>> The child nodes within mvpp2_probe are not accessed outside the lopps,
> 
> "lopps" ?
> 
>> and the socped version of the macro can be used to automatically
> 
> "socped" ?
> 

I'll fix the typos for v3.

>> decrement the refcount on early exits.
>>
>> Use `device_for_each_child_node()` and its scoped variant to indicate
>> device's direct child nodes.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> index 9adf4301c9b1..97f1faab6f28 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,10 +7651,8 @@ static int mvpp2_probe(struct platform_device *pdev)
>>  	return 0;
>>  
>>  err_port_probe:
>> -	fwnode_handle_put(port_fwnode);
>> -
>>  	i = 0;
>> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
>> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>>  		if (priv->port_list[i])
>>  			mvpp2_port_remove(priv->port_list[i]);
>>  		i++;
>> @@ -7677,13 +7673,12 @@ 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;
>>  
>>  	mvpp2_dbgfs_cleanup(priv);
>>  
>> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
>> +	device_for_each_child_node(&pdev->dev, port_fwnode) {
>>  		if (priv->port_list[i]) {
>>  			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
>>  			mvpp2_port_remove(priv->port_list[i]);
> 
> This loop is just silly. There is no need to iterate the child nodes.
> port_fwnode is not used, and the loop boils down to:
> 
> 	for (i = 0; i < priv->port_count; i++) {
> 		mutex_destroy(&priv->port_list[i]->gather_stats_lock);
> 		mvpp2_port_remove(priv->port_list[i]);
> 	}
> 
> Not only is walking the child nodes not necessary, but checking whether
> the pointer is NULL is also unnecessary. mvpp2_port_probe() populates
> the array using:
> 
>         priv->port_list[priv->port_count++] = port;
> 
> and "port" can not be NULL here, so we're guaranteed that all port_list
> entries for 0..priv->port_count will be non-NULL, and the driver makes
> this assumption in multiple places.
> 
> In fact, I'd say that using fwnode_for_each_available_child_node() or
> device_for_each_child_node() is buggy here if the availability of the
> children change - it could leave ports not cleaned up.
> 

I will add your suggestions in a separate patch with the corresponding
Suggested-by: tag. In that case, and taking into account that the
pointer check is unnecessary, the loop after a goto err_port_probe will
turn into this:

err_port_probe:
	for (i = 0; i < priv->port_count; i++)
		mvpp2_port_remove(priv->port_list[i]);

and the loop in mvpp2_remove() will be exactly the one you suggested.

Apart from that, there is a suspicious check towards the end of the same
function:

 if (is_acpi_node(port_fwnode))
		return;

At the point it is called in the current implementation, port_fwnode
could have been cleaned. And after removing the loop, it is simply
uninitialized. Was that meant to be pdev->dev->fwnode?

Thanks and best regards,
Javier Carrasco
Russell King (Oracle) July 29, 2024, 1 p.m. UTC | #4
On Mon, Jul 29, 2024 at 11:23:47AM +0200, Javier Carrasco wrote:
> Apart from that, there is a suspicious check towards the end of the same
> function:
> 
>  if (is_acpi_node(port_fwnode))
> 		return;
> 
> At the point it is called in the current implementation, port_fwnode
> could have been cleaned. And after removing the loop, it is simply
> uninitialized. Was that meant to be pdev->dev->fwnode?

If you're referring to the one before the clk_disable_unprepare() calls,
it's only slightly suspicious:

These clocks are setup in a:

        if (dev_of_node(&pdev->dev)) {
		...
	}

block, so they're only setup if we have device tree. So, avoiding it
for ACPI is entirely reasonable. However, we also have software nodes
as well, so the test should be:

	if (!dev_of_node(&pdev->dev))
		return;

to match what the probe function is doing.

Thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 9adf4301c9b1..97f1faab6f28 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,10 +7651,8 @@  static int mvpp2_probe(struct platform_device *pdev)
 	return 0;
 
 err_port_probe:
-	fwnode_handle_put(port_fwnode);
-
 	i = 0;
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
 		if (priv->port_list[i])
 			mvpp2_port_remove(priv->port_list[i]);
 		i++;
@@ -7677,13 +7673,12 @@  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;
 
 	mvpp2_dbgfs_cleanup(priv);
 
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node(&pdev->dev, port_fwnode) {
 		if (priv->port_list[i]) {
 			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
 			mvpp2_port_remove(priv->port_list[i]);