| Message ID | 4d1ba8c9a890393fce38083b01db6ce03df8ac5b.1739902968.git.u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad{4130,7124,7173}: A few fixes and ad7124 calibration | expand |
On 2/18/25 12:31 PM, 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 Now it is static_assert instead of BUILD_BUG. > 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> > ---
On Tue, Feb 18, 2025 at 8:31 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> 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 > struct ad4130_setup_info is expanded, the comparison is adapted, too. ... > +/* > + * If you make adaptions in this struct, you most likely also have to adapt adaptations? > + * ad4130_setup_info_eq(), too. > + */ ... > +static bool ad4130_setup_info_eq(struct ad4130_setup_info *a, > + struct ad4130_setup_info *b) > +{ > + /* > + * This is just to make sure that the comparison is adapted after > + * struct ad4130_setup_info was changed. > + */ > + static_assert(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; > + })); This can be moved out of the function, but in any case it should not affect code generation I believe. > + 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; > +} ... > 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)) { I don't remember if we discussed the idea of using crc32 to compare the data structures, like it's done for PLD data in USB Type-C cases. https://elixir.bootlin.com/linux/v6.14-rc3/C/ident/pld_crc > *slot = i; > return 0; > }
On Wed, Feb 19, 2025 at 10:53:54AM +0200, Andy Shevchenko wrote: > On Tue, Feb 18, 2025 at 8:31 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> 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 > > struct ad4130_setup_info is expanded, the comparison is adapted, too. > > ... > > > +/* > > + * If you make adaptions in this struct, you most likely also have to adapt > > adaptations? ack. > > +static bool ad4130_setup_info_eq(struct ad4130_setup_info *a, > > + struct ad4130_setup_info *b) > > +{ > > + /* > > + * This is just to make sure that the comparison is adapted after > > + * struct ad4130_setup_info was changed. > > + */ > > + static_assert(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; > > + })); > > This can be moved out of the function, but in any case it should not > affect code generation I believe. It can, but I like having it near its usage. > > + 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; > > +} > > ... > > > 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)) { > > I don't remember if we discussed the idea of using crc32 to compare > the data structures, like it's done for PLD data in USB Type-C cases. > https://elixir.bootlin.com/linux/v6.14-rc3/C/ident/pld_crc crc32 works on the binary representation again, right? That's what this patch targets to remove. Best regards Uwe
diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c index acc241cc0a7a..b4eb3065ae34 100644 --- a/drivers/iio/adc/ad4130.c +++ b/drivers/iio/adc/ad4130.c @@ -223,6 +223,10 @@ enum ad4130_pin_function { AD4130_PIN_FN_VBIAS = BIT(3), }; +/* + * If you make adaptions in this struct, you most likely also have to adapt + * ad4130_setup_info_eq(), too. + */ struct ad4130_setup_info { unsigned int iout0_val; unsigned int iout1_val; @@ -591,6 +595,40 @@ 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) +{ + /* + * This is just to make sure that the comparison is adapted after + * struct ad4130_setup_info was changed. + */ + static_assert(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 +642,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; }
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 | 41 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)