| Message ID | 20191209170541.198206-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: st_lsm6dsx: Fix selection of ST_LSM6DS3_ID | expand |
> At the moment, attempting to probe a device with ST_LSM6DS3_ID > (e.g. using the st,lsm6ds3 compatible) fails with: > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > Hi Stephan, thx for working on this. I guess we can skip 'void' iterations defining the array real size, do you agree? Regards, Lorenzo > This happens because st_lsm6dsx_check_whoami() also attempts > to match unspecified (zero-initialized) entries in the "id" array. > ST_LSM6DS3_ID = 0 will therefore match any entry in > st_lsm6dsx_sensor_settings (here: the first), because none of them > actually have all 12 entries listed in the "id" array. > > Avoid this by additionally checking if "name" is set, > which is only set for valid entries in the "id" array. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > index a7d40c02ce6b..b921dd9e108f 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > break; > } > if (j < ST_LSM6DSX_MAX_ID) > -- > 2.24.0 >
On Mon, Dec 09, 2019 at 10:48:52PM +0100, Lorenzo Bianconi wrote: > > At the moment, attempting to probe a device with ST_LSM6DS3_ID > > (e.g. using the st,lsm6ds3 compatible) fails with: > > > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > > > > Hi Stephan, > > thx for working on this. I guess we can skip 'void' iterations defining the > array real size, do you agree? > I'm not sure I understand you correctly. Do you mean having something like: struct st_lsm6dsx_settings { u8 wai; /* ... */ struct { enum st_lsm6dsx_hw_id hw_id; const char *name; } id[ST_LSM6DSX_MAX_ID]; int id_num; /* Add this field */ /* ... */ }; And then change the loop to use .id_num instead? I think it is pretty easy to forget to update "id_num" when adding new entries. Right now there is no need to worry about that. Thanks, Stephan > Regards, > Lorenzo > > > This happens because st_lsm6dsx_check_whoami() also attempts > > to match unspecified (zero-initialized) entries in the "id" array. > > ST_LSM6DS3_ID = 0 will therefore match any entry in > > st_lsm6dsx_sensor_settings (here: the first), because none of them > > actually have all 12 entries listed in the "id" array. > > > > Avoid this by additionally checking if "name" is set, > > which is only set for valid entries in the "id" array. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > --- > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > index a7d40c02ce6b..b921dd9e108f 100644 > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > break; > > } > > if (j < ST_LSM6DSX_MAX_ID) > > -- > > 2.24.0 > >
> On Mon, Dec 09, 2019 at 10:48:52PM +0100, Lorenzo Bianconi wrote: > > > At the moment, attempting to probe a device with ST_LSM6DS3_ID > > > (e.g. using the st,lsm6ds3 compatible) fails with: > > > > > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > > > > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > > > > > > > Hi Stephan, > > > > thx for working on this. I guess we can skip 'void' iterations defining the > > array real size, do you agree? > > > > I'm not sure I understand you correctly. > Do you mean having something like: > > struct st_lsm6dsx_settings { > u8 wai; > /* ... */ > struct { > enum st_lsm6dsx_hw_id hw_id; > const char *name; > } id[ST_LSM6DSX_MAX_ID]; > int id_num; /* Add this field */ > /* ... */ > }; > > And then change the loop to use .id_num instead? > > I think it is pretty easy to forget to update "id_num" > when adding new entries. Right now there is no need to worry about that. Uhm..this approach is even more safe if someone forgets to set name for a given device. So for the patch: Acked-by: Lorenzo Bianconi <lorenzo@kernel.org> Regards, Lorenzo > > Thanks, > Stephan > > > Regards, > > Lorenzo > > > > > This happens because st_lsm6dsx_check_whoami() also attempts > > > to match unspecified (zero-initialized) entries in the "id" array. > > > ST_LSM6DS3_ID = 0 will therefore match any entry in > > > st_lsm6dsx_sensor_settings (here: the first), because none of them > > > actually have all 12 entries listed in the "id" array. > > > > > > Avoid this by additionally checking if "name" is set, > > > which is only set for valid entries in the "id" array. > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > > --- > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > index a7d40c02ce6b..b921dd9e108f 100644 > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > > > > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > > > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > > > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > > > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > > break; > > > } > > > if (j < ST_LSM6DSX_MAX_ID) > > > -- > > > 2.24.0 > > > > >
On Mon, 9 Dec 2019 18:05:41 +0100 Stephan Gerhold <stephan@gerhold.net> wrote: > At the moment, attempting to probe a device with ST_LSM6DS3_ID > (e.g. using the st,lsm6ds3 compatible) fails with: > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > > This happens because st_lsm6dsx_check_whoami() also attempts > to match unspecified (zero-initialized) entries in the "id" array. > ST_LSM6DS3_ID = 0 will therefore match any entry in > st_lsm6dsx_sensor_settings (here: the first), because none of them > actually have all 12 entries listed in the "id" array. > > Avoid this by additionally checking if "name" is set, > which is only set for valid entries in the "id" array. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> Definitely sounds like this wants backporting. If you can figure out a fixes tag that would be great! Thanks, Jonathan > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > index a7d40c02ce6b..b921dd9e108f 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > break; > } > if (j < ST_LSM6DSX_MAX_ID)
On Sun, 15 Dec 2019 17:23:42 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 9 Dec 2019 18:05:41 +0100 > Stephan Gerhold <stephan@gerhold.net> wrote: > > > At the moment, attempting to probe a device with ST_LSM6DS3_ID > > (e.g. using the st,lsm6ds3 compatible) fails with: > > > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > > > > This happens because st_lsm6dsx_check_whoami() also attempts > > to match unspecified (zero-initialized) entries in the "id" array. > > ST_LSM6DS3_ID = 0 will therefore match any entry in > > st_lsm6dsx_sensor_settings (here: the first), because none of them > > actually have all 12 entries listed in the "id" array. > > > > Avoid this by additionally checking if "name" is set, > > which is only set for valid entries in the "id" array. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > Definitely sounds like this wants backporting. > > If you can figure out a fixes tag that would be great! I've taken a stab at working out when this got introduced and came up with: 81956a93b522 "iio: imu: st_lsm6dsx: get device name from st_lsm6dsx_sensor_settings" If that's wrong please let me know asap. Applied to the togreg branch of iio.git as we are near the merge window opening and marked for stable. Thanks, Jonathan > > Thanks, > > Jonathan > > > --- > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > index a7d40c02ce6b..b921dd9e108f 100644 > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > break; > > } > > if (j < ST_LSM6DSX_MAX_ID) >
On Mon, Jan 13, 2020 at 10:12:11PM +0000, Jonathan Cameron wrote: > On Sun, 15 Dec 2019 17:23:42 +0000 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Mon, 9 Dec 2019 18:05:41 +0100 > > Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > At the moment, attempting to probe a device with ST_LSM6DS3_ID > > > (e.g. using the st,lsm6ds3 compatible) fails with: > > > > > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > > > > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > > > > > > This happens because st_lsm6dsx_check_whoami() also attempts > > > to match unspecified (zero-initialized) entries in the "id" array. > > > ST_LSM6DS3_ID = 0 will therefore match any entry in > > > st_lsm6dsx_sensor_settings (here: the first), because none of them > > > actually have all 12 entries listed in the "id" array. > > > > > > Avoid this by additionally checking if "name" is set, > > > which is only set for valid entries in the "id" array. > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > Definitely sounds like this wants backporting. > > > > If you can figure out a fixes tag that would be great! > I've taken a stab at working out when this got introduced and > came up with: > > 81956a93b522 "iio: imu: st_lsm6dsx: get device name from st_lsm6dsx_sensor_settings" > > If that's wrong please let me know asap. > > Applied to the togreg branch of iio.git as we are near the merge > window opening and marked for stable. You have already applied the v2 I sent with the fixes/cc stable tags :) It's part of your "Second set of IIO fixes for the 5.5 cycle." pull request: https://lore.kernel.org/linux-iio/20200105110051.445c9a95@archlinux/ > > Thanks, > > Jonathan > > > > Thanks, > > > > Jonathan > > > > > --- > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > index a7d40c02ce6b..b921dd9e108f 100644 > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > > > > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > > > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > > > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > > > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > > break; > > > } > > > if (j < ST_LSM6DSX_MAX_ID) > > >
On Tue, 14 Jan 2020 10:06:52 +0100 Stephan Gerhold <stephan@gerhold.net> wrote: > On Mon, Jan 13, 2020 at 10:12:11PM +0000, Jonathan Cameron wrote: > > On Sun, 15 Dec 2019 17:23:42 +0000 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > On Mon, 9 Dec 2019 18:05:41 +0100 > > > Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > > At the moment, attempting to probe a device with ST_LSM6DS3_ID > > > > (e.g. using the st,lsm6ds3 compatible) fails with: > > > > > > > > st_lsm6dsx_i2c 1-006b: unsupported whoami [69] > > > > > > > > ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. > > > > > > > > This happens because st_lsm6dsx_check_whoami() also attempts > > > > to match unspecified (zero-initialized) entries in the "id" array. > > > > ST_LSM6DS3_ID = 0 will therefore match any entry in > > > > st_lsm6dsx_sensor_settings (here: the first), because none of them > > > > actually have all 12 entries listed in the "id" array. > > > > > > > > Avoid this by additionally checking if "name" is set, > > > > which is only set for valid entries in the "id" array. > > > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > > Definitely sounds like this wants backporting. > > > > > > If you can figure out a fixes tag that would be great! > > I've taken a stab at working out when this got introduced and > > came up with: > > > > 81956a93b522 "iio: imu: st_lsm6dsx: get device name from st_lsm6dsx_sensor_settings" > > > > If that's wrong please let me know asap. > > > > Applied to the togreg branch of iio.git as we are near the merge > > window opening and marked for stable. > > You have already applied the v2 I sent with the fixes/cc stable tags :) > It's part of your "Second set of IIO fixes for the 5.5 cycle." pull request: > https://lore.kernel.org/linux-iio/20200105110051.445c9a95@archlinux/ Gah! I clearly wasn't on top form last night. Will drop this one again. Thanks. Jonathan > > > > > Thanks, > > > > Jonathan > > > > > > Thanks, > > > > > > Jonathan > > > > > > > --- > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > index a7d40c02ce6b..b921dd9e108f 100644 > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, > > > > > > > > for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { > > > > for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { > > > > - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > > > + if (st_lsm6dsx_sensor_settings[i].id[j].name && > > > > + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) > > > > break; > > > > } > > > > if (j < ST_LSM6DSX_MAX_ID) > > > > > >
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index a7d40c02ce6b..b921dd9e108f 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -1301,7 +1301,8 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id, for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) { for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) { - if (id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) + if (st_lsm6dsx_sensor_settings[i].id[j].name && + id == st_lsm6dsx_sensor_settings[i].id[j].hw_id) break; } if (j < ST_LSM6DSX_MAX_ID)
At the moment, attempting to probe a device with ST_LSM6DS3_ID (e.g. using the st,lsm6ds3 compatible) fails with: st_lsm6dsx_i2c 1-006b: unsupported whoami [69] ... even though 0x69 is the whoami listed for ST_LSM6DS3_ID. This happens because st_lsm6dsx_check_whoami() also attempts to match unspecified (zero-initialized) entries in the "id" array. ST_LSM6DS3_ID = 0 will therefore match any entry in st_lsm6dsx_sensor_settings (here: the first), because none of them actually have all 12 entries listed in the "id" array. Avoid this by additionally checking if "name" is set, which is only set for valid entries in the "id" array. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)