Re: [PATCH v2 2/8] iio: gts: Simplify using __free

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 25 Nov 2024 11:16:22 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Jonathan,
> 
> Thanks once again for the review :)
> 
> On 23/11/2024 18:37, Jonathan Cameron wrote:
> > On Thu, 21 Nov 2024 10:20:07 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> The error path in the gain_to_scaletables() uses goto for unwinding an
> >> allocation on failure. This can be slightly simplified by using the
> >> automated free when exiting the scope.
> >>
> >> Use __free(kfree) and drop the goto based error handling.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >>
> >> ---
> >>
> >> Revision history:
> >>    v1 => v2:
> >>    - patch number changed because a change was added to the series.
> >>    - rebased on iio/testing to avoid conflicts with queued fixes
> >> ---
> >>   drivers/iio/industrialio-gts-helper.c | 19 ++++++++-----------
> >>   1 file changed, 8 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> >> index 291c0fc332c9..602d3d338e66 100644
> >> --- a/drivers/iio/industrialio-gts-helper.c
> >> +++ b/drivers/iio/industrialio-gts-helper.c
> >> @@ -4,6 +4,7 @@
> >>    * Copyright (c) 2023 Matti Vaittinen <mazziesaccount@gmail.com>
> >>    */
> >>   
> >> +#include <linux/cleanup.h>
> >>   #include <linux/device.h>
> >>   #include <linux/errno.h>
> >>   #include <linux/export.h>
> >> @@ -167,8 +168,8 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
> >>   
> >>   static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> >>   {
> >> -	int i, j, new_idx, time_idx, ret = 0;
> >> -	int *all_gains;
> >> +	int ret, i, j, new_idx, time_idx;
> >> +	int *all_gains __free(kfree) = NULL;  
> > See the docs in cleanup.h (added recently).
> > 
> > Constructor and destructor should go together.   Dan wrote good docs on this
> > (which are now in cleanup.h) so I'll not go into why!  
> 
> I went through the cleanup.h, and noticed the nice explanation for the 
> pitfall where we have multiple "scoped operations" with specific 
> ordering required. I didn't see other reasoning beyond that - I do hope 
> I didn't miss anything.
> 
> I find introducing variables mid-function very confusing. Only exception 
> for this has been introducing temporary variables at the start of a 
> block, to reduce the scope. I would still like to avoid this when it 
> isn't absolutely necessary, as it bleeds my eyes :)
> 
> I really don't see why we would have other cleanups which required 
> specific ordering with the allocated "all_gains".
> 
> Anyways, if you think we really have a problem here, would it then 
> suffice if I moved the:
> 
>          gain_bytes = array_size(gts->num_hwgain, sizeof(int));
>          all_gains = kcalloc(gts->num_itime, gain_bytes, GFP_KERNEL);
>          if (!all_gains)
>                  return -ENOMEM;
> 
> to the beginning of the function, and the "int *all_gains __free(kfree) 
> = NULL;" as last variable declaration?
> 

No.  You need to follow the standard way. It is something we are
all getting used to, but all use of cleanup.h needs to follow same rules
so that reviewers find it easy to review once they are seeing lots of
instances of it.

Many indeed find this ugly but reality is it's happening all over the place
just usually hidden in a macro.  From cleanup.h look at how
guard() works for instance.


Jonathan

> (This is not optimal as we will then do the allocation even if 
> converting gains to scales failed - but I don't think this is a real 
> problem as this should never happen after the driver is proven working 
> for the first time).
> 
> > Upshot is this goes where you do the kcalloc, not up here.  
> 
> *whining* "but, but, but ... it is ugly..." :)

:)  It won't look ugly after a few years!
> 
> Yours,
> 	-- Matti