[PATCH v2 0/3] iio: adc: ad7173: fix non-const info struct

[PATCH v2 0/3] iio: adc: ad7173: fix non-const info struct

[David Lechner]

While working ad7124, Uwe pointed out a bug in the ad7173 driver.
static struct ad_sigma_delta_info ad7173_sigma_delta_info was not const
and was being modified during driver probe, which could lead to race
conditions if two instances of the driver were probed at the same time.

The actual fix part is fairly trivial but I have only compile tested it.
Guillaume has access to ad4111 hardware, so it would be good to get a
Tested-by from him to make sure this doesn't break anything.

---
Changes in v2:
- Fixed chip name in a few places.
- Add new simpler patch for "fix" that gets backported.
- Rebase other patches on this and incorporate feedback.
- Link to v1: https://lore.kernel.org/r/20241122-iio-adc-ad7313-fix-non-const-info-struct-v1-0-d05c02324b73@baylibre.com

---
David Lechner (3):
      iio: adc: ad7173: fix using shared static info struct
      iio: adc: ad7173: remove special handling for irq number
      iio: adc: ad7173: don't make copy of ad_sigma_delta_info struct

 drivers/iio/adc/ad7173.c               | 474 +++++++++++++++++----------------
 drivers/iio/adc/ad_sigma_delta.c       |   5 +-
 include/linux/iio/adc/ad_sigma_delta.h |   2 -
 3 files changed, 249 insertions(+), 232 deletions(-)
---
base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
change-id: 20241122-iio-adc-ad7313-fix-non-const-info-struct-92e59b91ee2e

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

[PATCH v2 1/3] iio: adc: ad7173: fix using shared static info struct

[David Lechner]

Fix a possible race condition during driver probe in the ad7173 driver
due to using a shared static info struct. If more that one instance of
the driver is probed at the same time, some of the info could be
overwritten by the other instance, leading to incorrect operation.

To fix this, make the static info struct const so that it is read-only
and make a copy of the info struct for each instance of the driver that
can be modified.

Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Fixes: 76a1e6a42802 ("iio: adc: ad7173: add AD7173 driver")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7173.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 29ff9c7036c0..c83c4c90b090 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -199,6 +199,7 @@ struct ad7173_channel {
 
 struct ad7173_state {
 	struct ad_sigma_delta sd;
+	struct ad_sigma_delta_info sigma_delta_info;
 	const struct ad7173_device_info *info;
 	struct ad7173_channel *channels;
 	struct regulator_bulk_data regulators[3];
@@ -752,7 +753,7 @@ static int ad7173_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
 	return ad_sd_write_reg(sd, AD7173_REG_CH(chan), 2, 0);
 }
 
-static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
+static const struct ad_sigma_delta_info ad7173_sigma_delta_info = {
 	.set_channel = ad7173_set_channel,
 	.append_status = ad7173_append_status,
 	.disable_all = ad7173_disable_all,
@@ -1401,7 +1402,7 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Interrupt 'rdy' is required\n");
 
-	ad7173_sigma_delta_info.irq_line = ret;
+	st->sigma_delta_info.irq_line = ret;
 
 	return ad7173_fw_parse_channel_config(indio_dev);
 }
@@ -1434,8 +1435,9 @@ static int ad7173_probe(struct spi_device *spi)
 	spi->mode = SPI_MODE_3;
 	spi_setup(spi);
 
-	ad7173_sigma_delta_info.num_slots = st->info->num_configs;
-	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
+	st->sigma_delta_info = ad7173_sigma_delta_info;
+	st->sigma_delta_info.num_slots = st->info->num_configs;
+	ret = ad_sd_init(&st->sd, indio_dev, spi, &st->sigma_delta_info);
 	if (ret)
 		return ret;
 

-- 
2.43.0

[PATCH v2 2/3] iio: adc: ad7173: remove special handling for irq number

[David Lechner]

Remove the int irq_line field in struct ad_sigma_delta_info and all code
that referenced it.

This struct is intended to be used as static const data. Currently, the
only user that doesn't uses the static const struct directly, namely the
ad7173 driver is making a copy of this struct to be able to modify the
irq_line field. However, this field is written and never used due to the
fact that ad_sd_init() which reads the field is called before
ad7173_fw_parse_device_config() which writes it.

The runtime behavior does not change since ad_sd_init() was already
(unintentionally) being called with irq_line = 0.  But, even though
this could be considered a bug, the behavior was still correct. The SPI
subsystem always uses the first interrupt in the interrupts array from
the devicetree and the devicetree bindings for this family of chips
specify that the RDY interrupt is always the first interrupt. Therefore,
we don't actually need the special call to fwnode_irq_get_byname(), so
it is removed in this patch instead of moving it to the correct place.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v2 changes:
* Fixed chip name is subject line
* Uwe's comment made me realize that the special case was actually never
being used because of the ordering bug and could safely be removed
rather than trying to preserve it.
---
 drivers/iio/adc/ad7173.c               | 8 +-------
 drivers/iio/adc/ad_sigma_delta.c       | 5 +----
 include/linux/iio/adc/ad_sigma_delta.h | 2 --
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index c83c4c90b090..52fce43e4ce1 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -199,7 +199,7 @@ struct ad7173_channel {
 
 struct ad7173_state {
 	struct ad_sigma_delta sd;
-	struct ad_sigma_delta_info sigma_delta_info;
+	struct ad_sigma_delta_chip_info sigma_delta_info;
 	const struct ad7173_device_info *info;
 	struct ad7173_channel *channels;
 	struct regulator_bulk_data regulators[3];
@@ -1398,12 +1398,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 			return ret;
 	}
 
-	ret = fwnode_irq_get_byname(dev_fwnode(dev), "rdy");
-	if (ret < 0)
-		return dev_err_probe(dev, ret, "Interrupt 'rdy' is required\n");
-
-	st->sigma_delta_info.irq_line = ret;
-
 	return ad7173_fw_parse_channel_config(indio_dev);
 }
 
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 2f3b61765055..38c48743712e 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -674,10 +674,7 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 		}
 	}
 
-	if (info->irq_line)
-		sigma_delta->irq_line = info->irq_line;
-	else
-		sigma_delta->irq_line = spi->irq;
+	sigma_delta->irq_line = spi->irq;
 
 	iio_device_set_drvdata(indio_dev, sigma_delta);
 
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index f8c1d2505940..44f487323a43 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -52,7 +52,6 @@ struct iio_dev;
  *   be used.
  * @irq_flags: flags for the interrupt used by the triggered buffer
  * @num_slots: Number of sequencer slots
- * @irq_line: IRQ for reading conversions. If 0, spi->irq will be used
  */
 struct ad_sigma_delta_info {
 	int (*set_channel)(struct ad_sigma_delta *, unsigned int channel);
@@ -68,7 +67,6 @@ struct ad_sigma_delta_info {
 	unsigned int data_reg;
 	unsigned long irq_flags;
 	unsigned int num_slots;
-	int irq_line;
 };
 
 /**

-- 
2.43.0

Re: [PATCH v2 2/3] iio: adc: ad7173: remove special handling for irq number

[Andy Shevchenko]

On Wed, Nov 27, 2024 at 10:02 PM David Lechner <dlechner@baylibre.com> wrote:
>
> Remove the int irq_line field in struct ad_sigma_delta_info and all code
> that referenced it.
>
> This struct is intended to be used as static const data. Currently, the
> only user that doesn't uses the static const struct directly, namely the
> ad7173 driver is making a copy of this struct to be able to modify the
> irq_line field. However, this field is written and never used due to the
> fact that ad_sd_init() which reads the field is called before
> ad7173_fw_parse_device_config() which writes it.
>
> The runtime behavior does not change since ad_sd_init() was already
> (unintentionally) being called with irq_line = 0.  But, even though
> this could be considered a bug, the behavior was still correct. The SPI
> subsystem always uses the first interrupt in the interrupts array from
> the devicetree and the devicetree bindings for this family of chips
> specify that the RDY interrupt is always the first interrupt. Therefore,
> we don't actually need the special call to fwnode_irq_get_byname(), so
> it is removed in this patch instead of moving it to the correct place.

...

>  struct ad7173_state {
>         struct ad_sigma_delta sd;
> -       struct ad_sigma_delta_info sigma_delta_info;
> +       struct ad_sigma_delta_chip_info sigma_delta_info;
>         const struct ad7173_device_info *info;
>         struct ad7173_channel *channels;
>         struct regulator_bulk_data regulators[3];

Has this patch been compile-tested? Because I don't understand this
change and how it's going to be compiled.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] iio: adc: ad7173: remove special handling for irq number

[David Lechner]

On 11/28/24 12:47 AM, Andy Shevchenko wrote:
> On Wed, Nov 27, 2024 at 10:02 PM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Remove the int irq_line field in struct ad_sigma_delta_info and all code
>> that referenced it.
>>
>> This struct is intended to be used as static const data. Currently, the
>> only user that doesn't uses the static const struct directly, namely the
>> ad7173 driver is making a copy of this struct to be able to modify the
>> irq_line field. However, this field is written and never used due to the
>> fact that ad_sd_init() which reads the field is called before
>> ad7173_fw_parse_device_config() which writes it.
>>
>> The runtime behavior does not change since ad_sd_init() was already
>> (unintentionally) being called with irq_line = 0.  But, even though
>> this could be considered a bug, the behavior was still correct. The SPI
>> subsystem always uses the first interrupt in the interrupts array from
>> the devicetree and the devicetree bindings for this family of chips
>> specify that the RDY interrupt is always the first interrupt. Therefore,
>> we don't actually need the special call to fwnode_irq_get_byname(), so
>> it is removed in this patch instead of moving it to the correct place.
> 
> ...
> 
>>  struct ad7173_state {
>>         struct ad_sigma_delta sd;
>> -       struct ad_sigma_delta_info sigma_delta_info;
>> +       struct ad_sigma_delta_chip_info sigma_delta_info;
>>         const struct ad7173_device_info *info;
>>         struct ad7173_channel *channels;
>>         struct regulator_bulk_data regulators[3];
> 
> Has this patch been compile-tested? Because I don't understand this
> change and how it's going to be compiled.
> 

I did compile test each commit, but it looks like I might have
squashed a fix into the wrong patch. :-(

[PATCH v2 3/3] iio: adc: ad7173: don't make copy of ad_sigma_delta_info struct

[David Lechner]

Use two separate static const struct ad_sigma_delta_info instances
instead of making a copy for each driver instance.

Typically in the IIO subsystem, we use multiple static const instances
of the same struct when there are different variants of the same family
of devices as opposed to making a copy for each driver instance and
modifying it.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

Whether this patch is an improvement or not is debatable. It makes
things a bit more verbose, but to me it seems more consistent with
how such cases are handled in the IIO subsystem. So take this one or
leave it.
---
 drivers/iio/adc/ad7173.c | 470 +++++++++++++++++++++++++----------------------
 1 file changed, 248 insertions(+), 222 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 52fce43e4ce1..04533b37d9ec 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -166,6 +166,7 @@ struct ad7173_device_info {
 	unsigned int clock;
 	unsigned int id;
 	char *name;
+	const struct ad_sigma_delta_info *sd_info;
 	bool has_current_inputs;
 	bool has_vincom_input;
 	bool has_temp;
@@ -199,7 +200,6 @@ struct ad7173_channel {
 
 struct ad7173_state {
 	struct ad_sigma_delta sd;
-	struct ad_sigma_delta_chip_info sigma_delta_info;
 	const struct ad7173_device_info *info;
 	struct ad7173_channel *channels;
 	struct regulator_bulk_data regulators[3];
@@ -258,223 +258,6 @@ static unsigned int ad4111_current_channel_config[] = {
 	0x18B, /* 12:IIN3+   11:IIN3− */
 };
 
-static const struct ad7173_device_info ad4111_device_info = {
-	.name = "ad4111",
-	.id = AD4111_ID,
-	.num_voltage_in_div = 8,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_voltage_in = 8,
-	.num_gpios = 2,
-	.higher_gpio_bits = true,
-	.has_temp = true,
-	.has_vincom_input = true,
-	.has_input_buf = true,
-	.has_current_inputs = true,
-	.has_int_ref = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad4112_device_info = {
-	.name = "ad4112",
-	.id = AD4112_ID,
-	.num_voltage_in_div = 8,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_voltage_in = 8,
-	.num_gpios = 2,
-	.higher_gpio_bits = true,
-	.has_vincom_input = true,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_current_inputs = true,
-	.has_int_ref = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad4113_device_info = {
-	.name = "ad4113",
-	.id = AD4113_ID,
-	.num_voltage_in_div = 8,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_voltage_in = 8,
-	.num_gpios = 2,
-	.data_reg_only_16bit = true,
-	.higher_gpio_bits = true,
-	.has_vincom_input = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad4114_device_info = {
-	.name = "ad4114",
-	.id = AD4114_ID,
-	.num_voltage_in_div = 16,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_voltage_in = 16,
-	.num_gpios = 4,
-	.has_vincom_input = true,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad4115_device_info = {
-	.name = "ad4115",
-	.id = AD4115_ID,
-	.num_voltage_in_div = 16,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_voltage_in = 16,
-	.num_gpios = 4,
-	.has_vincom_input = true,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.clock = 8 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad4115_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad4116_device_info = {
-	.name = "ad4116",
-	.id = AD4116_ID,
-	.num_voltage_in_div = 11,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_voltage_in = 16,
-	.num_gpios = 4,
-	.has_vincom_input = true,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.clock = 4 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad4116_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7172_2_device_info = {
-	.name = "ad7172-2",
-	.id = AD7172_2_ID,
-	.num_voltage_in = 5,
-	.num_channels = 4,
-	.num_configs = 4,
-	.num_gpios = 2,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.has_pow_supply_monitoring = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7172_4_device_info = {
-	.name = "ad7172-4",
-	.id = AD7172_4_ID,
-	.num_voltage_in = 9,
-	.num_channels = 8,
-	.num_configs = 8,
-	.num_gpios = 4,
-	.has_input_buf = true,
-	.has_ref2 = true,
-	.has_pow_supply_monitoring = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7173_8_device_info = {
-	.name = "ad7173-8",
-	.id = AD7173_ID,
-	.num_voltage_in = 17,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_gpios = 4,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.has_ref2 = true,
-	.clock = 2 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7173_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7175_2_device_info = {
-	.name = "ad7175-2",
-	.id = AD7175_2_ID,
-	.num_voltage_in = 5,
-	.num_channels = 4,
-	.num_configs = 4,
-	.num_gpios = 2,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.has_pow_supply_monitoring = true,
-	.clock = 16 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7175_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7175_8_device_info = {
-	.name = "ad7175-8",
-	.id = AD7175_8_ID,
-	.num_voltage_in = 17,
-	.num_channels = 16,
-	.num_configs = 8,
-	.num_gpios = 4,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.has_ref2 = true,
-	.has_pow_supply_monitoring = true,
-	.clock = 16 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7175_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7176_2_device_info = {
-	.name = "ad7176-2",
-	.id = AD7176_ID,
-	.num_voltage_in = 5,
-	.num_channels = 4,
-	.num_configs = 4,
-	.num_gpios = 2,
-	.has_int_ref = true,
-	.clock = 16 * HZ_PER_MHZ,
-	.sinc5_data_rates = ad7175_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-};
-
-static const struct ad7173_device_info ad7177_2_device_info = {
-	.name = "ad7177-2",
-	.id = AD7177_ID,
-	.num_voltage_in = 5,
-	.num_channels = 4,
-	.num_configs = 4,
-	.num_gpios = 2,
-	.has_temp = true,
-	.has_input_buf = true,
-	.has_int_ref = true,
-	.has_pow_supply_monitoring = true,
-	.clock = 16 * HZ_PER_MHZ,
-	.odr_start_value = AD7177_ODR_START_VALUE,
-	.sinc5_data_rates = ad7175_sinc5_data_rates,
-	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-};
-
 static const char *const ad7173_ref_sel_str[] = {
 	[AD7173_SETUP_REF_SEL_EXT_REF]    = "vref",
 	[AD7173_SETUP_REF_SEL_EXT_REF2]   = "vref2",
@@ -753,7 +536,21 @@ static int ad7173_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
 	return ad_sd_write_reg(sd, AD7173_REG_CH(chan), 2, 0);
 }
 
-static const struct ad_sigma_delta_info ad7173_sigma_delta_info = {
+static const struct ad_sigma_delta_info ad7173_sigma_delta_info_4_slots = {
+	.set_channel = ad7173_set_channel,
+	.append_status = ad7173_append_status,
+	.disable_all = ad7173_disable_all,
+	.disable_one = ad7173_disable_one,
+	.set_mode = ad7173_set_mode,
+	.has_registers = true,
+	.addr_shift = 0,
+	.read_mask = BIT(6),
+	.status_ch_mask = GENMASK(3, 0),
+	.data_reg = AD7173_REG_DATA,
+	.num_slots = 4,
+};
+
+static const struct ad_sigma_delta_info ad7173_sigma_delta_info_8_slots = {
 	.set_channel = ad7173_set_channel,
 	.append_status = ad7173_append_status,
 	.disable_all = ad7173_disable_all,
@@ -764,6 +561,237 @@ static const struct ad_sigma_delta_info ad7173_sigma_delta_info = {
 	.read_mask = BIT(6),
 	.status_ch_mask = GENMASK(3, 0),
 	.data_reg = AD7173_REG_DATA,
+	.num_slots = 8,
+};
+
+static const struct ad7173_device_info ad4111_device_info = {
+	.name = "ad4111",
+	.id = AD4111_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in_div = 8,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 8,
+	.num_gpios = 2,
+	.higher_gpio_bits = true,
+	.has_temp = true,
+	.has_vincom_input = true,
+	.has_input_buf = true,
+	.has_current_inputs = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4112_device_info = {
+	.name = "ad4112",
+	.id = AD4112_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in_div = 8,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 8,
+	.num_gpios = 2,
+	.higher_gpio_bits = true,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_current_inputs = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4113_device_info = {
+	.name = "ad4113",
+	.id = AD4113_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in_div = 8,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 8,
+	.num_gpios = 2,
+	.data_reg_only_16bit = true,
+	.higher_gpio_bits = true,
+	.has_vincom_input = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4114_device_info = {
+	.name = "ad4114",
+	.id = AD4114_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in_div = 16,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 16,
+	.num_gpios = 4,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4115_device_info = {
+	.name = "ad4115",
+	.id = AD4115_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in_div = 16,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 16,
+	.num_gpios = 4,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 8 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad4115_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4116_device_info = {
+	.name = "ad4116",
+	.id = AD4116_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in_div = 11,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 16,
+	.num_gpios = 4,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 4 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad4116_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7172_2_device_info = {
+	.name = "ad7172-2",
+	.id = AD7172_2_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7172_4_device_info = {
+	.name = "ad7172-4",
+	.id = AD7172_4_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in = 9,
+	.num_channels = 8,
+	.num_configs = 8,
+	.num_gpios = 4,
+	.has_input_buf = true,
+	.has_ref2 = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7173_8_device_info = {
+	.name = "ad7173-8",
+	.id = AD7173_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in = 17,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_gpios = 4,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_ref2 = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7175_2_device_info = {
+	.name = "ad7175-2",
+	.id = AD7175_2_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7175_8_device_info = {
+	.name = "ad7175-8",
+	.id = AD7175_8_ID,
+	.sd_info = &ad7173_sigma_delta_info_8_slots,
+	.num_voltage_in = 17,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_gpios = 4,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_ref2 = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7176_2_device_info = {
+	.name = "ad7176-2",
+	.id = AD7176_ID,
+	.sd_info = &ad7173_sigma_delta_info_4_slots,
+	.num_voltage_in = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_int_ref = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7177_2_device_info = {
+	.name = "ad7177-2",
+	.id = AD7177_ID,
+	.sd_info = &ad7173_sigma_delta_info_4_slots,
+	.num_voltage_in = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.odr_start_value = AD7177_ODR_START_VALUE,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
 };
 
 static int ad7173_setup(struct iio_dev *indio_dev)
@@ -1429,9 +1457,7 @@ static int ad7173_probe(struct spi_device *spi)
 	spi->mode = SPI_MODE_3;
 	spi_setup(spi);
 
-	st->sigma_delta_info = ad7173_sigma_delta_info;
-	st->sigma_delta_info.num_slots = st->info->num_configs;
-	ret = ad_sd_init(&st->sd, indio_dev, spi, &st->sigma_delta_info);
+	ret = ad_sd_init(&st->sd, indio_dev, spi, st->info->sd_info);
 	if (ret)
 		return ret;
 

-- 
2.43.0

Re: [PATCH v2 3/3] iio: adc: ad7173: don't make copy of ad_sigma_delta_info struct

[Andy Shevchenko]

On Wed, Nov 27, 2024 at 10:02 PM David Lechner <dlechner@baylibre.com> wrote:
>
> Use two separate static const struct ad_sigma_delta_info instances
> instead of making a copy for each driver instance.
>
> Typically in the IIO subsystem, we use multiple static const instances
> of the same struct when there are different variants of the same family
> of devices as opposed to making a copy for each driver instance and
> modifying it.

...

> Whether this patch is an improvement or not is debatable. It makes
> things a bit more verbose, but to me it seems more consistent with
> how such cases are handled in the IIO subsystem. So take this one or
> leave it.

Constifications are always an improvement. This makes the related
pieces of data to be moved to rodata sections and hence have proper
page flag settings at runtime avoiding any modifications of the data
which might be used for ROP gadgets or something like that in some
cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] iio: adc: ad7173: fix non-const info struct

[Guillaume Ranquet]

On Wed, 27 Nov 2024 21:01, David Lechner <dlechner@baylibre.com> wrote:
>While working ad7124, Uwe pointed out a bug in the ad7173 driver.
>static struct ad_sigma_delta_info ad7173_sigma_delta_info was not const
>and was being modified during driver probe, which could lead to race
>conditions if two instances of the driver were probed at the same time.
>
>The actual fix part is fairly trivial but I have only compile tested it.
>Guillaume has access to ad4111 hardware, so it would be good to get a
>Tested-by from him to make sure this doesn't break anything.
>
>---
>Changes in v2:
>- Fixed chip name in a few places.
>- Add new simpler patch for "fix" that gets backported.
>- Rebase other patches on this and incorporate feedback.
>- Link to v1: https://lore.kernel.org/r/20241122-iio-adc-ad7313-fix-non-const-info-struct-v1-0-d05c02324b73@baylibre.com
>
>---
>David Lechner (3):
>      iio: adc: ad7173: fix using shared static info struct
>      iio: adc: ad7173: remove special handling for irq number
>      iio: adc: ad7173: don't make copy of ad_sigma_delta_info struct
>
> drivers/iio/adc/ad7173.c               | 474 +++++++++++++++++----------------
> drivers/iio/adc/ad_sigma_delta.c       |   5 +-
> include/linux/iio/adc/ad_sigma_delta.h |   2 -
> 3 files changed, 249 insertions(+), 232 deletions(-)
>---
>base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
>change-id: 20241122-iio-adc-ad7313-fix-non-const-info-struct-92e59b91ee2e
>
>Best regards,
>--
>David Lechner <dlechner@baylibre.com>

Tested-by: Guillaume Ranquet <granquet@baylibre.com>

Re: [PATCH v2 0/3] iio: adc: ad7173: fix non-const info struct

[Jonathan Cameron]

On Wed, 27 Nov 2024 14:01:52 -0600
David Lechner <dlechner@baylibre.com> wrote:

> While working ad7124, Uwe pointed out a bug in the ad7173 driver.
> static struct ad_sigma_delta_info ad7173_sigma_delta_info was not const
> and was being modified during driver probe, which could lead to race
> conditions if two instances of the driver were probed at the same time.
> 
> The actual fix part is fairly trivial but I have only compile tested it.
> Guillaume has access to ad4111 hardware, so it would be good to get a
> Tested-by from him to make sure this doesn't break anything.
> 
This is very big for a backport.  So I replied to previous version to suggest
instead duplicating the data before modifying.  That has much less code
movement and maybe a cleaner fix.  Perhaps we then cycle back to avoiding
that copy later.

Jonathan

> ---
> Changes in v2:
> - Fixed chip name in a few places.
> - Add new simpler patch for "fix" that gets backported.
> - Rebase other patches on this and incorporate feedback.
> - Link to v1: https://lore.kernel.org/r/20241122-iio-adc-ad7313-fix-non-const-info-struct-v1-0-d05c02324b73@baylibre.com
> 
> ---
> David Lechner (3):
>       iio: adc: ad7173: fix using shared static info struct
>       iio: adc: ad7173: remove special handling for irq number
>       iio: adc: ad7173: don't make copy of ad_sigma_delta_info struct
> 
>  drivers/iio/adc/ad7173.c               | 474 +++++++++++++++++----------------
>  drivers/iio/adc/ad_sigma_delta.c       |   5 +-
>  include/linux/iio/adc/ad_sigma_delta.h |   2 -
>  3 files changed, 249 insertions(+), 232 deletions(-)
> ---
> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
> change-id: 20241122-iio-adc-ad7313-fix-non-const-info-struct-92e59b91ee2e
> 
> Best regards,

Re: [PATCH v2 0/3] iio: adc: ad7173: fix non-const info struct

[David Lechner]

On 11/30/24 12:43 PM, Jonathan Cameron wrote:
> On Wed, 27 Nov 2024 14:01:52 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> While working ad7124, Uwe pointed out a bug in the ad7173 driver.
>> static struct ad_sigma_delta_info ad7173_sigma_delta_info was not const
>> and was being modified during driver probe, which could lead to race
>> conditions if two instances of the driver were probed at the same time.
>>
>> The actual fix part is fairly trivial but I have only compile tested it.
>> Guillaume has access to ad4111 hardware, so it would be good to get a
>> Tested-by from him to make sure this doesn't break anything.
>>
> This is very big for a backport.  So I replied to previous version to suggest
> instead duplicating the data before modifying.  That has much less code
> movement and maybe a cleaner fix.  Perhaps we then cycle back to avoiding
> that copy later.
> 
That is exactly what I did in v2. "iio: adc: ad7173: fix using shared
static info struct" copies the struct before modifying it and is the
only patch with a Fixes: tag.

Re: [PATCH v2 0/3] iio: adc: ad7173: fix non-const info struct

[Jonathan Cameron]

On Sat, 30 Nov 2024 12:50:39 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 11/30/24 12:43 PM, Jonathan Cameron wrote:
> > On Wed, 27 Nov 2024 14:01:52 -0600
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> While working ad7124, Uwe pointed out a bug in the ad7173 driver.
> >> static struct ad_sigma_delta_info ad7173_sigma_delta_info was not const
> >> and was being modified during driver probe, which could lead to race
> >> conditions if two instances of the driver were probed at the same time.
> >>
> >> The actual fix part is fairly trivial but I have only compile tested it.
> >> Guillaume has access to ad4111 hardware, so it would be good to get a
> >> Tested-by from him to make sure this doesn't break anything.
> >>  
> > This is very big for a backport.  So I replied to previous version to suggest
> > instead duplicating the data before modifying.  That has much less code
> > movement and maybe a cleaner fix.  Perhaps we then cycle back to avoiding
> > that copy later.
> >   
> That is exactly what I did in v2. "iio: adc: ad7173: fix using shared
> static info struct" copies the struct before modifying it and is the
> only patch with a Fixes: tag.
> 

Oops - brain fart. Applied patch 1 for now to the fixes-togreg branch of iio.git
and marked it for stable.  I'll get the others once that's upstream.

Thanks,

Jonathan