AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

["Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>]

Hi Jonathan,

let's think about a typical IMU, which has 6 2-byte channels (acc x,y,z  gyro x,y,z)	

3 2-byte channel we want, 3 2-byte channels we don't a 4-byte gap and an 8-byte timestamp struct scan {
	u16 wanted[3];
	u16 notwanted[3];
	... gap...
	aligned_s64 timestamp;
	
Hint: indio_dev->scan_bytes is 24, if we use available_scan_mask with all channels set (ref. https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L975)

Cutting down to the parts that change in_loc only.

	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
		in_ind = find_next_bit(indio_dev->active_scan_mask,
				       masklength, in_ind + 1);
		while (in_ind != out_ind) {
... length is the length of current channel .. We never enter here in the example.
			/* Make sure we are aligned */
			in_loc = roundup(in_loc, length) + length;

			in_ind = find_next_bit(indio_dev->active_scan_mask,
					       masklength, in_ind + 1);
		}

... length is the length of the current channel.  Get here on first hit.

		in_loc = roundup(in_loc, length);

		in_loc += length;
.. in loc = 2 + 2 + 2 = 6
	}
	/* Relies on scan_timestamp being last */
	if (buffer->scan_timestamp) {

... length is 8 

		in_loc = roundup(in_loc, length);
.. in_loc = 8 (should be 16 to match timestamp offset: https://elixir.bootlin.com/linux/v6.15.1/source/include/linux/iio/buffer.h#L37)
		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);

	}

Best Regards

Jianping Shen

>
>> Hi Jonathan,
>>
>> we find out the reason why the timestamp is invalid in the iio buffer.
>>
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix
>> ir.bootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrial
>> io-
>buffer.c%23L1093&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd
>84c
>>
>234178ee4bae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f
>4%7C0%7C
>>
>0%7C638889664948004786%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1
>hcGkiOnRydWU
>>
>sIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3
>D%
>>
>7C0%7C%7C%7C&sdata=DAYIrdjPq4RrrvH7tudEjOlAavn%2B2qlpGiRp2UXTh2
>c%3D&re
>> served=0
>>
>> 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;
>
>That looks correct, but I'm not seeing why the roundup above doesn't land us
>in the same place.  I'm not that keen on handling the timestamp even more
>differently to other channels.
>
>
>If we imagine an active scan with
>2-byte chanel we want, 2 2-byte channels we don't a 2-byte gap and an 8-byte
>timestamp struct scan {
>	u16 wanted;
>	u16 notwanted[2];
>	... gap...
>	aligned_s64 timestamp;
>
>
>
>Cutting down to the parts that change in_loc only.
>
>	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
>		in_ind = find_next_bit(indio_dev->active_scan_mask,
>				       masklength, in_ind + 1);
>		while (in_ind != out_ind) {
>... length is the length of current channel .. We never enter here in the example.
>			/* Make sure we are aligned */
>			in_loc = roundup(in_loc, length) + length;
>
>			in_ind = find_next_bit(indio_dev->active_scan_mask,
>					       masklength, in_ind + 1);
>		}
>
>... length is the length of the current channel.  Get here on first hit.
>
>		in_loc = roundup(in_loc, length);
>
>		in_loc += length;
>.. in loc = 2
>	}
>	/* Relies on scan_timestamp being last */
>	if (buffer->scan_timestamp) {
>
>... length is 8 ...
>
>		in_loc = roundup(in_loc, length);
>.. I think in_lock = 8?
>		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
>
>	}
>
>Perhaps you can talk through the example that is failing?
>
>>
>> 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?
>
>Fine to send the proposed fix but first we need to step through why the current
>code isn't working.
>
>
>Thanks,
>
>Jonathan
>
>>
>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli
>>
>>xir.b%2F&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd84c2341
>78ee4b
>>
>>ae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C
>0%7C63888
>>
>>9664948038159%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy
>dWUsIlYiOiI
>>
>>wLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7
>C0%7C%
>>
>>7C%7C&sdata=7kNAXwi9fkp5XocdJ2K5W2Cp9%2BQW4Wq6GLr5reGqies%3
>D&reserved
>> >=0
>>
>>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio-
>>
>>buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cf
>0
>>
>>9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee584
>1
>>
>>0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3d
>8
>>
>>eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
>j
>>
>>oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2gu
>A
>> >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;
>> >> >> >> +}
>> >> >>
>> >>
>>
>>