| Message ID | 20250124160124.435520-2-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad7124: Fix comparison of channel configs | expand |
On Fri, 24 Jan 2025 17:01:23 +0100 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 ad7124_channel_config::config_props is expanded, the comparison > is adapted, too. > > Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Hi Uwe, Fully agree the change makes sense. Though the floating point example is unlikely to bite us in kernel! Did you see an actual failure to match? If not I'd be tempted to take this for next cycle rather than as a fix to rush in. I'd be surprised if current code doesn't happen to work. Thanks, Jonathan > --- > drivers/iio/adc/ad7124.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > index 6ae27cdd3250..5eb8ced416ba 100644 > --- a/drivers/iio/adc/ad7124.c > +++ b/drivers/iio/adc/ad7124.c > @@ -338,15 +338,38 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_ > struct ad7124_channel_config *cfg) > { > struct ad7124_channel_config *cfg_aux; > - ptrdiff_t cmp_size; > int i; > > - cmp_size = sizeof_field(struct ad7124_channel_config, config_props); > + /* > + * This is just to make sure that the comparison is adapted after > + * struct ad7124_channel_config was changed. > + */ > + BUILD_BUG_ON(sizeof_field(struct ad7124_channel_config, config_props) != > + sizeof(struct { > + enum ad7124_ref_sel refsel; > + bool bipolar; > + bool buf_positive; > + bool buf_negative; > + unsigned int vref_mv; > + unsigned int pga_bits; > + unsigned int odr; > + unsigned int odr_sel_bits; > + unsigned int filter_type; > + })); > + > 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->refsel == cfg_aux->refsel && > + cfg->bipolar == cfg_aux->bipolar && > + cfg->buf_positive == cfg_aux->buf_positive && > + cfg->buf_negative == cfg_aux->buf_negative && > + cfg->vref_mv == cfg_aux->vref_mv && > + cfg->pga_bits == cfg_aux->pga_bits && > + cfg->odr == cfg_aux->odr && > + cfg->odr_sel_bits == cfg_aux->odr_sel_bits && > + cfg->filter_type == cfg_aux->filter_type) > return cfg_aux; > } > > > base-commit: 5ffa57f6eecefababb8cbe327222ef171943b183
Hello Jonathan, On Sat, Jan 25, 2025 at 02:48:28PM +0000, Jonathan Cameron wrote: > Fully agree the change makes sense. Though the floating point example > is unlikely to bite us in kernel! > > Did you see an actual failure to match? If not I'd be tempted > to take this for next cycle rather than as a fix to rush in. > I'd be surprised if current code doesn't happen to work. No, I didn't see a failure, I noticed that while working on the driver source. Also in this case nothing severe happens. The check might only result in failure to detect that a setup could be reused and uses a different one then. In the worst case that increases pressure to rotate the setup configuration in a timely manner. David also pointed out in a private conversion that the bool issue probably isn't relevant because the compiler (at least gcc, didn't check clang) only uses values 0 and 1 and the padding ideally should be all zero. Anyhow, I think comparing the actual byte representation is a bad pattern that shouldn't be used even if in that case it might work. So I'm not aware of a problem that would justify getting this in early as a fix and the next cycle it totally fine. Best regards Uwe
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index 6ae27cdd3250..5eb8ced416ba 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -338,15 +338,38 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_ struct ad7124_channel_config *cfg) { struct ad7124_channel_config *cfg_aux; - ptrdiff_t cmp_size; int i; - cmp_size = sizeof_field(struct ad7124_channel_config, config_props); + /* + * This is just to make sure that the comparison is adapted after + * struct ad7124_channel_config was changed. + */ + BUILD_BUG_ON(sizeof_field(struct ad7124_channel_config, config_props) != + sizeof(struct { + enum ad7124_ref_sel refsel; + bool bipolar; + bool buf_positive; + bool buf_negative; + unsigned int vref_mv; + unsigned int pga_bits; + unsigned int odr; + unsigned int odr_sel_bits; + unsigned int filter_type; + })); + 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->refsel == cfg_aux->refsel && + cfg->bipolar == cfg_aux->bipolar && + cfg->buf_positive == cfg_aux->buf_positive && + cfg->buf_negative == cfg_aux->buf_negative && + cfg->vref_mv == cfg_aux->vref_mv && + cfg->pga_bits == cfg_aux->pga_bits && + cfg->odr == cfg_aux->odr && + cfg->odr_sel_bits == cfg_aux->odr_sel_bits && + cfg->filter_type == cfg_aux->filter_type) return cfg_aux; }
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 ad7124_channel_config::config_props is expanded, the comparison is adapted, too. Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad7124.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) base-commit: 5ffa57f6eecefababb8cbe327222ef171943b183