linux-kernel - Re: [PATCH v3 05/10] iio: adc: ad4170: Add digital filter and sample frequency config support

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: <20250525114119.655e24ec@jic23-huawei>
Date: Sun, 25 May 2025 11:41:19 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <lars@...afoo.de>, <Michael.Hennerich@...log.com>, <dlechner@...libre.com>,
 <nuno.sa@...log.com>, <andy@...nel.org>, <robh@...nel.org>,
 <krzk+dt@...nel.org>, <conor+dt@...nel.org>, <linus.walleij@...aro.org>,
 <brgl@...ev.pl>, <marcelo.schmitt1@...il.com>
Subject: Re: [PATCH v3 05/10] iio: adc: ad4170: Add digital filter and
 sample frequency config support

On Tue, 13 May 2025 09:35:03 -0300
Marcelo Schmitt <marcelo.schmitt@...log.com> wrote:

> Add support for sinc3, sinc5, and averaged sinc5 digital filters along with
> sample frequency configuration.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>

> +static int ad4170_set_filter_type(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  unsigned int val)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> +	struct ad4170_setup *setup = &chan_info->setup;
> +	unsigned int old_filter_fs, old_filter, filter_type_conf;
> +	int ret;
> +
> +	switch (val) {
> +	case AD4170_SINC5_AVG:
> +		filter_type_conf = AD4170_FILTER_FILTER_TYPE_SINC5_AVG;
> +		break;
> +	case AD4170_SINC5:
> +		filter_type_conf = AD4170_FILTER_FILTER_TYPE_SINC5;
> +		break;
> +	case AD4170_SINC3:
> +		filter_type_conf = AD4170_FILTER_FILTER_TYPE_SINC3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	guard(mutex)(&st->lock);

You need to scope this because otherwise locks are released in a different
order to how they are taken.

	iio_device_claim_direct (takes mlock in the core)
	guard(mutex)(&st->lock)
	iio_device_release_direct (releases mlock in the core.

	... scope finishes
	guard(mutex)(&st->lock) cleanup happens.


Current code might be fine, but it's harder to reason about.

> +	/*
> +	 * The filters provide the same ODR for a given filter_fs value but
> +	 * there are different minimum and maximum filter_fs limits for each
> +	 * filter. The filter_fs value will be adjusted if the current filter_fs
> +	 * is out of the limits of the just requested filter. Since the
> +	 * filter_fs value affects the ODR (sampling_frequency), changing the
> +	 * filter may lead to a change in the sampling frequency.
> +	 */
> +	old_filter = setup->filter;
> +	old_filter_fs = setup->filter_fs;
> +	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 &= ~AD4170_FILTER_FILTER_TYPE_MSK;
> +	setup->filter |= FIELD_PREP(AD4170_FILTER_FILTER_TYPE_MSK,
> +				    filter_type_conf);
> +
> +	ret = ad4170_write_channel_setup(st, chan->address, false);
> +	if (ret) {
> +		setup->filter = old_filter;
> +		setup->filter_fs = old_filter_fs;
> +	}
> +
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}