[PATCH v3 linux-next] leds: powernv: replace of_node_put to __free

[PATCH v3 linux-next] leds: powernv: replace of_node_put to __free

[MarileneGarcia]

Use __free for device_node values, and thus drop calls to
of_node_put.

The variable attribute __free adds a scope based cleanup to
the device node. The goal is to reduce memory management issues
in the kernel code.

The of_node_put calls were removed, and the
for_each_available_child_of_node was replaced to the equivalent
for_each_available_child_of_node_scoped which use the __free.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: MarileneGarcia <marilene.agarcia@gmail.com>
---
Hello, 
Thank you for the feedback.

The line-break strategy was fixed, and now it is according to
the top one suggested.

The __free is a wrapper to __attribute__((__cleanup__())) so 
the variavel definition is needed. And according to Julia, it 
is preferred to combine the declaration and the allocation to 
ensure that there is no return that can occur after the declaration, 
but before the allocation (or more precisely the initialization).  
If there is no other option for the initialization of the variable, 
then it should be NULL.

 drivers/leds/leds-powernv.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
index 4f01acb75727..49ab8c9a3f29 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -246,29 +246,25 @@ static int powernv_led_classdev(struct platform_device *pdev,
 	const char *cur = NULL;
 	int rc = -1;
 	struct property *p;
-	struct device_node *np;
 	struct powernv_led_data *powernv_led;
 	struct device *dev = &pdev->dev;
 
-	for_each_available_child_of_node(led_node, np) {
+	for_each_available_child_of_node_scoped(led_node, np) {
 		p = of_find_property(np, "led-types", NULL);
 
 		while ((cur = of_prop_next_string(p, cur)) != NULL) {
 			powernv_led = devm_kzalloc(dev, sizeof(*powernv_led),
 						   GFP_KERNEL);
-			if (!powernv_led) {
-				of_node_put(np);
+			if (!powernv_led)
 				return -ENOMEM;
-			}
 
 			powernv_led->common = powernv_led_common;
 			powernv_led->loc_code = (char *)np->name;
 
 			rc = powernv_led_create(dev, powernv_led, cur);
-			if (rc) {
-				of_node_put(np);
+			if (rc)
 				return rc;
-			}
+
 		} /* while end */
 	}
 
@@ -278,12 +274,11 @@ static int powernv_led_classdev(struct platform_device *pdev,
 /* Platform driver probe */
 static int powernv_led_probe(struct platform_device *pdev)
 {
-	struct device_node *led_node;
 	struct powernv_led_common *powernv_led_common;
 	struct device *dev = &pdev->dev;
-	int rc;
+	struct device_node *led_node
+		__free(device_node) = of_find_node_by_path("/ibm,opal/leds");
 
-	led_node = of_find_node_by_path("/ibm,opal/leds");
 	if (!led_node) {
 		dev_err(dev, "%s: LED parent device node not found\n",
 			__func__);
@@ -292,20 +287,15 @@ static int powernv_led_probe(struct platform_device *pdev)
 
 	powernv_led_common = devm_kzalloc(dev, sizeof(*powernv_led_common),
 					  GFP_KERNEL);
-	if (!powernv_led_common) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!powernv_led_common)
+		return -ENOMEM;
 
 	mutex_init(&powernv_led_common->lock);
 	powernv_led_common->max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
 
 	platform_set_drvdata(pdev, powernv_led_common);
 
-	rc = powernv_led_classdev(pdev, led_node, powernv_led_common);
-out:
-	of_node_put(led_node);
-	return rc;
+	return powernv_led_classdev(pdev, led_node, powernv_led_common);
 }
 
 /* Platform driver remove */
-- 
2.34.1

Re: [PATCH v3 linux-next] leds: powernv: replace of_node_put to __free

[Marilene Andrade Garcia]

On 01/06/2024 00:17, MarileneGarcia wrote:
> Use __free for device_node values, and thus drop calls to
> of_node_put.
> 
> The variable attribute __free adds a scope based cleanup to
> the device node. The goal is to reduce memory management issues
> in the kernel code.
> 
> The of_node_put calls were removed, and the
> for_each_available_child_of_node was replaced to the equivalent
> for_each_available_child_of_node_scoped which use the __free.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: MarileneGarcia <marilene.agarcia@gmail.com>
> ---
> Hello,
> Thank you for the feedback.
> 
> The line-break strategy was fixed, and now it is according to
> the top one suggested.
> 
> The __free is a wrapper to __attribute__((__cleanup__())) so
> the variavel definition is needed. And according to Julia, it
> is preferred to combine the declaration and the allocation to
> ensure that there is no return that can occur after the declaration,
> but before the allocation (or more precisely the initialization).
> If there is no other option for the initialization of the variable,
> then it should be NULL.
> 
>   drivers/leds/leds-powernv.c | 28 +++++++++-------------------
>   1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
> index 4f01acb75727..49ab8c9a3f29 100644
> --- a/drivers/leds/leds-powernv.c
> +++ b/drivers/leds/leds-powernv.c
> @@ -246,29 +246,25 @@ static int powernv_led_classdev(struct platform_device *pdev,
>   	const char *cur = NULL;
>   	int rc = -1;
>   	struct property *p;
> -	struct device_node *np;
>   	struct powernv_led_data *powernv_led;
>   	struct device *dev = &pdev->dev;
>   
> -	for_each_available_child_of_node(led_node, np) {
> +	for_each_available_child_of_node_scoped(led_node, np) {
>   		p = of_find_property(np, "led-types", NULL);
>   
>   		while ((cur = of_prop_next_string(p, cur)) != NULL) {
>   			powernv_led = devm_kzalloc(dev, sizeof(*powernv_led),
>   						   GFP_KERNEL);
> -			if (!powernv_led) {
> -				of_node_put(np);
> +			if (!powernv_led)
>   				return -ENOMEM;
> -			}
>   
>   			powernv_led->common = powernv_led_common;
>   			powernv_led->loc_code = (char *)np->name;
>   
>   			rc = powernv_led_create(dev, powernv_led, cur);
> -			if (rc) {
> -				of_node_put(np);
> +			if (rc)
>   				return rc;
> -			}
> +
>   		} /* while end */
>   	}
>   
> @@ -278,12 +274,11 @@ static int powernv_led_classdev(struct platform_device *pdev,
>   /* Platform driver probe */
>   static int powernv_led_probe(struct platform_device *pdev)
>   {
> -	struct device_node *led_node;
>   	struct powernv_led_common *powernv_led_common;
>   	struct device *dev = &pdev->dev;
> -	int rc;
> +	struct device_node *led_node
> +		__free(device_node) = of_find_node_by_path("/ibm,opal/leds");
>   
> -	led_node = of_find_node_by_path("/ibm,opal/leds");
>   	if (!led_node) {
>   		dev_err(dev, "%s: LED parent device node not found\n",
>   			__func__);
> @@ -292,20 +287,15 @@ static int powernv_led_probe(struct platform_device *pdev)
>   
>   	powernv_led_common = devm_kzalloc(dev, sizeof(*powernv_led_common),
>   					  GFP_KERNEL);
> -	if (!powernv_led_common) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> +	if (!powernv_led_common)
> +		return -ENOMEM;
>   
>   	mutex_init(&powernv_led_common->lock);
>   	powernv_led_common->max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>   
>   	platform_set_drvdata(pdev, powernv_led_common);
>   
> -	rc = powernv_led_classdev(pdev, led_node, powernv_led_common);
> -out:
> -	of_node_put(led_node);
> -	return rc;
> +	return powernv_led_classdev(pdev, led_node, powernv_led_common);
>   }
>   
>   /* Platform driver remove */

Hello,
Did you have a chance to look at the patch after the requested change?

Thank you,
Marilene

Re: [PATCH v3 linux-next] leds: powernv: replace of_node_put to __free

[Lee Jones]

On Fri, 07 Jun 2024, Marilene Andrade Garcia wrote:

> On 01/06/2024 00:17, MarileneGarcia wrote:
> > Use __free for device_node values, and thus drop calls to
> > of_node_put.
> > 
> > The variable attribute __free adds a scope based cleanup to
> > the device node. The goal is to reduce memory management issues
> > in the kernel code.
> > 
> > The of_node_put calls were removed, and the
> > for_each_available_child_of_node was replaced to the equivalent
> > for_each_available_child_of_node_scoped which use the __free.
> > 
> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: MarileneGarcia <marilene.agarcia@gmail.com>
> > ---
> > Hello,
> > Thank you for the feedback.
> > 
> > The line-break strategy was fixed, and now it is according to
> > the top one suggested.
> > 
> > The __free is a wrapper to __attribute__((__cleanup__())) so
> > the variavel definition is needed. And according to Julia, it
> > is preferred to combine the declaration and the allocation to
> > ensure that there is no return that can occur after the declaration,
> > but before the allocation (or more precisely the initialization).
> > If there is no other option for the initialization of the variable,
> > then it should be NULL.
> > 
> >   drivers/leds/leds-powernv.c | 28 +++++++++-------------------
> >   1 file changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
> > index 4f01acb75727..49ab8c9a3f29 100644
> > --- a/drivers/leds/leds-powernv.c
> > +++ b/drivers/leds/leds-powernv.c
> > @@ -246,29 +246,25 @@ static int powernv_led_classdev(struct platform_device *pdev,
> >   	const char *cur = NULL;
> >   	int rc = -1;
> >   	struct property *p;
> > -	struct device_node *np;
> >   	struct powernv_led_data *powernv_led;
> >   	struct device *dev = &pdev->dev;
> > -	for_each_available_child_of_node(led_node, np) {
> > +	for_each_available_child_of_node_scoped(led_node, np) {
> >   		p = of_find_property(np, "led-types", NULL);
> >   		while ((cur = of_prop_next_string(p, cur)) != NULL) {
> >   			powernv_led = devm_kzalloc(dev, sizeof(*powernv_led),
> >   						   GFP_KERNEL);
> > -			if (!powernv_led) {
> > -				of_node_put(np);
> > +			if (!powernv_led)
> >   				return -ENOMEM;
> > -			}
> >   			powernv_led->common = powernv_led_common;
> >   			powernv_led->loc_code = (char *)np->name;
> >   			rc = powernv_led_create(dev, powernv_led, cur);
> > -			if (rc) {
> > -				of_node_put(np);
> > +			if (rc)
> >   				return rc;
> > -			}
> > +
> >   		} /* while end */
> >   	}
> > @@ -278,12 +274,11 @@ static int powernv_led_classdev(struct platform_device *pdev,
> >   /* Platform driver probe */
> >   static int powernv_led_probe(struct platform_device *pdev)
> >   {
> > -	struct device_node *led_node;
> >   	struct powernv_led_common *powernv_led_common;
> >   	struct device *dev = &pdev->dev;
> > -	int rc;
> > +	struct device_node *led_node
> > +		__free(device_node) = of_find_node_by_path("/ibm,opal/leds");
> > -	led_node = of_find_node_by_path("/ibm,opal/leds");
> >   	if (!led_node) {
> >   		dev_err(dev, "%s: LED parent device node not found\n",
> >   			__func__);
> > @@ -292,20 +287,15 @@ static int powernv_led_probe(struct platform_device *pdev)
> >   	powernv_led_common = devm_kzalloc(dev, sizeof(*powernv_led_common),
> >   					  GFP_KERNEL);
> > -	if (!powernv_led_common) {
> > -		rc = -ENOMEM;
> > -		goto out;
> > -	}
> > +	if (!powernv_led_common)
> > +		return -ENOMEM;
> >   	mutex_init(&powernv_led_common->lock);
> >   	powernv_led_common->max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> >   	platform_set_drvdata(pdev, powernv_led_common);
> > -	rc = powernv_led_classdev(pdev, led_node, powernv_led_common);
> > -out:
> > -	of_node_put(led_node);
> > -	return rc;
> > +	return powernv_led_classdev(pdev, led_node, powernv_led_common);
> >   }
> >   /* Platform driver remove */
> 
> Hello,
> Did you have a chance to look at the patch after the requested change?

Was that for me?  Please refrain from pinging.

My TODO list is long and I'm doing my best to get through it.

-- 
Lee Jones [李琼斯]

Re: (subset) [PATCH v3 linux-next] leds: powernv: replace of_node_put to __free

[Lee Jones]

On Sat, 01 Jun 2024 00:17:13 -0300, MarileneGarcia wrote:
> Use __free for device_node values, and thus drop calls to
> of_node_put.
> 
> The variable attribute __free adds a scope based cleanup to
> the device node. The goal is to reduce memory management issues
> in the kernel code.
> 
> [...]

Applied, thanks!

[1/1] leds: powernv: replace of_node_put to __free
      commit: 7c85503b6d597b84ea58fe3dd95cd9eaeb1f3206

--
Lee Jones [李琼斯]