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]

Message-ID: <20240603203003.GA444780@vamoiridPC>
Date: Mon, 3 Jun 2024 22:30:03 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, 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, Jun 03, 2024 at 08:25:37PM +0100, Jonathan Cameron wrote:
> On Sun, 2 Jun 2024 21:30:23 +0200
> Vasileios Amoiridis <vassilisamir@...il.com> wrote:
> 
> > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote:
> > > 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.
> > >   
> > 
> > What about the I2C implementation though? I watched recently a video
> > from Wolfram Sang [1] and as far as I understood, the buffers are not
> > provided by the I2C API, but you have to provide them. In any case, I
> > should maybe check both SPI and I2C reads to understand the internals.
> > 
> > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s
> > 
> 
> I'm not sure Wolfram got far with his desire for generally avoiding the
> bounce buffers for i2c.  I think it's strictly opt in only so don't opt in
> unless your code is safe for it and regmap never will by default as too
> many drivers will be subtly broken.
> 

The things that I found about DMA "safety" in I2C are [1] and [2] so I think
that the IIO_DMA_MINALIGN should remain because in the future, in case it's
needed for triggered buffers to do buffer reads from the volatile registers
of the device, then it might be a problem for I2C. 

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L2627
[2]: https://elixir.bootlin.com/linux/latest/source/include/linux/i2c.h#L92

> 
> > > > 
> > > > [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.
> > >   
> > 
> > Same comment. What about I2C?
> > 
> > > >  };
> > > >  
> > > >  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 me it's the same, I could change it to what you proposed, no problem!
> > 
> > Cheers,
> > Vasilis
> > 
> > > 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;
> > > >  	}  
> > >   
>