Message-ID:
 <AM8PR10MB4721FB1A78F25B204BE3A26ACD5FA@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM>
Date: Wed, 23 Jul 2025 09:46:47 +0000
From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "lars@...afoo.de" <lars@...afoo.de>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "dima.fedrau@...il.com" <dima.fedrau@...il.com>,
	"marcelo.schmitt1@...il.com" <marcelo.schmitt1@...il.com>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Lorenz
 Christian (ME-SE/EAD2)" <Christian.Lorenz3@...bosch.com>, "Frauendorf Ulrike
 (ME/PJ-SW3)" <Ulrike.Frauendorf@...bosch.com>, "Dolde Kai (ME-SE/PAE-A3)"
	<Kai.Dolde@...bosch.com>
Subject: AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

Hi Jonathan,

we find out the reason why the timestamp is invalid in the iio buffer.

https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L1093

In "iio_buffer_update_demux" to copy the timestamp, the address calculation is the root causes.

1083  in_loc += length;
....
1093  in_loc = roundup(in_loc, length);

When finish to copy the channel data, in_loc is just incremented and used as address of timestamp. This is correct only when the channel direct before timestamp is enabled.

If there is a gap between the last enabled channel and timestamp, then iio core will copy the wrong data.

We have a fix to this issue,

1093 in_loc = (indio_dev->scan_bytes / sizeof(int64_t) - 1) * length;

just not sure, if there will be any side-effects with this fix.

Are you going to fix this finding, or shall we create a new patch for that?

Best regards
Jianping Shen


>>
>> >>
>> >> >> +
>> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> >> >> +      struct iio_poll_func *pf = p;
>> >> >> +      struct iio_dev *indio_dev = pf->indio_dev;
>> >> >> +      struct smi330_data *data = iio_priv(indio_dev);
>> >> >> +      int ret, chan;
>> >> >> +      int i = 0;
>> >> >> +
>> >> >> +      ret = regmap_bulk_read(data->regmap,
>SMI330_ACCEL_X_REG, data-
>> >> >>buf,
>> >> >> +                             ARRAY_SIZE(smi330_channels));
>> >> >> +      if (ret)
>> >> >> +              goto out;
>> >> >> +
>> >> >> +      if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK)
>{
>> >> >> +              iio_for_each_active_channel(indio_dev, chan)
>> >> >> +                      data->buf[i++] = data->buf[chan];
>> >> >
>> >> >If I follow this correctly you are reading all the channels and
>> >> >just copying out the ones you want.  Just let the IIO core do that
>> >> >for you by setting iio_dev-
>> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the
>> >> >>whole
>> >> >buffer every time.
>> >>
>> >> For the most frequent use cases, we define available_scan_masks = {
>> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK,
>SMI330_GYRO_XYZ_MSK,
>> >0 }; and push the whole buffer every time.
>> >> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z.
>> >Then we enable buffer and expect that only the gyro values and
>> >timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values
>> >and the timestamp in iio_buffer.
>> >
>> >> It seems that the iio core does not take care which channel is
>> >> enabled,  just
>> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code
>> >still needs to take care and just copy the enabled channel value to buffer.
>> >
>> >Look again at how it works.  If you provide ACC_XYZ_MSK, then your
>> >driver has to handle it.
>> >available_scan_masks is saying what your driver supports. The driver
>> >can check active_scan_mask to find out what is enabled.  So right
>> >option here is only { SMI330_ALL_CHAN_MSK, 0, }  In that case the
>> >driver never needs to check as there is only one option.
>> >
>> >Then if any subset of channels is enabled the IIO core copy out just
>> >the data that is relevant.
>> >
>> >
>> >>
>> >> Another side effect after using available_scan_masks is that the
>> >active_scan_masks sometimes does not reflect current channel
>> >activation status.
>> >>
>> >> Is some step missing to properly use available_scan_masks ?  How
>> >> can a user
>> >find out from user space which channel combination is defined in
>> >available_scan_masks ?
>> >
>> >Why would userspace want to?  Userspace requested a subset of
>> >channels and it gets that subset.  So it if asks for the channels
>> >that make up SMI330_ACC_XYZ_MSK, if available_scan_mask == {
>> >SMI330_ALL_CHAN_MSK,
>> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest
>> >available mask that is superset of what we asked for) and sets
>> >active_scan_mask to that.  The driver follows what active_scan_mask
>> >specifies and passes all channel data via the iio_push_to_buffers*()
>> >call. The demux in the IIO core than takes that 'scan' and repacks it
>> >so that userspace receives just the data it asked for formatting
>> >exactly as the driver would have done it if you had handled each channels
>separately in the driver.
>> >
>>
>> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the
>> whole buffer. iio_push_to_buffers_with_timestamp (indio_dev, data->buf, pf-
>>timestamp); We enable the accX, accY, and accZ from userspace. And expect 3
>acc values and the timestamp in iio buffer.
>>
>> Raw iio buffer data:
>> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......
>            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
>                               4093587712
>> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............
>                               4143907584
>> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........
>                               So this one looks bad.
>
>> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......
>                               4177473280
>
>> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............
>                               also bad.
>> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
>> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
>> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
>> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
>> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
>> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
>> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
>>
>> Converted value
>I guess this is different data as doesn't seem to line up with the above?
>
>> 0.015625 -0.009277 1.024411 589929
>> 0.015869 -0.009521 1.040769 4294901719
>> 0.020508 -0.008301 1.025632 458712
>> 0.018799 -0.006836 1.032956 851960
>> 0.019287 -0.009521 1.033201 4294836275
>> 0.015625 -0.010498 1.031003 4293328982
>> 0.015137 -0.010498 1.031980 4293853176
>> 0.015869 -0.009521 1.031492 4293722141
>> 0.018555 -0.011475 1.033445 4294311886
>>
>> The 3 acc values is correct in buffer.  Nevertheless, invalid timestamp. The
>timestamp is actually the value of the gyroscope, which directly followed by acc
>values.
>> If we enable the gyroX, gyroY, and gyroZ from userspace, then all the data is
>correct. Since the gyro values are the last 3 values and flowed by timestamp.
>
>Ok. That's odd and we should debug that.  This code is used in a lot of drivers
>so if it is not working correctly we need to figure out why asap and fix it.
>




>However, I'm not seeing what looks to be gyro data in bytes 8-15 etc It isn't the
>stable sequence we'd expect for a timestamp though some specific values
>might be plausible.
>
>Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS() is the wrong
>size.  That should not include channel space for the timestamp. That should
>make it too big though which shouldn't be a problem.
>Also wrong type - should be using __le16 not s16 for the buffer elements given
>your channel declarations.
>
>Please could you add a print to your code alongside the
>iio_push_buffer_with_timestamp() to verify that the value in the pf-
>>timestamp is reasonable looking for a timestamp.
>
>For reference this is the code that handles the timestamp entry creation in the
>demux tables.
>https://elixir.b/
>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio-
>buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cf0
>9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee5841
>0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3d8
>eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
>oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2guA
>iH3J9jBRd0%2Bj6UmcmgyhtBCuKK1HE0%3D&reserved=0
>
>Jonathan
>
>
>>
>> Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 },
>the iio core is able to correct handle the enabled channel data, but not the
>timestamp.
>> The working solution for now is that our driver takes care and just copys the
>enabled channel value to buffer without using available_scan_masks.
>>
>> >So the aim is that userspace never knows anything about this.  Just
>> >set what channels you want and get that data.
>> >
>> >Jonathan
>> >
>> >
>> >>
>> >> >
>> >> >The handling the core code is reasonably sophisticated and will
>> >> >use bulk copying where appropriate.
>> >> >
>> >> >If there is a strong reason to not use that, add a comment here so
>> >> >we don't have anyone 'fix' this code in future.
>> >> >
>> >> >> +      }
>> >> >> +
>> >> >> +      iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
>> >> >> +pf->timestamp);
>> >> >> +
>> >> >> +out:
>> >> >> +      iio_trigger_notify_done(indio_dev->trig);
>> >> >> +
>> >> >> +      return IRQ_HANDLED;
>> >> >> +}
>> >>
>>

Powered by blists - more mailing lists