linux-kernel - Re: [PATCH v14 06/11] iio: afe: rescale: make use of units.h

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Feb 2022 18:41:41 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Liam Beguin <liambeguin@...il.com>, jic23@...nel.org,
        andy.shevchenko@...il.com, lars@...afoo.de
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, robh+dt@...nel.org
Subject: Re: [PATCH v14 06/11] iio: afe: rescale: make use of units.h

Hi!

On 2022-02-08 03:04, Liam Beguin wrote:
> Make use of well-defined SI metric prefixes to improve code readability.
> 
> Signed-off-by: Liam Beguin <liambeguin@...il.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 67273de46843..4601f84a83c2 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -51,11 +51,16 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>  		}
>  		fallthrough;
>  	case IIO_VAL_FRACTIONAL_LOG2:
> -		tmp = (s64)*val * 1000000000LL;
> +		/*
> +		 * GIGA is used here as an arbitrarily large multiplier to avoid

s/arbitrarily/arbitrary/, however...

> +		 * precision loss in the division. It doesn't have any physical
> +		 * meaning attached to it.

... 1000000000 is NOT arbitrary. Before patch 4/11 that was true, but
with that patch the multiplier MUST match the multiplier used below
when calculating with the decimals for the IIO_VAL_INT_PLUS_NANO
value. Again, the connection with that name makes me think that NANO
is just so much better that GIGA. Still fine with raw numbers of
course.

So, the comment is actively misleading. How about the following?

/*
 * We need to multiply by something large to avoid loss of
 * precision, and NANO fits that while at the same time
 * being compatible with the conversion to
 * IIO_VAL_INT_PLUS_NANO for cases where that maintains even
 * more precision.
 */

> +		 */
> +		tmp = (s64)*val * GIGA;
>  		tmp = div_s64(tmp, rescale->denominator);
>  		tmp *= rescale->numerator;
>  
> -		tmp = div_s64_rem(tmp, 1000000000LL, &rem);
> +		tmp = div_s64_rem(tmp, GIGA, &rem);
>  		*val = tmp;
>  
>  		if (!rem)
> @@ -71,7 +76,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>  
>  		*val2 = rem / (int)tmp;
>  		if (rem2)
> -			*val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
> +			*val2 += div_s64((s64)rem2 * GIGA, tmp);

NANO again, because IIO_VAL_INT_PLUS_NANO.

>  
>  		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_VAL_INT_PLUS_NANO:
> @@ -332,8 +337,8 @@ static int rescale_current_sense_amplifier_props(struct device *dev,
>  	 * gain_div / (gain_mult * sense), while trying to keep the
>  	 * numerator/denominator from overflowing.
>  	 */
> -	factor = gcd(sense, 1000000);
> -	rescale->numerator = 1000000 / factor;
> +	factor = gcd(sense, MEGA);
> +	rescale->numerator = MEGA / factor;

Here, we actually have a connection with prefix of a unit of a
physical quantity. microohms. If anything, why not MICRO?

>  	rescale->denominator = sense / factor;
>  
>  	factor = gcd(rescale->numerator, gain_mult);
> @@ -361,8 +366,8 @@ static int rescale_current_sense_shunt_props(struct device *dev,
>  		return ret;
>  	}
>  
> -	factor = gcd(shunt, 1000000);
> -	rescale->numerator = 1000000 / factor;
> +	factor = gcd(shunt, MEGA);
> +	rescale->numerator = MEGA / factor;

MICRO again, bacause microohms.

>  	rescale->denominator = shunt / factor;
>  
>  	return 0;

Cheers,
Peter