[PATCH] iio: adc: ad4000: Avoid potential double data word read

[PATCH] iio: adc: ad4000: Avoid potential double data word read

[Marcelo Schmitt]

Currently, SPI-Engine offload module always sends 32-bit data elements to
DMA engine. Appropriately, when set for SPI offloading, the IIO driver uses
32 storagebits for IIO ADC channel buffer elements. However, setting SPI
transfer length according to storagebits (32-bits in case of offload) can
lead to unnecessarily long transfers for ADCs that are 16-bit or less
precision. Adjust AD4000 single-shot read to run transfers of 2 bytes when
that is enough to get all ADC data bits.

Fixes: d0dba3df842f ("iio: adc: ad4000: Add support for SPI offload")
Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
After enough sleep and some time not looking at this driver I finally realize
the potential issue David was talking about. Although I didn't see any clearly
wrong reading when testing it last week, I'm adding the fixes tag since it's
probably easier to drop the tag than to go fetch the commit log.
Also adding a suggested-by tag.

Thanks,
Marcelo

 drivers/iio/adc/ad4000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
index e69a9d2a3e8c..5813db28510d 100644
--- a/drivers/iio/adc/ad4000.c
+++ b/drivers/iio/adc/ad4000.c
@@ -941,7 +941,7 @@ static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
 	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
 
 	xfers[1].rx_buf = &st->scan.data;
-	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+	xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
 
 	/*
 	 * If the device is set up for SPI offloading, IIO channel scan_type is

base-commit: 1c2409fe38d5c19015d69851d15ba543d1911932
-- 
2.47.2

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[Andy Shevchenko]

On Tue, Apr 15, 2025 at 3:21 PM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Currently, SPI-Engine offload module always sends 32-bit data elements to
> DMA engine. Appropriately, when set for SPI offloading, the IIO driver uses
> 32 storagebits for IIO ADC channel buffer elements. However, setting SPI
> transfer length according to storagebits (32-bits in case of offload) can
> lead to unnecessarily long transfers for ADCs that are 16-bit or less
> precision. Adjust AD4000 single-shot read to run transfers of 2 bytes when
> that is enough to get all ADC data bits.

...

>         xfers[1].rx_buf = &st->scan.data;
> -       xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +       xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;

But wouldn't be logical to have

       xfers[1].len = BITS_TO_BYTES(chan->scan_type.realbits);

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[David Lechner]

On 4/15/25 1:02 PM, Andy Shevchenko wrote:
> On Tue, Apr 15, 2025 at 3:21 PM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
>>
>> Currently, SPI-Engine offload module always sends 32-bit data elements to
>> DMA engine. Appropriately, when set for SPI offloading, the IIO driver uses
>> 32 storagebits for IIO ADC channel buffer elements. However, setting SPI
>> transfer length according to storagebits (32-bits in case of offload) can
>> lead to unnecessarily long transfers for ADCs that are 16-bit or less
>> precision. Adjust AD4000 single-shot read to run transfers of 2 bytes when
>> that is enough to get all ADC data bits.
> 
> ...
> 
>>         xfers[1].rx_buf = &st->scan.data;
>> -       xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
>> +       xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
> 
> But wouldn't be logical to have
> 
>        xfers[1].len = BITS_TO_BYTES(chan->scan_type.realbits);
> 
> ?
> 

No, SPI expects 1, 2 or 4 bytes, never 3. If realbits is 18, we
need len = 4.

It would have to be:

	xfers[1].len = roundup_pow_of_two(BITS_TO_BYTES(chan->scan_type.realbits));

But that gets too long for 1 line, so I prefer what Marcelo wrote.

Maybe an idea for another day:

#define SPI_LEN_FOR_BITS(bits) roundup_pow_of_two(BITS_TO_BYTES(bits))

There are a couple of places in spi/ that could use this and several
iio drivers.

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 12:22 AM David Lechner <dlechner@baylibre.com> wrote:
> On 4/15/25 1:02 PM, Andy Shevchenko wrote:
> > On Tue, Apr 15, 2025 at 3:21 PM Marcelo Schmitt
> > <marcelo.schmitt@analog.com> wrote:

...

> >>         xfers[1].rx_buf = &st->scan.data;
> >> -       xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> >> +       xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
> >
> > But wouldn't be logical to have
> >
> >        xfers[1].len = BITS_TO_BYTES(chan->scan_type.realbits);
> >
> > ?
>
> No, SPI expects 1, 2 or 4 bytes, never 3. If realbits is 18, we
> need len = 4.
>
> It would have to be:
>
>         xfers[1].len = roundup_pow_of_two(BITS_TO_BYTES(chan->scan_type.realbits));
>
> But that gets too long for 1 line

Actually there are a handful of drivers including SPI core that need
that helper already, I would prefer to have a helper defined in spi.h.

, so I prefer what Marcelo wrote.
>
> Maybe an idea for another day:
>
> #define SPI_LEN_FOR_BITS(bits) roundup_pow_of_two(BITS_TO_BYTES(bits))

Right, but as static inline to have stricter types.

> There are a couple of places in spi/ that could use this and several
> iio drivers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 8:59 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Apr 16, 2025 at 12:22 AM David Lechner <dlechner@baylibre.com> wrote:
> > On 4/15/25 1:02 PM, Andy Shevchenko wrote:
> > > On Tue, Apr 15, 2025 at 3:21 PM Marcelo Schmitt
> > > <marcelo.schmitt@analog.com> wrote:

...

> > >>         xfers[1].rx_buf = &st->scan.data;
> > >> -       xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> > >> +       xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
> > >
> > > But wouldn't be logical to have
> > >
> > >        xfers[1].len = BITS_TO_BYTES(chan->scan_type.realbits);
> > >
> > > ?
> >
> > No, SPI expects 1, 2 or 4 bytes, never 3. If realbits is 18, we
> > need len = 4.
> >
> > It would have to be:
> >
> >         xfers[1].len = roundup_pow_of_two(BITS_TO_BYTES(chan->scan_type.realbits));
> >
> > But that gets too long for 1 line
>
> Actually there are a handful of drivers including SPI core that need
> that helper already, I would prefer to have a helper defined in spi.h.
>
> , so I prefer what Marcelo wrote.
> >
> > Maybe an idea for another day:
> >
> > #define SPI_LEN_FOR_BITS(bits) roundup_pow_of_two(BITS_TO_BYTES(bits))
>
> Right, but as static inline to have stricter types.
>
> > There are a couple of places in spi/ that could use this and several
> > iio drivers.

Or even wider, in bitops.h / bit*.h somewhere. Let me craft a patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 09:01:02AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 8:59 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Apr 16, 2025 at 12:22 AM David Lechner <dlechner@baylibre.com> wrote:
> > > On 4/15/25 1:02 PM, Andy Shevchenko wrote:

...

> > > It would have to be:
> > >
> > >         xfers[1].len = roundup_pow_of_two(BITS_TO_BYTES(chan->scan_type.realbits));
> > >
> > > But that gets too long for 1 line
> >
> > Actually there are a handful of drivers including SPI core that need
> > that helper already, I would prefer to have a helper defined in spi.h.
> >
> > , so I prefer what Marcelo wrote.
> > >
> > > Maybe an idea for another day:
> > >
> > > #define SPI_LEN_FOR_BITS(bits) roundup_pow_of_two(BITS_TO_BYTES(bits))
> >
> > Right, but as static inline to have stricter types.
> >
> > > There are a couple of places in spi/ that could use this and several
> > > iio drivers.
> 
> Or even wider, in bitops.h / bit*.h somewhere. Let me craft a patch.

Just sent a mini-series, you are in Cc there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[Jonathan Cameron]

On Wed, 16 Apr 2025 09:47:03 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, Apr 16, 2025 at 09:01:02AM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 16, 2025 at 8:59 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:  
> > > On Wed, Apr 16, 2025 at 12:22 AM David Lechner <dlechner@baylibre.com> wrote:  
> > > > On 4/15/25 1:02 PM, Andy Shevchenko wrote:  
> 
> ...
> 
> > > > It would have to be:
> > > >
> > > >         xfers[1].len = roundup_pow_of_two(BITS_TO_BYTES(chan->scan_type.realbits));
> > > >
> > > > But that gets too long for 1 line  
> > >
> > > Actually there are a handful of drivers including SPI core that need
> > > that helper already, I would prefer to have a helper defined in spi.h.
> > >
> > > , so I prefer what Marcelo wrote.  
> > > >
> > > > Maybe an idea for another day:
> > > >
> > > > #define SPI_LEN_FOR_BITS(bits) roundup_pow_of_two(BITS_TO_BYTES(bits))  
> > >
> > > Right, but as static inline to have stricter types.
> > >  
> > > > There are a couple of places in spi/ that could use this and several
> > > > iio drivers.  
> > 
> > Or even wider, in bitops.h / bit*.h somewhere. Let me craft a patch.  
> 
> Just sent a mini-series, you are in Cc there.
> 

I've picked this one up for now.  Assuming we'll cycle round to use the new
helper once it's available in my upstream.

Thanks,

Jonathan

Re: [PATCH] iio: adc: ad4000: Avoid potential double data word read

[David Lechner]

On 4/15/25 7:21 AM, Marcelo Schmitt wrote:
> Currently, SPI-Engine offload module always sends 32-bit data elements to
> DMA engine. Appropriately, when set for SPI offloading, the IIO driver uses
> 32 storagebits for IIO ADC channel buffer elements. However, setting SPI
> transfer length according to storagebits (32-bits in case of offload) can
> lead to unnecessarily long transfers for ADCs that are 16-bit or less
> precision. Adjust AD4000 single-shot read to run transfers of 2 bytes when
> that is enough to get all ADC data bits.
> 
> Fixes: d0dba3df842f ("iio: adc: ad4000: Add support for SPI offload")
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---

Reviewed-by: David Lechner <dlechner@baylibre.com>