| From 1c9c2642f1b1f764ce0ba2d387b2ce849cb9f31b Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Mon, 3 Mar 2025 12:47:02 +0100 |
| Subject: iio: adc: ad7173: Fix comparison of channel configs |
| MIME-Version: 1.0 |
| Content-Type: text/plain; charset=UTF-8 |
| Content-Transfer-Encoding: 8bit |
| |
| From: Uwe Kleine-König <u.kleine-koenig@baylibre.com> |
| |
| [ Upstream commit 7b6033ed5a9e1a369a9cf58018388ae4c5f17e41 ] |
| |
| 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 static_assert guard to (somewhat) ensure that when |
| struct ad7173_channel_config::config_props is expanded, the comparison |
| is adapted, too. |
| |
| This issue is somewhat theoretic, but using memcmp() on a struct is a |
| bad pattern that is worth fixing. |
| |
| Fixes: 76a1e6a42802 ("iio: adc: ad7173: add AD7173 driver") |
| Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> |
| Link: https://patch.msgid.link/20250303114659.1672695-14-u.kleine-koenig@baylibre.com |
| Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/iio/adc/ad7173.c | 25 +++++++++++++++++++++---- |
| 1 file changed, 21 insertions(+), 4 deletions(-) |
| |
| diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c |
| index 8b03c1e5567e5..050e965358cb3 100644 |
| --- a/drivers/iio/adc/ad7173.c |
| +++ b/drivers/iio/adc/ad7173.c |
| @@ -183,7 +183,11 @@ struct ad7173_channel_config { |
| u8 cfg_slot; |
| bool live; |
| |
| - /* Following fields are used to compare equality. */ |
| + /* |
| + * Following fields are used to compare equality. If you |
| + * make adaptations in it, you most likely also have to adapt |
| + * ad7173_find_live_config(), too. |
| + */ |
| struct_group(config_props, |
| bool bipolar; |
| bool input_buf; |
| @@ -602,15 +606,28 @@ 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); |
| + /* |
| + * This is just to make sure that the comparison is adapted after |
| + * struct ad7173_channel_config was changed. |
| + */ |
| + static_assert(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; |
| -- |
| 2.39.5 |
| |