| Message ID | 20241010-b4-cleanup-h-of-node-put-thermal-v4-0-bfbe29ad81f4@linaro.org |
|---|---|
| Headers | show |
| Series | thermal: scope/cleanup.h improvements | expand |
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 >
On 14/10/2024 10:32, Chen-Yu Tsai wrote: > On Fri, Oct 11, 2024 at 2:06 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> Changes in v4: >> - Patch 2: rewrite, significant change: kzalloc() also with >> scoped-handling so the entire error handling could be removed. >> Due to above, drop review-tags (Chen-Yu, Jonathan). >> - Add Rb tags for other patches. >> - Link to v3: https://lore.kernel.org/r/20241008-b4-cleanup-h-of-node-put-thermal-v3-0-825122398f71@linaro.org >> >> Changes in v3: >> - Rebase, because there was bigger rework in thermal code. >> This made two patches obsolete, but brought new one: >> 1/6: thermal: of: Simplify thermal_of_should_bind with scoped for each OF child >> - Link to v2: https://lore.kernel.org/r/20240816-b4-cleanup-h-of-node-put-thermal-v2-0-cee9fc490478@linaro.org >> >> Changes in v2: >> - Drop left-over of_node_put in regular exit path (Chen-Yu) >> - Link to v1: https://lore.kernel.org/r/20240814-b4-cleanup-h-of-node-put-thermal-v1-0-7a1381e1627e@linaro.org >> >> Few code simplifications with scope/cleanup.h. >> >> Best regards, >> Krzysztof >> >> --- >> Krzysztof Kozlowski (6): >> thermal: of: Simplify thermal_of_should_bind with scoped for each OF child > > I couldn't find this in my inbox. But since I already reviewed all the other > patches, and I looked at this one on lore, consider the whole series is now Sorry for that. Your wens@csie.org was cc-ed, but not the chromium. If I respin, I will add both on Cc. > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> Best regards, Krzysztof
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) >
Changes in v4: - Patch 2: rewrite, significant change: kzalloc() also with scoped-handling so the entire error handling could be removed. Due to above, drop review-tags (Chen-Yu, Jonathan). - Add Rb tags for other patches. - Link to v3: https://lore.kernel.org/r/20241008-b4-cleanup-h-of-node-put-thermal-v3-0-825122398f71@linaro.org Changes in v3: - Rebase, because there was bigger rework in thermal code. This made two patches obsolete, but brought new one: 1/6: thermal: of: Simplify thermal_of_should_bind with scoped for each OF child - Link to v2: https://lore.kernel.org/r/20240816-b4-cleanup-h-of-node-put-thermal-v2-0-cee9fc490478@linaro.org Changes in v2: - Drop left-over of_node_put in regular exit path (Chen-Yu) - Link to v1: https://lore.kernel.org/r/20240814-b4-cleanup-h-of-node-put-thermal-v1-0-7a1381e1627e@linaro.org Few code simplifications with scope/cleanup.h. Best regards, Krzysztof --- Krzysztof Kozlowski (6): thermal: of: Simplify thermal_of_should_bind with scoped for each OF child thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init() thermal: of: Use scoped device node handling to simplify of_thermal_zone_find() thermal: qcom-spmi-adc-tm5: Simplify with scoped for each OF child loop thermal: tegra: Simplify with scoped for each OF child loop thermal: sun8i: Use scoped device node handling to simplify error paths drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 7 ++--- drivers/thermal/sun8i_thermal.c | 11 +++---- drivers/thermal/tegra/soctherm.c | 5 ++- drivers/thermal/thermal_of.c | 54 ++++++++++---------------------- 4 files changed, 25 insertions(+), 52 deletions(-) --- base-commit: 33ce24234fca4c083e6685a18b460a18ebb5d5c1 change-id: 20240814-b4-cleanup-h-of-node-put-thermal-2268440cc6f7 Best regards,