linux-kernel - Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87wmicpvso.fsf@justinweiss.com>
Date: Sat, 12 Oct 2024 19:43:19 -0700
From: Justin Weiss <justin@...tinweiss.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alex Lanzano <lanzano.alex@...il.com>,  Lars-Peter Clausen
 <lars@...afoo.de>,  linux-iio@...r.kernel.org,
  linux-kernel@...r.kernel.org,  "Derek J . Clark"
 <derekjohn.clark@...il.com>,  Philip Müller
 <philm@...jaro.org>
Subject: Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

Jonathan Cameron <jic23@...nel.org> writes:

> On Fri, 11 Oct 2024 08:37:48 -0700
> Justin Weiss <justin@...tinweiss.com> wrote:
>
>> Set up a triggered buffer for the accel and angl_vel values.
>> 
>> Signed-off-by: Justin Weiss <justin@...tinweiss.com>
> Hi Justin
>
> A few suggestions inline. Other than the DMA safe buffer thing, looks good
> but you might want to consider using a single bulk read.
>
> My cynical view is that if someone paid for an IMU they probably want all
> the channels, so optimizing for that case is a good plan.
>
>> ---
>>  drivers/iio/imu/bmi270/Kconfig       |  1 +
>>  drivers/iio/imu/bmi270/bmi270.h      |  8 +++++
>>  drivers/iio/imu/bmi270/bmi270_core.c | 47 ++++++++++++++++++++++++++++
>>  3 files changed, 56 insertions(+)
>> 
>> diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
>> index 0ffd29794fda..6362acc706da 100644
>> --- a/drivers/iio/imu/bmi270/Kconfig
>> +++ b/drivers/iio/imu/bmi270/Kconfig
>> @@ -6,6 +6,7 @@
>>  config BMI270
>>  	tristate
>>  	select IIO_BUFFER
>
> Hmm. The IIO_BUFFER select shouldn't have been here as no obvious use
> in the driver. Ah well - this patch 'fixes' that :)
>
>> +	select IIO_TRIGGERED_BUFFER
>>  
>>  config BMI270_I2C
>>  	tristate "Bosch BMI270 I2C driver"
>> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
>> index 51e374fd4290..335400c34b0d 100644
>> --- a/drivers/iio/imu/bmi270/bmi270.h
>> +++ b/drivers/iio/imu/bmi270/bmi270.h
>> @@ -11,6 +11,14 @@ struct bmi270_data {
>>  	struct device *dev;
>>  	struct regmap *regmap;
>>  	const struct bmi270_chip_info *chip_info;
>> +
>> +	/*
>> +	 * Ensure natural alignment for timestamp if present.
>> +	 * Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts.
>> +	 * If fewer channels are enabled, less space may be needed, as
>> +	 * long as the timestamp is still aligned to 8 bytes.
>> +	 */
>> +	__le16 buf[12] __aligned(8);
>>  };
>>  
>>  enum bmi270_device_type {
>> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
>> index e5ee80c12166..f49db5d1bffd 100644
>> --- a/drivers/iio/imu/bmi270/bmi270_core.c
>> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
>> @@ -7,6 +7,8 @@
>>  #include <linux/regmap.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>>  
>>  #include "bmi270.h"
>>  
>> @@ -66,6 +68,7 @@ enum bmi270_scan {
>>  	BMI270_SCAN_GYRO_X,
>>  	BMI270_SCAN_GYRO_Y,
>>  	BMI270_SCAN_GYRO_Z,
>> +	BMI270_SCAN_TIMESTAMP,
>>  };
>>  
>>  const struct bmi270_chip_info bmi270_chip_info[] = {
>> @@ -82,6 +85,29 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
>>  };
>>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
>>  
>> +static irqreturn_t bmi270_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct bmi270_data *bmi270_device = iio_priv(indio_dev);
>> +	int i, ret, j = 0, base = BMI270_ACCEL_X_REG;
>> +	__le16 sample;
>> +
>> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
>> +		ret = regmap_bulk_read(bmi270_device->regmap,
>> +				       base + i * sizeof(sample),
>> +				       &sample, sizeof(sample));
>
> This is always a fun corner.
> regmap doesn't guarantee to bounce buffer the data used by the underlying
> transport. In the case of SPI that means we need a DMA safe buffer for bulk
> accesses.  In practice it may well bounce the data today but there are optmizations
> that would make it zero copy that might get applied in future.
>
> Easiest way to do that is put your sample variable in the iio_priv structure
> at the end and mark it __aligned(IIO_DMA_MINALIGN)
>
> Given you are reading a bunch of contiguous registers here it may well make
> sense to do a single bulk read directly into buf and then use
> the available_scan_masks to let the IIO core know it always gets a full set
> of samples. Then if the user selects a subset the IIO core will reorganize
> the data that they get presented with.

That's convenient :-)

It should make this much simpler. To clarify, I'll use regmap_bulk_read
to read all of the registers at once into a stack-allocated buffer, and
then push that buffer. Then I can remove bmi270_device->buf entirely,
and avoid the DMA problem that way.

Then I'll provide one avail_mask that sets all of the
BMI270_SCAN_ACCEL_* and BMI270_SCAN_GYRO_* bits.

> Whether that makes sense from a performance point of view depends on
> the speed of the spi transfers vs the cost of setting up the individual ones.
>
> You could optimize contiguous reads in here, but probably not worth that
> complexity.
>
>> +		if (ret)
>> +			goto done;
>> +		bmi270_device->buf[j++] = sample;
>
> It's not a huge buffer and you aren't DMAing into it, so maybe just put this
> on the stack?
>
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
>> +done:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +	return IRQ_HANDLED;
>> +}