linux-kernel - Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

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]

Date: Sun, 2 Jun 2024 13:57:26 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: lars@...afoo.de, himanshujha199640@...il.com, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for
 calibration data

On Mon, 27 May 2024 20:37:59 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> Calibration data are located in contiguous-ish registers
> inside the chip. For that reason we can use bulk reads as is
> done as well in the BME68x Sensor API [1].
> 
> The arrays that are used for reading the data out of the sensor
> are located inside DMA safe buffer.

See below. I think in this case that isn't necessary.
However it's a quirk of how the custom regmap works. Whilst
we can't rely on regmap core spi implementations continuing to
bounce buffer, we can rely on one local to our particular driver.

> 
> [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>


> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 681f271f9b06..ed4cdb4d64af 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c

> +
>  struct bme680_calib {
>  	u16 par_t1;
>  	s16 par_t2;
> @@ -64,6 +109,16 @@ struct bme680_data {
>  	 * and humidity compensation calculations.
>  	 */
>  	s32 t_fine;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) may require the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	union {
> +		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> +		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> +		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> +	} __aligned(IIO_DMA_MINALIGN);
Ah! I should have read ahead.  I don't think you need this alignment forcing
because bme680_regmap_spi_read uses spi_write_then_read() which always
bounces the data.

>  };
>  
>  static const struct regmap_range bme680_volatile_ranges[] = {
> @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
>  			     struct bme680_calib *calib)
>  {


> +	calib->par_h3 = data->bme680_cal_buf_2[H3];
> +	calib->par_h4 = data->bme680_cal_buf_2[H4];
> +	calib->par_h5 = data->bme680_cal_buf_2[H5];
> +	calib->par_h6 = data->bme680_cal_buf_2[H6];
> +	calib->par_h7 = data->bme680_cal_buf_2[H7];
> +	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> +	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> +	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> +	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
>  
> -	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> +	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> +			       &data->bme680_cal_buf_3[0],
This one is always debated, but personally I'd prefer
				data->bme680_cal_buf_3,

for cases like this. Up to you though.
> +			       sizeof(data->bme680_cal_buf_3));
>  	if (ret < 0) {
> -		dev_err(dev, "failed to read BME680_H7_REG\n");
> +		dev_err(dev, "failed to read 3rd set of calib data;\n");
>  		return ret;
>  	}