Re: [PATCH 2/3] iio: accel: Add support for ICM42370P

[Jonathan Cameron]

> > +/**
> > + * struct inv_icm42370_data - driver state variables
> > + * @lock:		lock for serializing multiple register access.
> > + * @name:		chip name.
> > + * @map:		regmap pointer.
> > + * @vdd_supply:		VDD voltage regulator for the chip.
> > + * @vddio_supply:	I/O voltage regulator for the chip.
> > + * @indio_accel:	accelerometer IIO device.
> > + * @sensor_state:	per-sensor state tracking (e.g. power, ODR).
> > + * @buffer:		buffer for reading data registers, aligned for DMA.
> > + * @accel_calibbias:	accelerometer calibration bias for X, Y, and Z axes.
> > + * @fifo:		FIFO state and configuration.
> > + * @timestamp:		interrupt timestamp.
> > + * @chip:		chip identifier.
> > + * @conf:		chip sensors configurations.
> > + */
> > +struct inv_icm42370_data {  
> There is another 'state' struct declared below which leaves us with two types of
> state structs for icm42370? Would it make sense to have them merged?
> 
> > +	struct mutex lock;
> > +	const char *name;
> > +	struct regmap *map;
> > +	struct regulator *vdd_supply;
> > +	struct regulator *vddio_supply;  
> I'm not seeing the regulators being used after probe. For these power supplies,
> devm_regulator_get_enable() is usually enough to get them running. If that
> applies to icm42370, then there will be no need for vdd fields in the state struct.
> 
> > +	struct iio_dev *indio_accel;
> > +	struct inv_icm42370_sensor_state *sensor_state;
> > +	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);  
> The __aligned() pragma forces cache line alignment on the field it annotates.
> IIO device drivers often have annotations like that to make data buffers use
> separate cache lines and thus avoid data mishandling when doing DMA.
> Though, if the alignment is forced closer to the beginning of the struct, we
> may end up with holes in memory. This is not a subject I specialize in, though,
> you may find this LWN article better than my poor explanation.
> https://lwn.net/Articles/335942/
> 
> Besides avoiding holes in data structures, running pahole may also help you
> better understand the alignment issue.

Marcelo, you indeed highlighted a problem but not quite the reason it is
a problem.  __aligned(IIO_DMA_MINALIGN) forces the alignment of the beginning
of the buffer to be on a cacheline.  It doesn't do anything about the next
element.  The aim here is to ensure that any data accesses by DMA doesn't
share a cacheline with data that may be accessed concurrently by the CPU.
It seems very likely some of the stuff that follows might be accessed
at that time and hence potentially corrupted by stale values coming back
from the DMA engine which thinks it has exclusive control of the cacheline.

Hence put them at the end of the structure not in the middle ensures there
is nothing in that space.  Structures are padded to a multiple of the
highest aligned element.

I just took a quick glance at existing comments to see where things
stood before looking at v2 and this jumped out at me

Jonathan

> 
> > +	s16 accel_calibbias[3];
> > +	struct inv_icm42370_fifo fifo;
> > +	s64 timestamp;
> > +	enum inv_icm42370_chip chip;
> > +	struct inv_icm42370_conf conf;
> > +};