Message-ID: <2d16bf36-57d3-4c54-bbee-2e7d93399f29@gmail.com>
Date: Wed, 27 Nov 2024 16:14:41 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/8] iio: gts: Simplify using __free

Hi & Thanks Jonathan,

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

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

Powered by blists - more mailing lists