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 anSee the docs in cleanup.h (added recently).
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;
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.
(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!