[PATCH 0/3] iio: gts: Simplify available scales building

[PATCH 0/3] iio: gts: Simplify available scales building

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

Simplify the available scales building.

Building the available scales tables looks confusing. It has also been a
source of a few memory leaks.

https://lore.kernel.org/lkml/20241016012453.2013302-1-ruanjinjie@huawei.com/
https://lore.kernel.org/lkml/20241011095512.3667549-1-ruanjinjie@huawei.com/

Attempt making it a bit less error prone by simplifying some error paths
using __free(kfree) from the cleanup.h. Try doing this without
introducing variables in the middle of a function while also improving
the logic of the functions. This is possble by splitting the long scale
building logic in a couple of different, well purposed functions, and
finally streamlining the logic a bit by taking out one extra 'total gains
to scales'-conversion.

Inspired by the discussion here:
https://lore.kernel.org/lkml/2d16bf36-57d3-4c54-bbee-2e7d93399f29@gmail.com/

Matti Vaittinen (3):
  iio: gts: Simplify using __free
  iio: gts: split table-building function
  iio: gts: simplify scale table build

 drivers/iio/industrialio-gts-helper.c | 171 ++++++++++++++------------
 1 file changed, 91 insertions(+), 80 deletions(-)


base-commit: a61ff7eac77e86de828fe28c4e42b8ae9ec2b195
-- 
2.47.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/3] iio: gts: Simplify using __free

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5785 bytes --]

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@gmail.com>
---
This is derived from the:
https://lore.kernel.org/lkml/5efc30d832275778d1f48d7e2c75b1ecc63511d5.1732105157.git.mazziesaccount@gmail.com/
---
 drivers/iio/industrialio-gts-helper.c | 108 +++++++++++++++-----------
 1 file changed, 63 insertions(+), 45 deletions(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 291c0fc332c9..e15d0112e9e3 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@gmail.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/export.h>
@@ -165,11 +166,9 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
 	return *(int *)a - *(int *)b;
 }
 
-static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
+static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **scales)
 {
-	int i, j, new_idx, time_idx, ret = 0;
-	int *all_gains;
-	size_t gain_bytes;
+	int i, j, ret;
 
 	for (i = 0; i < gts->num_itime; i++) {
 		/*
@@ -189,8 +188,15 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
 		}
 	}
 
-	gain_bytes = array_size(gts->num_hwgain, sizeof(int));
-	all_gains = kcalloc(gts->num_itime, gain_bytes, GFP_KERNEL);
+	return 0;
+}
+
+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);
-	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));
+
+	return build_combined_table(gts, gains, gain_bytes);
 }
 
 /**
@@ -337,26 +353,11 @@ static void iio_gts_us_to_int_micro(int *time_us, int *int_micro_times,
 	}
 }
 
-/**
- * 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)
+static int __iio_gts_build_avail_time_table(struct iio_gts *gts)
 {
-	int *times, i, j, idx = 0, *int_micro_times;
-
-	if (!gts->num_itime)
-		return 0;
+	int i, j, idx = 0, *int_micro_times;
+	int *times __free(kfree) = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
 
-	times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
 	if (!times)
 		return -ENOMEM;
 
@@ -384,25 +385,42 @@ static int iio_gts_build_avail_time_table(struct iio_gts *gts)
 
 	/* create a list of times formatted as list of IIO_VAL_INT_PLUS_MICRO */
 	int_micro_times = kcalloc(idx, sizeof(int) * 2, GFP_KERNEL);
-	if (int_micro_times) {
-		/*
-		 * This is just to survive a unlikely corner-case where times in
-		 * the given time table were not unique. Else we could just
-		 * trust the gts->num_itime.
-		 */
-		gts->num_avail_time_tables = idx;
-		iio_gts_us_to_int_micro(times, int_micro_times, idx);
-	}
-
-	gts->avail_time_tables = int_micro_times;
-	kfree(times);
-
 	if (!int_micro_times)
 		return -ENOMEM;
 
+	/*
+	 * This is just to survive a unlikely corner-case where times in
+	 * the given time table were not unique. Else we could just
+	 * trust the gts->num_itime.
+	 */
+	gts->num_avail_time_tables = idx;
+	iio_gts_us_to_int_micro(times, int_micro_times, idx);
+
+	gts->avail_time_tables = int_micro_times;
+
 	return 0;
 }
 
+/**
+ * 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)
+{
+	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
-- 
2.47.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/3] iio: gts: split table-building function

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3636 bytes --]

The function which builds the available scales tables for the GTS
helpers is hard to follow. Split it up so it is more obvious for
occasional reader to see what is done and why.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/industrialio-gts-helper.c | 66 +++++++++++++++++----------
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index e15d0112e9e3..7f900f578f1d 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -191,14 +191,10 @@ static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **sca
 	return 0;
 }
 
-static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_bytes)
+static int combine_gain_tables(struct iio_gts *gts, int **gains,
+			       int *all_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;
+	int i, new_idx, time_idx;
 
 	/*
 	 * We assume all the gains for same integration time were unique.
@@ -212,8 +208,8 @@ static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_by
 	new_idx = gts->num_hwgain;
 
 	while (time_idx-- > 0) {
-		for (j = 0; j < gts->num_hwgain; j++) {
-			int candidate = gains[time_idx][j];
+		for (i = 0; i < gts->num_hwgain; i++) {
+			int candidate = gains[time_idx][i];
 			int chk;
 
 			if (candidate > all_gains[new_idx - 1]) {
@@ -236,25 +232,47 @@ static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_by
 		}
 	}
 
-	gts->avail_all_scales_table = kcalloc(new_idx, 2 * sizeof(int),
-					      GFP_KERNEL);
-	if (!gts->avail_all_scales_table)
-		return -ENOMEM;
+	return new_idx;
+}
 
-	gts->num_avail_all_scales = new_idx;
+static int *build_all_scales_table(struct iio_gts *gts, int *all_gains, int num_scales)
+{
+	int i, ret;
+	int *all_scales __free(kfree) = kcalloc(num_scales, 2 * sizeof(int),
+						GFP_KERNEL);
 
-	for (i = 0; i < gts->num_avail_all_scales; i++) {
-		ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
-					&gts->avail_all_scales_table[i * 2],
-					&gts->avail_all_scales_table[i * 2 + 1]);
+	if (!all_scales)
+		return ERR_PTR(-ENOMEM);
 
-		if (ret) {
-			kfree(gts->avail_all_scales_table);
-			gts->num_avail_all_scales = 0;
-			return ret;
-		}
+	for (i = 0; i < num_scales; i++) {
+		ret = iio_gts_total_gain_to_scale(gts, all_gains[i], &all_scales[i * 2],
+						  &all_scales[i * 2 + 1]);
+		if (ret)
+			return ERR_PTR(ret);
 	}
 
+	return_ptr(all_scales);
+}
+
+static int build_combined_tables(struct iio_gts *gts,
+				 int **gains, size_t gain_bytes)
+{
+	int *all_scales, num_gains;
+	int *all_gains __free(kfree) = kcalloc(gts->num_itime, gain_bytes,
+					       GFP_KERNEL);
+
+	if (!all_gains)
+		return -ENOMEM;
+
+	num_gains = combine_gain_tables(gts, gains, all_gains, gain_bytes);
+
+	all_scales = build_all_scales_table(gts, all_gains, num_gains);
+	if (IS_ERR(all_scales))
+		return PTR_ERR(all_scales);
+
+	gts->num_avail_all_scales = num_gains;
+	gts->avail_all_scales_table = all_scales;
+
 	return 0;
 }
 
@@ -269,7 +287,7 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
 
 	gain_bytes = array_size(gts->num_hwgain, sizeof(int));
 
-	return build_combined_table(gts, gains, gain_bytes);
+	return build_combined_tables(gts, gains, gain_bytes);
 }
 
 /**
-- 
2.47.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] iio: gts: simplify scale table build

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 6326 bytes --]

The GTS helpers offer two different set of "available scales" -tables.
Drivers can choose to advertice the scales which are available on a
currently selected integration time (by just changing the hwgain).
Another option is to list all scales which can be supported using any of
the integration times. This is useful for drivers which allow scale
setting to also change the integration time to meet the scale user
prefers.

The helper function which build these tables for the GTS did firstbuild
the "time specific" scale arrays for all the times. This is done by
calculating the scales based on the integration time specific "total
gain" arrays (gain contributed by both the integration time and hw-gain).

After this the helper code calculates an array for all available scales.
This is done combining all the time specific total-gains into one sorted
array, removing dublicate gains and finally converting the gains to
scales as above.

This can be somewhat simplified by changing the logic for calculating
the 'all available scales' -array to directly use the time specific
scale arrays instead of time specific total-gain arrays. Code can
directly just add all the already computed time specific scales to one
big 'all scales'-array, keep it sorted and remove duplicates.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---

This has been tested by IIO-gts kunit tests only. All testing is
appreciated.

Comparing the scales is not as pretty as comparing the gains was, as
scales are in two ints where the gains were in one. This makes the code
slightly more hairy. I however believe that the logic is now more
obvious. This might be more important for one reading this later...
---
 drivers/iio/industrialio-gts-helper.c | 109 ++++++++++----------------
 1 file changed, 42 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 7f900f578f1d..31101848b194 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -191,86 +191,61 @@ static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **sca
 	return 0;
 }
 
-static int combine_gain_tables(struct iio_gts *gts, int **gains,
-			       int *all_gains, size_t gain_bytes)
+static int scale_eq(int *sc1, int *sc2)
 {
-	int i, new_idx, time_idx;
+	return *sc1 == *sc2 && *(sc1 + 1) == *(sc2 + 1);
+}
 
-	/*
-	 * We assume all the gains for same integration time were unique.
-	 * It is likely the first time table had greatest time multiplier as
-	 * the times are in the order of preference and greater times are
-	 * usually preferred. Hence we start from the last table which is likely
-	 * to have the smallest total gains.
-	 */
-	time_idx = gts->num_itime - 1;
-	memcpy(all_gains, gains[time_idx], gain_bytes);
-	new_idx = gts->num_hwgain;
+static int scale_smaller(int *sc1, int *sc2)
+{
+	if (*sc1 != *sc2)
+		return *sc1 < *sc2;
+
+	/* If integer parts are equal, fixp parts */
+	return *(sc1 + 1) < *(sc2 + 1);
+}
+
+static int do_combined_scaletable(struct iio_gts *gts, int **scales, size_t scale_bytes)
+{
+	int t_idx, i, new_idx;
+	int *all_scales = kcalloc(gts->num_itime, scale_bytes, GFP_KERNEL);
 
-	while (time_idx-- > 0) {
-		for (i = 0; i < gts->num_hwgain; i++) {
-			int candidate = gains[time_idx][i];
+	if (!all_scales)
+		return -ENOMEM;
+
+	t_idx = gts->num_itime - 1;
+	memcpy(all_scales, scales[t_idx], scale_bytes);
+	new_idx = gts->num_hwgain * 2;
+
+	while (t_idx-- > 0) {
+		for (i = 0; i < gts->num_hwgain ; i++) {
+			int *candidate = &scales[t_idx][i * 2];
 			int chk;
 
-			if (candidate > all_gains[new_idx - 1]) {
-				all_gains[new_idx] = candidate;
-				new_idx++;
+			if (scale_smaller(candidate, &all_scales[new_idx - 2])) {
+				all_scales[new_idx] = *candidate;
+				all_scales[new_idx + 1] = *(candidate + 1);
+				new_idx += 2;
 
 				continue;
 			}
-			for (chk = 0; chk < new_idx; chk++)
-				if (candidate <= all_gains[chk])
+			for (chk = 0; chk < new_idx; chk += 2)
+				if (!scale_smaller(candidate, &all_scales[chk]))
 					break;
 
-			if (candidate == all_gains[chk])
+
+			if (scale_eq(candidate, &all_scales[chk]))
 				continue;
 
-			memmove(&all_gains[chk + 1], &all_gains[chk],
+			memmove(&all_scales[chk + 2], &all_scales[chk],
 				(new_idx - chk) * sizeof(int));
-			all_gains[chk] = candidate;
-			new_idx++;
+			all_scales[chk] = *candidate;
+			all_scales[chk + 1] = *(candidate + 1);
+			new_idx += 2;
 		}
 	}
 
-	return new_idx;
-}
-
-static int *build_all_scales_table(struct iio_gts *gts, int *all_gains, int num_scales)
-{
-	int i, ret;
-	int *all_scales __free(kfree) = kcalloc(num_scales, 2 * sizeof(int),
-						GFP_KERNEL);
-
-	if (!all_scales)
-		return ERR_PTR(-ENOMEM);
-
-	for (i = 0; i < num_scales; i++) {
-		ret = iio_gts_total_gain_to_scale(gts, all_gains[i], &all_scales[i * 2],
-						  &all_scales[i * 2 + 1]);
-		if (ret)
-			return ERR_PTR(ret);
-	}
-
-	return_ptr(all_scales);
-}
-
-static int build_combined_tables(struct iio_gts *gts,
-				 int **gains, size_t gain_bytes)
-{
-	int *all_scales, num_gains;
-	int *all_gains __free(kfree) = kcalloc(gts->num_itime, gain_bytes,
-					       GFP_KERNEL);
-
-	if (!all_gains)
-		return -ENOMEM;
-
-	num_gains = combine_gain_tables(gts, gains, all_gains, gain_bytes);
-
-	all_scales = build_all_scales_table(gts, all_gains, num_gains);
-	if (IS_ERR(all_scales))
-		return PTR_ERR(all_scales);
-
-	gts->num_avail_all_scales = num_gains;
+	gts->num_avail_all_scales = new_idx / 2;
 	gts->avail_all_scales_table = all_scales;
 
 	return 0;
@@ -279,15 +254,15 @@ static int build_combined_tables(struct iio_gts *gts,
 static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
 {
 	int ret;
-	size_t gain_bytes;
+	size_t scale_bytes;
 
 	ret = fill_and_sort_scaletables(gts, gains, scales);
 	if (ret)
 		return ret;
 
-	gain_bytes = array_size(gts->num_hwgain, sizeof(int));
+	scale_bytes = array_size(gts->num_hwgain, 2 * sizeof(int));
 
-	return build_combined_tables(gts, gains, gain_bytes);
+	return do_combined_scaletable(gts, scales, scale_bytes);
 }
 
 /**
-- 
2.47.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] iio: gts: Simplify using __free

[Jonathan Cameron]

On Thu, 28 Nov 2024 18:50:24 +0200
Matti Vaittinen <mazziesaccount@gmail.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@gmail.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.

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.

> -	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.

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

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.

> +{
> +	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

Re: [PATCH 3/3] iio: gts: simplify scale table build

[Jonathan Cameron]

On Thu, 28 Nov 2024 18:51:00 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The GTS helpers offer two different set of "available scales" -tables.
> Drivers can choose to advertice the scales which are available on a
> currently selected integration time (by just changing the hwgain).
> Another option is to list all scales which can be supported using any of
> the integration times. This is useful for drivers which allow scale
> setting to also change the integration time to meet the scale user
> prefers.
> 
> The helper function which build these tables for the GTS did firstbuild
The helper function which builds these tables for the GTS first builds the "time specific" ..

> the "time specific" scale arrays for all the times. This is done by
> calculating the scales based on the integration time specific "total
> gain" arrays (gain contributed by both the integration time and hw-gain).
> 
> After this the helper code calculates an array for all available scales.
> This is done combining all the time specific total-gains into one sorted
> array, removing dublicate gains and finally converting the gains to
> scales as above.
> 
> This can be somewhat simplified by changing the logic for calculating
> the 'all available scales' -array to directly use the time specific
> scale arrays instead of time specific total-gain arrays. Code can
> directly just add all the already computed time specific scales to one
> big 'all scales'-array, keep it sorted and remove duplicates.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
Minor comments inline.

Thanks,

Jonathan

> ---
> 
> This has been tested by IIO-gts kunit tests only. All testing is
> appreciated.
> 
> Comparing the scales is not as pretty as comparing the gains was, as
> scales are in two ints where the gains were in one. This makes the code
> slightly more hairy. I however believe that the logic is now more
> obvious. This might be more important for one reading this later...
> ---
>  drivers/iio/industrialio-gts-helper.c | 109 ++++++++++----------------
>  1 file changed, 42 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 7f900f578f1d..31101848b194 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -191,86 +191,61 @@ static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **sca
>  	return 0;
>  }
>  
> -static int combine_gain_tables(struct iio_gts *gts, int **gains,
> -			       int *all_gains, size_t gain_bytes)
> +static int scale_eq(int *sc1, int *sc2)
>  {
> -	int i, new_idx, time_idx;
> +	return *sc1 == *sc2 && *(sc1 + 1) == *(sc2 + 1);
	return sc1[0] == sc2[0] && sc1[1] == sc2[1];

Would be easier to read in my opinion.

> +}
>  
> -	/*
> -	 * We assume all the gains for same integration time were unique.
> -	 * It is likely the first time table had greatest time multiplier as
> -	 * the times are in the order of preference and greater times are
> -	 * usually preferred. Hence we start from the last table which is likely
> -	 * to have the smallest total gains.
> -	 */
> -	time_idx = gts->num_itime - 1;
> -	memcpy(all_gains, gains[time_idx], gain_bytes);
> -	new_idx = gts->num_hwgain;
> +static int scale_smaller(int *sc1, int *sc2)
> +{
> +	if (*sc1 != *sc2)
> +		return *sc1 < *sc2;
> +
> +	/* If integer parts are equal, fixp parts */
> +	return *(sc1 + 1) < *(sc2 + 1);
> +}
> +
> +static int do_combined_scaletable(struct iio_gts *gts, int **scales, size_t scale_bytes)
> +{
> +	int t_idx, i, new_idx;
> +	int *all_scales = kcalloc(gts->num_itime, scale_bytes, GFP_KERNEL);
>  
> -	while (time_idx-- > 0) {
> -		for (i = 0; i < gts->num_hwgain; i++) {
> -			int candidate = gains[time_idx][i];
> +	if (!all_scales)
> +		return -ENOMEM;
> +
> +	t_idx = gts->num_itime - 1;
> +	memcpy(all_scales, scales[t_idx], scale_bytes);
> +	new_idx = gts->num_hwgain * 2;
> +
> +	while (t_idx-- > 0) {
maybe a reverse for loop is clearer

	for (tidx = t_idx; tidx; tidx--)
For me a for loop indicates bounds are known and we change the index
one per loop. While loop indicates either unknown bounds, or that we are
modifying the index other than than in the loop controls.


> +		for (i = 0; i < gts->num_hwgain ; i++) {

Extra space after hwgain

> +			int *candidate = &scales[t_idx][i * 2];
>  			int chk;
>  
> -			if (candidate > all_gains[new_idx - 1]) {
> -				all_gains[new_idx] = candidate;
> -				new_idx++;
> +			if (scale_smaller(candidate, &all_scales[new_idx - 2])) {
> +				all_scales[new_idx] = *candidate;

Maybe candidate[0] and candidate[1] will be more readable as it's effectively a row of
of 2D matrix.

> +				all_scales[new_idx + 1] = *(candidate + 1);
> +				new_idx += 2;
>  
>  				continue;
>  			}
> -			for (chk = 0; chk < new_idx; chk++)
> -				if (candidate <= all_gains[chk])
> +			for (chk = 0; chk < new_idx; chk += 2)
> +				if (!scale_smaller(candidate, &all_scales[chk]))
>  					break;
>  
> -			if (candidate == all_gains[chk])
> +
> +			if (scale_eq(candidate, &all_scales[chk]))
>  				continue;
>  
> -			memmove(&all_gains[chk + 1], &all_gains[chk],
> +			memmove(&all_scales[chk + 2], &all_scales[chk],
>  				(new_idx - chk) * sizeof(int));
> -			all_gains[chk] = candidate;
> -			new_idx++;
> +			all_scales[chk] = *candidate;
As above. Maybe treat as a 2 element array.

> +			all_scales[chk + 1] = *(candidate + 1);
> +			new_idx += 2;
>  		}
>  	}

Re: [PATCH 3/3] iio: gts: simplify scale table build

[Matti Vaittinen]

On 30/11/2024 20:01, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 18:51:00 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The GTS helpers offer two different set of "available scales" -tables.
>> Drivers can choose to advertice the scales which are available on a
>> currently selected integration time (by just changing the hwgain).
>> Another option is to list all scales which can be supported using any of
>> the integration times. This is useful for drivers which allow scale
>> setting to also change the integration time to meet the scale user
>> prefers.
>>
>> The helper function which build these tables for the GTS did firstbuild
> The helper function which builds these tables for the GTS first builds the "time specific" ..
> 
>> the "time specific" scale arrays for all the times. This is done by
>> calculating the scales based on the integration time specific "total
>> gain" arrays (gain contributed by both the integration time and hw-gain).
>>
>> After this the helper code calculates an array for all available scales.
>> This is done combining all the time specific total-gains into one sorted
>> array, removing dublicate gains and finally converting the gains to
>> scales as above.
>>
>> This can be somewhat simplified by changing the logic for calculating
>> the 'all available scales' -array to directly use the time specific
>> scale arrays instead of time specific total-gain arrays. Code can
>> directly just add all the already computed time specific scales to one
>> big 'all scales'-array, keep it sorted and remove duplicates.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
> Minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>
>> This has been tested by IIO-gts kunit tests only. All testing is
>> appreciated.
>>
>> Comparing the scales is not as pretty as comparing the gains was, as
>> scales are in two ints where the gains were in one. This makes the code
>> slightly more hairy. I however believe that the logic is now more
>> obvious. This might be more important for one reading this later...
>> ---
>>   drivers/iio/industrialio-gts-helper.c | 109 ++++++++++----------------
>>   1 file changed, 42 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>> index 7f900f578f1d..31101848b194 100644
>> --- a/drivers/iio/industrialio-gts-helper.c
>> +++ b/drivers/iio/industrialio-gts-helper.c
>> @@ -191,86 +191,61 @@ static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **sca
>>   	return 0;
>>   }
>>   
>> -static int combine_gain_tables(struct iio_gts *gts, int **gains,
>> -			       int *all_gains, size_t gain_bytes)
>> +static int scale_eq(int *sc1, int *sc2)
>>   {
>> -	int i, new_idx, time_idx;
>> +	return *sc1 == *sc2 && *(sc1 + 1) == *(sc2 + 1);
> 	return sc1[0] == sc2[0] && sc1[1] == sc2[1];
> 
> Would be easier to read in my opinion.

I agree. (As with the other (ptr + 1) cases you commented)

>> +}
>>   
>> -	/*
>> -	 * We assume all the gains for same integration time were unique.
>> -	 * It is likely the first time table had greatest time multiplier as
>> -	 * the times are in the order of preference and greater times are
>> -	 * usually preferred. Hence we start from the last table which is likely
>> -	 * to have the smallest total gains.
>> -	 */
>> -	time_idx = gts->num_itime - 1;
>> -	memcpy(all_gains, gains[time_idx], gain_bytes);
>> -	new_idx = gts->num_hwgain;
>> +static int scale_smaller(int *sc1, int *sc2)
>> +{
>> +	if (*sc1 != *sc2)
>> +		return *sc1 < *sc2;
>> +
>> +	/* If integer parts are equal, fixp parts */
>> +	return *(sc1 + 1) < *(sc2 + 1);
>> +}
>> +
>> +static int do_combined_scaletable(struct iio_gts *gts, int **scales, size_t scale_bytes)
>> +{
>> +	int t_idx, i, new_idx;
>> +	int *all_scales = kcalloc(gts->num_itime, scale_bytes, GFP_KERNEL);
>>   
>> -	while (time_idx-- > 0) {
>> -		for (i = 0; i < gts->num_hwgain; i++) {
>> -			int candidate = gains[time_idx][i];
>> +	if (!all_scales)
>> +		return -ENOMEM;
>> +
>> +	t_idx = gts->num_itime - 1;
>> +	memcpy(all_scales, scales[t_idx], scale_bytes);
>> +	new_idx = gts->num_hwgain * 2;
>> +
>> +	while (t_idx-- > 0) {
> maybe a reverse for loop is clearer
> 
> 	for (tidx = t_idx; tidx; tidx--)
> For me a for loop indicates bounds are known and we change the index
> one per loop.

I could've said that :)

> While loop indicates either unknown bounds, or that we are
> modifying the index other than than in the loop controls.

I'm not entirely sure why I've used while here, could be a result of 
some review discussion.

I'll fix these for the next version.

Yours,
	-- Matti

Re: [PATCH 1/3] iio: gts: Simplify using __free

[Matti Vaittinen]

On 30/11/2024 19:51, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 18:50:24 +0200
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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