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
