[PATCH v2] iio: adc: ad7173: prevent scan if too many setups requested

[PATCH v2] iio: adc: ad7173: prevent scan if too many setups requested

[David Lechner]

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 92c247216918 ("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 when 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 in ad7173_setup_equal() is refactored to a separate
function since we need to call it in two places now.

Fixes: 92c247216918 ("iio: adc: ad7173: fix num_slots")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
My musings in v1 might have confused things a bit, but my intention is
to have this patch fix things well enough for backporting stable kernels
and then perhaps look at a better way of handling the setup slots in the
future.
---
Changes in v2:
- Updated the commit hash of the commit being fixed since it was rebased
  before being sent to Greg.
- Fixed a few typos in the commit message.
- Link to v1: https://lore.kernel.org/r/20250709-iio-adc-ad7173-fix-setup-use-limits-v1-1-e41233029d44@baylibre.com
---
 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 4413207be28f60a4d9529e3522f5d2fd6276bcc2..131cd1cf8a23c57dfeb156a96864e6cf2a2b6bf9 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;
@@ -561,12 +561,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)
+/**
+ * ad7173_setup_equal - Compare two channel setups
+ * @cfg1: First channel configuration
+ * @cfg2: Second channel configuration
+ *
+ * Compares all configuration options that affect the registers connected to
+ * SETUP_SEL, namely CONFIGx, FILTERx, GAINx and OFFSETx.
+ *
+ * Returns: true if the setups are identical, false otherwise
+ */
+static bool ad7173_setup_equal(const struct ad7173_channel_config *cfg1,
+			       const struct ad7173_channel_config *cfg2)
 {
-	struct ad7173_channel_config *cfg_aux;
-	int i;
-
 	/*
 	 * This is just to make sure that the comparison is adapted after
 	 * struct ad7173_channel_config was changed.
@@ -579,14 +586,22 @@ ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *c
 				     u8 ref_sel;
 			     }));
 
+	return cfg1->bipolar == cfg2->bipolar &&
+	       cfg1->input_buf == cfg2->input_buf &&
+	       cfg1->odr == cfg2->odr &&
+	       cfg1->ref_sel == cfg2->ref_sel;
+}
+
+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;
@@ -1228,7 +1243,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))
@@ -1239,6 +1254,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: 0a686b9c4f847dc21346df8e56d5b119918fefef
change-id: 20250709-iio-adc-ad7173-fix-setup-use-limits-0a5d2b6a6780

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH v2] iio: adc: ad7173: prevent scan if too many setups requested

[Andy Shevchenko]

On Tue, Jul 22, 2025 at 02:20:07PM -0500, David Lechner 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 92c247216918 ("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 when 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 in ad7173_setup_equal() is refactored to a separate
> function since we need to call it in two places now.

...

> + * ad7173_setup_equal - Compare two channel setups

Better naming is
ad7173_is_setup_equal().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: adc: ad7173: prevent scan if too many setups requested

[Jonathan Cameron]

On Wed, 23 Jul 2025 17:34:42 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Jul 22, 2025 at 02:20:07PM -0500, David Lechner 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 92c247216918 ("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 when 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 in ad7173_setup_equal() is refactored to a separate
> > function since we need to call it in two places now.  
> 
> ...
> 
> > + * ad7173_setup_equal - Compare two channel setups  
> 
> Better naming is
> ad7173_is_setup_equal().
> 
Agree.  Tweaked with following and applied to the fixes-togreg-for-6.17
branch of iio.git + marked for stable.

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 131cd1cf8a23..683146e83ab2 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_setup_equal(), too.
+        * ad7173_is_setup_equal(), too.
         */
        struct_group(config_props,
                bool bipolar;
@@ -562,7 +562,7 @@ static void ad7173_reset_usage_cnts(struct ad7173_state *st)
 }
 
 /**
- * ad7173_setup_equal - Compare two channel setups
+ * ad7173_is_setup_equal - Compare two channel setups
  * @cfg1: First channel configuration
  * @cfg2: Second channel configuration
  *
@@ -571,8 +571,8 @@ static void ad7173_reset_usage_cnts(struct ad7173_state *st)
  *
  * Returns: true if the setups are identical, false otherwise
  */
-static bool ad7173_setup_equal(const struct ad7173_channel_config *cfg1,
-                              const struct ad7173_channel_config *cfg2)
+static bool ad7173_is_setup_equal(const struct ad7173_channel_config *cfg1,
+                                 const struct ad7173_channel_config *cfg2)
 {
        /*
         * This is just to make sure that the comparison is adapted after
@@ -601,7 +601,7 @@ ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *c
        for (i = 0; i < st->num_channels; i++) {
                cfg_aux = &st->channels[i].cfg;
 
-               if (cfg_aux->live && ad7173_setup_equal(cfg, cfg_aux))
+               if (cfg_aux->live && ad7173_is_setup_equal(cfg, cfg_aux))
                        return cfg_aux;
        }
        return NULL;
@@ -1283,7 +1283,7 @@ static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
                         * have too many unique configurations requested for
                         * the available slots and at least one was overwritten.
                         */
-                       if (!ad7173_setup_equal(cfg1, cfg2)) {
+                       if (!ad7173_is_setup_equal(cfg1, cfg2)) {
                                /*
                                 * At this point, there isn't a way to tell
                                 * which setups are actually programmed in the