linux-kernel - Re: [PATCH v2 1/1] iio: adc: ad4170-4: Correctly update filter

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]

Message-ID: <3rl5w6ydn4pzrflaakx5njft7ojx2anf2c4jpha7zkm2oltuec@4jbc7gis7awr>
Date: Mon, 21 Jul 2025 10:21:51 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	jic23@...nel.org, dan.carpenter@...aro.org, lars@...afoo.de, 
	Michael.Hennerich@...log.com, dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, 
	Markus.Elfring@....de, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 1/1] iio: adc: ad4170-4: Correctly update filter_fs
 after filter type change

On Sun, Jul 20, 2025 at 12:37:24PM -0300, Marcelo Schmitt wrote:
> Previously, the driver was directly using the filter type value to update
> the filter frequency (filter_fs) configuration. That caused the driver to
> switch to the lowest filter_fs configuration (highest sampling frequency)
> on every update to the filter type. Correct the filter_fs collateral update
> by clamping it to the range of supported values instead of mistakenly
> using the filter type to update the filter_fs.
> 
> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> Link: https://lore.kernel.org/linux-iio/c6e54942-5b42-484b-be53-9d4606fd25c4@sabinyo.mountain/
> Suggested-by: Jonathan Cameron <jic23@...nel.org>
> Fixes: 8ab7434734cd ("iio: adc: ad4170-4: Add digital filter and sample frequency config support")
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
> Hi, this is the same fix as the fix provided in implied v1 patch but with the
> difference of doing it the way Markus suggested in reply to v1.
> I have a slight preference for v1 since that one keeps code contained within
> 80 columns. Though, totally fine with whatever version of the fix gets picked up.
> 
> Change log v1 (implied) -> v2
> - Replaces if by use of ternary operator as suggested by Markus in reply to v1.
> - Fixed commit log typo: colateral -> collateral
> - Fixed commit log typo: clampling -> clamping
> 
> Thanks,
> Marcelo
> 
>  drivers/iio/adc/ad4170-4.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
> index 6cd84d6fb08b..2e61f9a9a1ef 100644
> --- a/drivers/iio/adc/ad4170-4.c
> +++ b/drivers/iio/adc/ad4170-4.c
> @@ -879,12 +879,11 @@ static int ad4170_set_filter_type(struct iio_dev *indio_dev,
>  		if (!iio_device_claim_direct(indio_dev))
>  			return -EBUSY;
>  
> -		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
> -			setup->filter_fs = clamp(val, AD4170_SINC3_MIN_FS,
> -						 AD4170_SINC3_MAX_FS);
> -		else
> -			setup->filter_fs = clamp(val, AD4170_SINC5_MIN_FS,
> -						 AD4170_SINC5_MAX_FS);
> +		setup->filter_fs = (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
> +				    ? clamp(setup->filter_fs,
> +					    AD4170_SINC3_MIN_FS, AD4170_SINC3_MAX_FS)
> +				    : clamp(setup->filter_fs,
> +					    AD4170_SINC5_MIN_FS, AD4170_SINC5_MAX_FS);

I very much prefer the approach in v1. To me, this code is just harder
to read...

Reminder to why is a good idea to wait a bit and don't rush into
spinning new versions. Also, Markus has a very proven record of not
being helpful at all in reviews (just look in lore :))

With that:

Reviewed-by: Nuno Sá <nuno.sa@...log.com>

- Nuno Sá

>  
>  		setup->filter &= ~AD4170_FILTER_FILTER_TYPE_MSK;
>  		setup->filter |= FIELD_PREP(AD4170_FILTER_FILTER_TYPE_MSK,
> 
> base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
> -- 
> 2.47.2
>