| Message ID | aa397f3749403477ee4a8f416b5890099f430ca5.1738258777.git.u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad*: Fix comparisons using memcmp | expand |
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.
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;
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(-)