> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Sent: Thursday, April 15, 2021 9:54 AM
> To: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>
> Subject: RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer
>
> [External]
>
>
>
> > -----Original Message-----
> > From: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>
> > Sent: Wednesday, April 14, 2021 9:29 AM
> > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> > Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; Jonathan Cameron
> > <jic23@xxxxxxxxxx>; Hennerich, Michael
> > <Michael.Hennerich@xxxxxxxxxx>; Lars-Peter Clausen
> > <lars@xxxxxxxxxx>
> > Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> >
> > [External]
> >
> > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@xxxxxxxxxx>
> > wrote:
> > >
> > > In case 'spi_sync()' fails, we would be left with a max spi transfer
> > > which is not the one the user expects it to be. Hence, we need to
> re-
> > set
> > > it also in this error path.
> > >
> > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > ---
> > > drivers/iio/imu/adis16475.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/imu/adis16475.c
> b/drivers/iio/imu/adis16475.c
> > > index 51b76444db0b..9dca7e506200 100644
> > > --- a/drivers/iio/imu/adis16475.c
> > > +++ b/drivers/iio/imu/adis16475.c
> > > @@ -1067,8 +1067,10 @@ static irqreturn_t
> > adis16475_trigger_handler(int irq, void *p)
> > > adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
> > >
> > > ret = spi_sync(adis->spi, &adis->msg);
> >
> > Purely stylistic here.
> > But, the restore from the cached variable could be done here in a
> > single line.
> > So. just moving [1] here.
>
> You mean also doing it in the label? I thought about that and the
> reason
> why I didn't is that on a normal run, I want to reset the max freq as
> soon
> as possible so that if someone concurrently tries to read 'direct mode'
> attrs
> gets the max freq. This was my reasoning but I admit that it's not that
> important so I will leave this to Jonathan's preference...
>
> Hmm now that I spoke about the concurrently access to IIO attr and
> being paranoid about
> the compiler, I wonder if we should not use
> WRITE_ONCE(adis->spi->max_speed_hz,
> ADIS16475_BURST_MAX_SPEED)...
Hmmm, actually WRITE_ONCE would not be any help since the spi core
does not use READ_ONCE. So, if we are going to be paranoid about the
compiler and load/store tearing, I guess the only safe way here is to
acquire the adis lock [btw, I'm a bit paranoid with this stuff :)]...
Anyways, arguably the likelihood for this to happen is really, really small...
