Message-ID: <20250713155839.07476235@jic23-huawei>
Date: Sun, 13 Jul 2025 15:58:39 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: ad7173: prevent scan if too many setups
 requested

On Wed, 09 Jul 2025 11:35:52 -0500
David Lechner <dlechner@...libre.com> wrote:

> Add a check to ad7173_update_scan_mode() to ensure that we didn't exceed
> the maximum number of unique channel configurations.
> 
> In the AD7173 family of chips, there are some chips that have 16
> CHANNELx registers but only 8 setups (combination of CONFIGx, FILTERx,
> GAINx and OFFSETx registers). Since commit 2233378a8c60 ("iio: adc:
> ad7173: fix num_slots"), it is possible to have more than 8 channels
> enabled in a scan at the same time, so it is possible to get a bad
> configuration where more than 8 channels are using unique configurations.
> This happens because the algorithm to allocate the setup slots only
> takes into account which slot has been least recently used and doesn't
> know about the maximum number of slots available.
> 
> Since the algorithm to allocate the setup slots is quite complex, it is
> simpler to check after the fact if the current state is valid or not.
> So this patch adds a check in ad7173_update_scan_mode() after setting up
> all of the configurations to make sure that the actual setup still
> matches the requested setup for each enabled channel. If not, we prevent
> the scan from being enabled and return an error.
> 
> The setup comparison is ad7173_setup_equal() is refactored to a separate
> function since we need to call it in two places now.
> 
> Fixes: 2233378a8c60 ("iio: adc: ad7173: fix num_slots")
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> I know this isn't pretty, but after puzzling over it for a day, this was
> the best I could come up with that didn't involve a complete rewrite of
> the setup allocation algorithm.
> 
> I don't really understand why we care about which setup was the least
> recently used - it isn't like we are going to wear out one setup by
> using it too much. 
> Maybe it was just to reduce the number of SPI xfers?

Been a while, so I may be remembering the intent here wrong.
The challenge of these allocators is exactly what you have called out.
How do we cope if too many configs are needed to deliver the mix of
channel configs requested.  I think the LRU thing was an attempt to
reduce the amount of reconfiguring needed.  That's mostly relevant of
single channel reads I think...


> Anyway, ad7124 has a similar setup allocation algorithm, so it could be
> nice to eventually replace both of these with something common that is
> a bit simpler, e.g. always use SETUP 0 for single transfers and allocate
> the rest of the setups in order for buffered reads with more than one
> channel enabled.

So don't use setup 0 for buffered reads?  That sounds odd.

> And just always re-write the setup each time so we
> don't have to try to keep track of what each slot is programmed with.

Fair enough as a simplification.

If you've stopped using the lru, why are things like the _lru() functions still used?

> 
> Also, the commit hash this references is currently in iio/fixes-togreg,
> so if that gets rebased, it will need to be updated here.
> ---
>  drivers/iio/adc/ad7173.c | 87 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 75 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index c41bc5b9ac597f57eea6a097cc3a118de7b42210..d1d6c20fb1ee3f8479e8faa2209206e208adb90a 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -200,7 +200,7 @@ struct ad7173_channel_config {
>  	/*
>  	 * Following fields are used to compare equality. If you
>  	 * make adaptations in it, you most likely also have to adapt
> -	 * ad7173_find_live_config(), too.
> +	 * ad7173_setup_equal(), too.
>  	 */
>  	struct_group(config_props,
>  		bool bipolar;
> @@ -563,12 +563,19 @@ static void ad7173_reset_usage_cnts(struct ad7173_state *st)
>  	st->config_usage_counter = 0;
>  }
>  

> +static struct ad7173_channel_config *
> +ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
> +{
> +	struct ad7173_channel_config *cfg_aux;
> +	int i;
> +
>  	for (i = 0; i < st->num_channels; i++) {
>  		cfg_aux = &st->channels[i].cfg;
>  
> -		if (cfg_aux->live &&
> -		    cfg->bipolar == cfg_aux->bipolar &&
> -		    cfg->input_buf == cfg_aux->input_buf &&
> -		    cfg->odr == cfg_aux->odr &&
> -		    cfg->ref_sel == cfg_aux->ref_sel)
> +		if (cfg_aux->live && ad7173_setup_equal(cfg, cfg_aux))
>  			return cfg_aux;
>  	}
>  	return NULL;
> @@ -1230,7 +1245,7 @@ static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
>  				   const unsigned long *scan_mask)
>  {
>  	struct ad7173_state *st = iio_priv(indio_dev);
> -	int i, ret;
> +	int i, j, k, ret;
>  
>  	for (i = 0; i < indio_dev->num_channels; i++) {
>  		if (test_bit(i, scan_mask))
> @@ -1241,6 +1256,54 @@ static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
>  			return ret;
>  	}
>  
> +	/*
> +	 * On some chips, there are more channels that setups, so if there were
> +	 * more unique setups requested than the number of available slots,
> +	 * ad7173_set_channel() will have written over some of the slots. We
> +	 * can detect this by making sure each assigned cfg_slot matches the
> +	 * requested configuration. If it doesn't, we know that the slot was
> +	 * overwritten by a different channel.
> +	 */
> +	for_each_set_bit(i, scan_mask, indio_dev->num_channels) {
> +		const struct ad7173_channel_config *cfg1, *cfg2;
> +
> +		cfg1 = &st->channels[i].cfg;
> +
> +		for_each_set_bit(j, scan_mask, indio_dev->num_channels) {
> +			cfg2 = &st->channels[j].cfg;
> +
> +			/*
> +			 * Only compare configs that are assigned to the same
> +			 * SETUP_SEL slot and don't compare channel to itself.
> +			 */
> +			if (i == j || cfg1->cfg_slot != cfg2->cfg_slot)
> +				continue;
> +
> +			/*
> +			 * If we find two different configs trying to use the
> +			 * same SETUP_SEL slot, then we know that the that we
> +			 * have too many unique configurations requested for
> +			 * the available slots and at least one was overwritten.
> +			 */
> +			if (!ad7173_setup_equal(cfg1, cfg2)) {
> +				/*
> +				 * At this point, there isn't a way to tell
> +				 * which setups are actually programmed in the
> +				 * ADC anymore, so we could read them back to
> +				 * see, but it is simpler to just turn off all
> +				 * of the live flags so that everything gets
> +				 * reprogramed on the next attempt read a sample.
> +				 */
> +				for (k = 0; k < st->num_channels; k++)
> +					st->channels[k].cfg.live = false;
> +
> +				dev_err(&st->sd.spi->dev,
> +					"Too many unique channel configurations requested for scan\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> 
> ---
> base-commit: 2233378a8c606f7f6893d4c16aa6eb6fea027a52
> change-id: 20250709-iio-adc-ad7173-fix-setup-use-limits-0a5d2b6a6780
> 
> Best regards,

Powered by blists - more mailing lists