| Message ID | 8f765cfd6e93fad4e755dd95d709b7bea2a388e2.1744718916.git.marcelo.schmitt@analog.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad4000: Avoid potential double data word read | expand |
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); ?
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.
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.
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.
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.
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>
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
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(-) base-commit: 1c2409fe38d5c19015d69851d15ba543d1911932