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

[Matti Vaittinen]

Hi & Thanks Jonathan,

On 26/11/2024 19:52, Jonathan Cameron wrote:
> On Mon, 25 Nov 2024 11:16:22 +0200
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > > 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@xxxxxxxxx>
> > > >     */
> > > >    
> > > > +#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.

Well, those macros are better in that the variables they internally 
declare aren't visible in the outside code. The 'all_gains' pointer is 
used throughout the function, and I really dislike having local 
variables which aren't declared at the beginning of a function/block 
emerge out of nowhere. Makes me think: "why this terribly named global?".

Well, maybe I really just need to try to adapt these things but I will 
drop this one out of the series for now. TBH, I don't really like how 
this table building function looks like. It's too long and confusing. I 
will see if there is a sane way to split it, and maybe get the __free() 
pointers to the beginning of a function as well ;)

> > (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!

Could be. But now I am in the middle of "everything used to be better in 
the good old day" -crisis. Playing 8-bit NES games and wondering if I 
could fix my old C64 ^_^;

In any case, thanks for the guidance (and optimism!) XD

Yours,
	-- Matti