[PATCH 0/2] iio: adc: ad*: Fix comparisons using memcmp

[PATCH 0/2] iio: adc: ad*: Fix comparisons using memcmp

[Uwe Kleine-König]

Hello,

I recently sent a patch fixing a bad coding pattern in the ad7124
driver; see
https://lore.kernel.org/linux-iio/20250124160124.435520-2-u.kleine-koenig@baylibre.com/.

The same pattern is used for ad4130 and ad7173. This series fixes these
two drivers in the same way. As for the ad7124 driver this didn't break
in production and if the memcmp comparison wrongly returns false,
nothing bad happens. Only usage of the resources isn't optimal then but
that is probably nothing a user would notice. So there is no urge here.

Still it's the correct thing to do to drop the comparison using memcmp
and compare member-by-member instead.

Best regards
Uwe

Uwe Kleine-König (2):
  iio: adc: ad4130: Fix comparison of channel setups
  iio: adc: ad7173: Fix comparison of channel configs

 drivers/iio/adc/ad4130.c | 33 +++++++++++++++++++++++++++++++--
 drivers/iio/adc/ad7173.c | 15 ++++++++++++---
 2 files changed, 43 insertions(+), 5 deletions(-)

base-commit: a13f6e0f405ed0d3bcfd37c692c7d7fa3c052154
-- 
2.47.1

[PATCH 1/2] iio: adc: ad4130: Fix comparison of channel setups

[Uwe Kleine-König]

Checking the binary representation of two structs (of the same type)
for equality doesn't have the same semantic as comparing all members for
equality. The former might find a difference where the latter doesn't in
the presence of padding or when ambiguous types like float or bool are
involved. (Floats typically have different representations for single
values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
true, but memcmp finds a difference.)

When searching for a channel that already has the configuration we need,
the comparison by member is the one that is needed.

Convert the comparison accordingly to compare the members one after
another. Also add a BUILD_BUG guard to (somewhat) ensure that when
struct ad4130_setup_info is expanded, the comparison is adapted, too.

Fixes: 62094060cf3a ("iio: adc: ad4130: add AD4130 driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad4130.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index de32cc9d18c5..ae321df426b5 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -591,6 +591,36 @@ static irqreturn_t ad4130_irq_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static bool ad4130_setup_info_eq(struct ad4130_setup_info *a,
+				 struct ad4130_setup_info *b)
+{
+	BUILD_BUG_ON(sizeof(*a) !=
+		     sizeof(struct {
+				    unsigned int iout0_val;
+				    unsigned int iout1_val;
+				    unsigned int burnout;
+				    unsigned int pga;
+				    unsigned int fs;
+				    u32 ref_sel;
+				    enum ad4130_filter_mode filter_mode;
+				    bool ref_bufp;
+				    bool ref_bufm;
+			    }));
+
+	if (a->iout0_val != b->iout0_val ||
+	    a->iout1_val != b->iout1_val ||
+	    a->burnout != b->burnout ||
+	    a->pga != b->pga ||
+	    a->fs != b->fs ||
+	    a->ref_sel != b->ref_sel ||
+	    a->filter_mode != b->filter_mode ||
+	    a->ref_bufp != b->ref_bufp ||
+	    a->ref_bufm != b->ref_bufm)
+		return false;
+
+	return true;
+}
+
 static int ad4130_find_slot(struct ad4130_state *st,
 			    struct ad4130_setup_info *target_setup_info,
 			    unsigned int *slot, bool *overwrite)
@@ -604,8 +634,7 @@ static int ad4130_find_slot(struct ad4130_state *st,
 		struct ad4130_slot_info *slot_info = &st->slots_info[i];
 
 		/* Immediately accept a matching setup info. */
-		if (!memcmp(target_setup_info, &slot_info->setup,
-			    sizeof(*target_setup_info))) {
+		if (ad4130_setup_info_eq(target_setup_info, &slot_info->setup)) {
 			*slot = i;
 			return 0;
 		}
-- 
2.47.1

[PATCH 2/2] iio: adc: ad7173: Fix comparison of channel configs

[Uwe Kleine-König]

Checking the binary representation of two structs (of the same type)
for equality doesn't have the same semantic as comparing all members for
equality. The former might find a difference where the latter doesn't in
the presence of padding or when ambiguous types like float or bool are
involved. (Floats typically have different representations for single
values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
true, but memcmp finds a difference.)

When searching for a channel that already has the configuration we need,
the comparison by member is the one that is needed.

Convert the comparison accordingly to compare the members one after
another. Also add a BUILD_BUG guard to (somewhat) ensure that when
struct ad7173_channel_config::config_props is expanded, the comparison
is adapted, too.

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

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 6c4ed10ae580..67821c889010 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -712,15 +712,24 @@ static struct ad7173_channel_config *
 ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
 {
 	struct ad7173_channel_config *cfg_aux;
-	ptrdiff_t cmp_size;
 	int i;
 
-	cmp_size = sizeof_field(struct ad7173_channel_config, config_props);
+	BUILD_BUG_ON(sizeof_field(struct ad7173_channel_config, config_props) !=
+		     sizeof(struct {
+				    bool bipolar;
+				    bool input_buf;
+				    u8 odr;
+				    u8 ref_sel;
+			    }));
+
 	for (i = 0; i < st->num_channels; i++) {
 		cfg_aux = &st->channels[i].cfg;
 
 		if (cfg_aux->live &&
-		    !memcmp(&cfg->config_props, &cfg_aux->config_props, cmp_size))
+		    cfg->bipolar == cfg_aux->bipolar &&
+		    cfg->input_buf == cfg_aux->input_buf &&
+		    cfg->odr == cfg_aux->odr &&
+		    cfg->ref_sel == cfg_aux->ref_sel)
 			return cfg_aux;
 	}
 	return NULL;
-- 
2.47.1

Re: [PATCH 1/2] iio: adc: ad4130: Fix comparison of channel setups

[Andy Shevchenko]

On Thu, Jan 30, 2025 at 06:45:01PM +0100, Uwe Kleine-König wrote:
> Checking the binary representation of two structs (of the same type)
> for equality doesn't have the same semantic as comparing all members for
> equality. The former might find a difference where the latter doesn't in
> the presence of padding or when ambiguous types like float or bool are
> involved. (Floats typically have different representations for single
> values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
> at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
> true, but memcmp finds a difference.)
> 
> When searching for a channel that already has the configuration we need,
> the comparison by member is the one that is needed.
> 
> Convert the comparison accordingly to compare the members one after
> another. Also add a BUILD_BUG guard to (somewhat) ensure that when

Why do you use BUILD_BUG_ON() instead of static_assert()?

> struct ad4130_setup_info is expanded, the comparison is adapted, too.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: ad7173: Fix comparison of channel configs

[Andy Shevchenko]

On Thu, Jan 30, 2025 at 06:45:02PM +0100, Uwe Kleine-König wrote:
> Checking the binary representation of two structs (of the same type)
> for equality doesn't have the same semantic as comparing all members for
> equality. The former might find a difference where the latter doesn't in
> the presence of padding or when ambiguous types like float or bool are
> involved. (Floats typically have different representations for single
> values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
> at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
> true, but memcmp finds a difference.)

memcmp()

> When searching for a channel that already has the configuration we need,
> the comparison by member is the one that is needed.
> 
> Convert the comparison accordingly to compare the members one after
> another. Also add a BUILD_BUG guard to (somewhat) ensure that when

static_assert() will give a better messaging.

> struct ad7173_channel_config::config_props is expanded, the comparison
> is adapted, too.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: adc: ad4130: Fix comparison of channel setups

[Andy Shevchenko]

On Thu, Jan 30, 2025 at 06:45:01PM +0100, Uwe Kleine-König wrote:

...

> +	BUILD_BUG_ON(sizeof(*a) !=
> +		     sizeof(struct {
> +				    unsigned int iout0_val;
> +				    unsigned int iout1_val;
> +				    unsigned int burnout;
> +				    unsigned int pga;
> +				    unsigned int fs;
> +				    u32 ref_sel;
> +				    enum ad4130_filter_mode filter_mode;
> +				    bool ref_bufp;
> +				    bool ref_bufm;
> +			    }));

Is I shuffle the fields (for whatever reason) this may give false positive
warnings. I think this BUILD_BUG_ON() is unreliable and ugly looking
(static_assert() won't help much here either), so on the second though I think
it's better to simply add a comments in both places (here and near to the
structure definition) to explain that these needs to be in sync.

> +	if (a->iout0_val != b->iout0_val ||
> +	    a->iout1_val != b->iout1_val ||
> +	    a->burnout != b->burnout ||
> +	    a->pga != b->pga ||
> +	    a->fs != b->fs ||
> +	    a->ref_sel != b->ref_sel ||
> +	    a->filter_mode != b->filter_mode ||
> +	    a->ref_bufp != b->ref_bufp ||
> +	    a->ref_bufm != b->ref_bufm)
> +		return false;
> +
> +	return true;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: adc: ad4130: Fix comparison of channel setups

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On Thu, Jan 30, 2025 at 08:14:39PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 30, 2025 at 06:45:01PM +0100, Uwe Kleine-König wrote:
> 
> ...
> 
> > +	BUILD_BUG_ON(sizeof(*a) !=
> > +		     sizeof(struct {
> > +				    unsigned int iout0_val;
> > +				    unsigned int iout1_val;
> > +				    unsigned int burnout;
> > +				    unsigned int pga;
> > +				    unsigned int fs;
> > +				    u32 ref_sel;
> > +				    enum ad4130_filter_mode filter_mode;
> > +				    bool ref_bufp;
> > +				    bool ref_bufm;
> > +			    }));
> 
> Is I shuffle the fields (for whatever reason) this may give false positive
> warnings.

That's fine in by book. Whenever the struct is changed it's a good
opportunity to check if the cmp function needs adaption, too.

> I think this BUILD_BUG_ON() is unreliable and ugly looking
> (static_assert() won't help much here either), so on the second though I think
> it's better to simply add a comments in both places (here and near to the
> structure definition) to explain that these needs to be in sync.

The nice thing about BUILD_BUG_ON (or static assert) is that the
compiler quite reliably enforces the two being in sync. A comment
doesn't.

But I don't care much and can rework to use whatever pleases the
responsible guys.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] iio: adc: ad4130: Fix comparison of channel setups

[Nuno Sá]

On Wed, 2025-02-12 at 10:01 +0100, Uwe Kleine-König wrote:
> On Thu, Jan 30, 2025 at 08:14:39PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 30, 2025 at 06:45:01PM +0100, Uwe Kleine-König wrote:
> > 
> > ...
> > 
> > > +	BUILD_BUG_ON(sizeof(*a) !=
> > > +		     sizeof(struct {
> > > +				    unsigned int iout0_val;
> > > +				    unsigned int iout1_val;
> > > +				    unsigned int burnout;
> > > +				    unsigned int pga;
> > > +				    unsigned int fs;
> > > +				    u32 ref_sel;
> > > +				    enum ad4130_filter_mode filter_mode;
> > > +				    bool ref_bufp;
> > > +				    bool ref_bufm;
> > > +			    }));
> > 
> > Is I shuffle the fields (for whatever reason) this may give false positive
> > warnings.
> 
> That's fine in by book. Whenever the struct is changed it's a good
> opportunity to check if the cmp function needs adaption, too.
> 

In practise I think shuffling the fields is unlikely...

> > I think this BUILD_BUG_ON() is unreliable and ugly looking
> > (static_assert() won't help much here either), so on the second though I
> > think
> > it's better to simply add a comments in both places (here and near to the
> > structure definition) to explain that these needs to be in sync.
> 
> The nice thing about BUILD_BUG_ON (or static assert) is that the
> compiler quite reliably enforces the two being in sync. A comment
> doesn't.
> 

Agreed... I guess we could also add a comment on top of the affected struct so
that if someone messes with it, is also aware of this comparison.

- Nuno Sá