Message-ID: <670d860b-f6fc-4ef1-ba05-bbce24ed82fd@gmail.com>
Date: Mon, 2 Dec 2024 08:44:57 +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>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] iio: gts: Simplify using __free

On 30/11/2024 19:51, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 18:50:24 +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>
>> ---
>> This is derived from the:
>> https://lore.kernel.org/lkml/5efc30d832275778d1f48d7e2c75b1ecc63511d5.1732105157.git.mazziesaccount@gmail.com/
> 
> Hi Matti
> 
> A few comments on specific parts of this below
> 
> Thanks,
> 
> Jonathan
> 
>> +static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_bytes)
>> +{
>> +	int ret, i, j, new_idx, time_idx;
>> +	int *all_gains __free(kfree) = kcalloc(gts->num_itime, gain_bytes,
>> +					       GFP_KERNEL);
>> +
>>   	if (!all_gains)
>>   		return -ENOMEM;
>>   
>> @@ -232,10 +238,9 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>>   
>>   	gts->avail_all_scales_table = kcalloc(new_idx, 2 * sizeof(int),
>>   					      GFP_KERNEL);
> 
> I'm not particularly keen in a partial application of __free magic.

I am starting to think the partial application of __free would actually 
be what I preferred... (see below).

> Perhaps you can use a local variable for this and a no_free_ptr() to assign it after we know
> there can't be an error that requires it to be freed.

I am having second thoughts of this whole series. I do love the idea of 
__free() magic, when applied on a simple temporary allocations that are 
intended to be freed at the end of a function. Eg, for cases where we 
know the scope from the very beginning. With a consistent use of __free 
in such cases could make it much more obvious for a reader that this 
stuff is valid only for a duration of this block. I have a feeling that 
mixing the no_free_ptr() is a violation, and will obfuscate this.

I know I used it in this series while trying to simplify the flow - and 
I am already regretting this.

Additionally, I indeed am not okay with introducing variables in middle 
of a function. I do really feel quite strongly about that.

It seems that in many functions it makes sense to have some checks or 
potentially failing operations done before doing memory allocations. So, 
keeping the allocation at the start of a block can often require some 
additional "check/do these things before calling an internal function 
which does alloc + rest of the work" -wrappers.

It will then also mean that the internal function (called from a wrapper 
with checks) will lack of the aforementioned checks, and, is thus 
somehow unsafe. I am not saying such wrappers are always wrong - 
sometimes it may be ok - but it probably should be consistent approach 
and not a mixture of conventions depending on allocations...

Also, sometimes this would result some very strangely split functions 
with no well defined purpose.

As a result, I would definitely only use the "__free magic" in places 
where it does fit well for freeing a memory which is known to be needed 
only for a specific block. And, I would only use it where the alloc can 
be done at the beginning of a function in a rather natural way. This, 
however, is very likely to lead in mixed use of "__free magic" and 
regular allocs.

>> -	if (!gts->avail_all_scales_table) {
>> -		ret = -ENOMEM;
>> -		goto free_out;
>> -	}
>> +	if (!gts->avail_all_scales_table)
>> +		return -ENOMEM;
>> +
>>   	gts->num_avail_all_scales = new_idx;
>>   
>>   	for (i = 0; i < gts->num_avail_all_scales; i++) {
>> @@ -246,14 +251,25 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>>   		if (ret) {
>>   			kfree(gts->avail_all_scales_table);
>>   			gts->num_avail_all_scales = 0;
>> -			goto free_out;
>> +			return ret;
>>   		}
>>   	}
>>   
>> -free_out:
>> -	kfree(all_gains);
>> +	return 0;
>> +}
>>   
>> -	return ret;
>> +static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>> +{
>> +	int ret;
>> +	size_t gain_bytes;
>> +
>> +	ret = fill_and_sort_scaletables(gts, gains, scales);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gain_bytes = array_size(gts->num_hwgain, sizeof(int));
> 
> array_size is documented as being for 2D arrays, not an array of a multi byte
> type.  We should not use it for this purpose.

Thanks for pointing this out. I was not familiar with that. I am 
actually pretty sure that using the array_size() has been recommended to 
me :)

> I'd be tempted to not worry about overflow, but if you do want to be sure then
> copy what kcalloc does and use a check_mul_overflow()
> 
> https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/slab.h#L919

Thanks for the direct pointer :) I'll take a look at it!

> 
> You don't have to tidy that up in this patch though.
> 
>> +
>> +	return build_combined_table(gts, gains, gain_bytes);
>>   }
> 
>>   
>> +/**
>> + * iio_gts_build_avail_time_table - build table of available integration times
>> + * @gts:	Gain time scale descriptor
>> + *
>> + * Build the table which can represent the available times to be returned
>> + * to users using the read_avail-callback.
>> + *
>> + * NOTE: Space allocated for the tables must be freed using
>> + * iio_gts_purge_avail_time_table() when the tables are no longer needed.
>> + *
>> + * Return: 0 on success.
>> + */
>> +static int iio_gts_build_avail_time_table(struct iio_gts *gts)
> Hmm. I guess this wrapper exists because perhaps you aren't comfortable
> yet with the __free() handling mid function.  It does not harm so I'm fine
> having this.

Yes. This was the reason for this wrapper. But I'm not really happy 
about the wrappers either (even if I agree with you that it does not 
really hurt here). Furthermore, if you're feeling strongly about not 
mixing the __free and regular allocs, then I simply prefer not using the 
magic one. I don't think using the __free with allocations intended to 
last longer than the scope is clear. Ues, goto sequences may not always 
be simple, but at least people are used to be wary with them.

>> +{
>> +	if (!gts->num_itime)
>> +		return 0;
>> +
>> +	return __iio_gts_build_avail_time_table(gts);
>> +}
>> +
>>   /**
>>    * iio_gts_purge_avail_time_table - free-up the available integration time table
>>    * @gts:	Gain time scale descriptor
> 

Thanks a ton for the review :) It helped me to clarify my thoughts on this!

Yours,
	-- Matti

Powered by blists - more mailing lists