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@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?
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
Attachment:
signature.asc
Description: PGP signature
