On Tue, Feb 18, 2025 at 8:31 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxx> 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;
> }
--
With Best Regards,
Andy Shevchenko
