1. Patchew
  2. linux
  3. [PATCH v4 0/6] thermal: scope/cleanup.h improvements
  4. View patch

[PATCH v4 2/6] thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init()

Krzysztof Kozlowski posted 6 patches 1 month, 2 weeks ago
[PATCH v4 2/6] thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init()
Posted by Krzysztof Kozlowski 1 month, 2 weeks ago 
Obtain the device node reference and allocate memory with
scoped/cleanup.h to reduce error handling and make the code a bit
simpler.

The code is not equivalent in one minor aspect: outgoing parameter
"*ntrips" will not be zeroed on errors of memory allocation.  This
difference is not important, because code was already not zeroing it in
case of earlier errors and the only caller does not rely on ntrips being
0 in case of errors.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Chen-Yu Tsai <wenst@chromium.org>

Changes in v4:
1. Significant change: kzalloc() also with scoped-handling so the entire
   error handling could be removed.
2. Due to above, drop review-tags (Chen-Yu, Jonathan).

Changes in v2:
1. Drop left-over of_node_put in regular exit path (Chen-Yu)
---
 drivers/thermal/thermal_of.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index f0ffc0e335ba9406f4fd858d6c561f9d23f4b842..37db435b54b124abf25b1d75d6cc4fb75f1c1e5c 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -95,11 +95,9 @@ static int thermal_of_populate_trip(struct device_node *np,
 
 static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *ntrips)
 {
-	struct thermal_trip *tt;
-	struct device_node *trips;
 	int ret, count;
 
-	trips = of_get_child_by_name(np, "trips");
+	struct device_node *trips __free(device_node) = of_get_child_by_name(np, "trips");
 	if (!trips) {
 		pr_err("Failed to find 'trips' node\n");
 		return ERR_PTR(-EINVAL);
@@ -108,36 +106,23 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
 	count = of_get_child_count(trips);
 	if (!count) {
 		pr_err("No trip point defined\n");
-		ret = -EINVAL;
-		goto out_of_node_put;
+		return ERR_PTR(-EINVAL);
 	}
 
-	tt = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
-	if (!tt) {
-		ret = -ENOMEM;
-		goto out_of_node_put;
-	}
-
-	*ntrips = count;
+	struct thermal_trip *tt __free(kfree) = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
+	if (!tt)
+		return ERR_PTR(-ENOMEM);
 
 	count = 0;
 	for_each_child_of_node_scoped(trips, trip) {
 		ret = thermal_of_populate_trip(trip, &tt[count++]);
 		if (ret)
-			goto out_kfree;
+			return ERR_PTR(ret);
 	}
 
-	of_node_put(trips);
+	*ntrips = count;
 
-	return tt;
-
-out_kfree:
-	kfree(tt);
-	*ntrips = 0;
-out_of_node_put:
-	of_node_put(trips);
-
-	return ERR_PTR(ret);
+	return no_free_ptr(tt);
 }
 
 static struct device_node *of_thermal_zone_find(struct device_node *sensor, int id)

-- 
2.43.0
Re: [PATCH v4 2/6] thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init()
Posted by Jonathan Cameron 1 month, 1 week ago 
On Thu, 10 Oct 2024 20:06:18 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Obtain the device node reference and allocate memory with
> scoped/cleanup.h to reduce error handling and make the code a bit
> simpler.
> 
> The code is not equivalent in one minor aspect: outgoing parameter
> "*ntrips" will not be zeroed on errors of memory allocation.  This
> difference is not important, because code was already not zeroing it in
> case of earlier errors and the only caller does not rely on ntrips being
> 0 in case of errors.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Trivial unrelated comment inline + maybe return_ptr() is the way to go as
Chen-Yu mentioned.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Chen-Yu Tsai <wenst@chromium.org>
> 
> Changes in v4:
> 1. Significant change: kzalloc() also with scoped-handling so the entire
>    error handling could be removed.
> 2. Due to above, drop review-tags (Chen-Yu, Jonathan).
> 
> Changes in v2:
> 1. Drop left-over of_node_put in regular exit path (Chen-Yu)
> ---
>  drivers/thermal/thermal_of.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index f0ffc0e335ba9406f4fd858d6c561f9d23f4b842..37db435b54b124abf25b1d75d6cc4fb75f1c1e5c 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -95,11 +95,9 @@ static int thermal_of_populate_trip(struct device_node *np,
>  
>  static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *ntrips)
>  {
> -	struct thermal_trip *tt;
> -	struct device_node *trips;
>  	int ret, count;
>  
> -	trips = of_get_child_by_name(np, "trips");
> +	struct device_node *trips __free(device_node) = of_get_child_by_name(np, "trips");
>  	if (!trips) {
>  		pr_err("Failed to find 'trips' node\n");
>  		return ERR_PTR(-EINVAL);
> @@ -108,36 +106,23 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
>  	count = of_get_child_count(trips);
>  	if (!count) {
>  		pr_err("No trip point defined\n");
> -		ret = -EINVAL;
> -		goto out_of_node_put;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
> -	tt = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
> -	if (!tt) {
> -		ret = -ENOMEM;
> -		goto out_of_node_put;
> -	}
> -
> -	*ntrips = count;
> +	struct thermal_trip *tt __free(kfree) = kzalloc(sizeof(*tt) * count, GFP_KERNEL);

Trivial and unrelated, but maybe kcalloc(count, sizeof(tt), GFP_KERNEL);

> +	if (!tt)
> +		return ERR_PTR(-ENOMEM);
>  
>  	count = 0;
>  	for_each_child_of_node_scoped(trips, trip) {
>  		ret = thermal_of_populate_trip(trip, &tt[count++]);
>  		if (ret)
> -			goto out_kfree;
> +			return ERR_PTR(ret);
>  	}
>  
> -	of_node_put(trips);
> +	*ntrips = count;
>  
> -	return tt;
> -
> -out_kfree:
> -	kfree(tt);
> -	*ntrips = 0;
> -out_of_node_put:
> -	of_node_put(trips);
> -
> -	return ERR_PTR(ret);
> +	return no_free_ptr(tt);
>  }
>  
>  static struct device_node *of_thermal_zone_find(struct device_node *sensor, int id)
>
Re: [PATCH v4 2/6] thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init()
Posted by Chen-Yu Tsai 1 month, 2 weeks ago 
On Fri, Oct 11, 2024 at 2:06 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Obtain the device node reference and allocate memory with
> scoped/cleanup.h to reduce error handling and make the code a bit
> simpler.
>
> The code is not equivalent in one minor aspect: outgoing parameter
> "*ntrips" will not be zeroed on errors of memory allocation.  This
> difference is not important, because code was already not zeroing it in
> case of earlier errors and the only caller does not rely on ntrips being
> 0 in case of errors.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Chen-Yu Tsai <wenst@chromium.org>
>
> Changes in v4:
> 1. Significant change: kzalloc() also with scoped-handling so the entire
>    error handling could be removed.
> 2. Due to above, drop review-tags (Chen-Yu, Jonathan).

The additional changes are the same as what I had done, except that
I used "return_ptr(tt)" instead of "return no_free_ptr(tt)", and I
had reset *ntrips to 0 at the beginning.

So,

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

> Changes in v2:
> 1. Drop left-over of_node_put in regular exit path (Chen-Yu)
> ---
>  drivers/thermal/thermal_of.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index f0ffc0e335ba9406f4fd858d6c561f9d23f4b842..37db435b54b124abf25b1d75d6cc4fb75f1c1e5c 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -95,11 +95,9 @@ static int thermal_of_populate_trip(struct device_node *np,
>
>  static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *ntrips)
>  {
> -       struct thermal_trip *tt;
> -       struct device_node *trips;
>         int ret, count;
>
> -       trips = of_get_child_by_name(np, "trips");
> +       struct device_node *trips __free(device_node) = of_get_child_by_name(np, "trips");
>         if (!trips) {
>                 pr_err("Failed to find 'trips' node\n");
>                 return ERR_PTR(-EINVAL);
> @@ -108,36 +106,23 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
>         count = of_get_child_count(trips);
>         if (!count) {
>                 pr_err("No trip point defined\n");
> -               ret = -EINVAL;
> -               goto out_of_node_put;
> +               return ERR_PTR(-EINVAL);
>         }
>
> -       tt = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
> -       if (!tt) {
> -               ret = -ENOMEM;
> -               goto out_of_node_put;
> -       }
> -
> -       *ntrips = count;
> +       struct thermal_trip *tt __free(kfree) = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
> +       if (!tt)
> +               return ERR_PTR(-ENOMEM);
>
>         count = 0;
>         for_each_child_of_node_scoped(trips, trip) {
>                 ret = thermal_of_populate_trip(trip, &tt[count++]);
>                 if (ret)
> -                       goto out_kfree;
> +                       return ERR_PTR(ret);
>         }
>
> -       of_node_put(trips);
> +       *ntrips = count;
>
> -       return tt;
> -
> -out_kfree:
> -       kfree(tt);
> -       *ntrips = 0;
> -out_of_node_put:
> -       of_node_put(trips);
> -
> -       return ERR_PTR(ret);
> +       return no_free_ptr(tt);
>  }
>
>  static struct device_node *of_thermal_zone_find(struct device_node *sensor, int id)
>
> --
> 2.43.0
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.