linux-kernel - Re: [PATCH] iio: test: fixed-point: new kunit test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <166d0753-413f-4f83-892c-2e799c0c9f15@gmail.com>
Date: Tue, 14 Oct 2025 09:43:54 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: David Lechner <dlechner@...libre.com>, Jonathan Cameron
 <jic23@...nel.org>, Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH] iio: test: fixed-point: new kunit test

On 14/10/2025 00:33, David Lechner wrote:
> Add a kunit test for iio_str_to_fixpoint(). This function has an
> unintuitive API so this is helpful to see how to use it and shows the
> various edge cases.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> When reviewing [1], I noticed that iio_str_to_fixpoint() has an odd API
> which lead me to find the bug in [2]. To make sure I was understanding
> the API correctly, I wrote a KUnit test for it. So here it is.
> 
> [1]: https://lore.kernel.org/linux-iio/20251009173609.992452-3-flavra@baylibre.com/
> [2]: https://lore.kernel.org/linux-iio/20251010-iio-adc-ad7280a-fix-ad7280_store_balance_timer-v1-1-e11746735192@baylibre.com/
> ---
> Discussion unrelated to the patch:
> 
> I'm also a little tempted to introduce a new function that is a bit
> easier to use. Many callers of iio_str_to_fixpoint_s64() are doing
> something like int_part * 1000 + fract_part and ignoring the possibility
> of negative values which require special handling.
> 
> static int iio_str_to_fixpoint_s64(const char *str, u32 decimal_places, s64 *value)
> {
> 	int int_part, fract_part;
> 	int ret;
> 
> 	ret = iio_str_to_fixpoint(str, int_pow(10, decimal_places - 1),
> 				  &int_part, &fract_part);
> 	if (ret)
> 		return ret;
> 
> 	*value = (s64)int_part * int_pow(10, decimal_places) +
> 		 (int_part < 0 ? -1 : 1) * fract_part;
> 
> 	return 0;
> }

FWIW: I like this.

> ---
>   drivers/iio/test/Kconfig                | 13 ++++++++
>   drivers/iio/test/Makefile               |  1 +
>   drivers/iio/test/iio-test-fixed-point.c | 58 +++++++++++++++++++++++++++++++++
>   3 files changed, 72 insertions(+)
> 
> diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
> index 6e65e929791ca247df9ac993fddbb4da557d5dfa..d210067ea59199d342b15bf373e724d0aa2c55a0 100644
> --- a/drivers/iio/test/Kconfig
> +++ b/drivers/iio/test/Kconfig
> @@ -4,6 +4,19 @@

// snip

> +static void iio_test_iio_str_to_fixed_point(struct kunit *test)
> +{
> +	int int_part, fract_part;
> +	int ret;
> +
> +	/* Positive value > 1 */
> +	ret = iio_str_to_fixpoint("1.234", 100, &int_part, &fract_part);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	KUNIT_EXPECT_EQ(test, int_part * 1000 + fract_part, 1234);

Do you think doing two checks:
	KUNIT_EXPECT_EQ(test, int_part, 1);
	KUNIT_EXPECT_EQ(test, fract_part, 234);

instead of the:
	KUNIT_EXPECT_EQ(test, int_part * 1000 + fract_part, 1234);

would work? For me seeing it all ass
	ret = iio_str_to_fixpoint("1.234", 100, &int_part, &fract_part);
	KUNIT_EXPECT_EQ(test, 0, ret);
	KUNIT_EXPECT_EQ(test, int_part, 1);
	KUNIT_EXPECT_EQ(test, fract_part, 234);

would be super clear - albeit also a line longer :)
(Same applies to the rest of the cases).

Well, it's not critical, so with or without that change:

Reviewed-by: Matti Vaittinen <mazziesaccount@...il.com>

Thanks!

Yours,
	-- Matti