Add a DMA-safe buffer and use it for spi_read() instead of a stack
memory. All SPI buffers must be DMA-safe.
Since we only need up to 3 bytes, we just use a u8[] instead of __be16
and __be32 and change the conversion functions appropriately.
Fixes: 4d671b71beef ("iio: adc: ti-adc161s626: add support for TI 1-channel differential ADCs")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
drivers/iio/adc/ti-adc161s626.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
index 1d427548e0b3..6416d6a7ada0 100644
--- a/drivers/iio/adc/ti-adc161s626.c
+++ b/drivers/iio/adc/ti-adc161s626.c
@@ -70,6 +70,7 @@ struct ti_adc_data {
u8 read_size;
u8 shift;
+ u8 buf[3] __aligned(IIO_DMA_MINALIGN);
};
static int ti_adc_read_measurement(struct ti_adc_data *data,
@@ -78,26 +79,20 @@ static int ti_adc_read_measurement(struct ti_adc_data *data,
int ret;
switch (data->read_size) {
- case 2: {
- __be16 buf;
-
- ret = spi_read(data->spi, (void *) &buf, 2);
+ case 2:
+ ret = spi_read(data->spi, data->buf, 2);
if (ret)
return ret;
- *val = be16_to_cpu(buf);
+ *val = get_unaligned_be16(data->buf);
break;
- }
- case 3: {
- __be32 buf;
-
- ret = spi_read(data->spi, (void *) &buf, 3);
+ case 3:
+ ret = spi_read(data->spi, data->buf, 3);
if (ret)
return ret;
- *val = be32_to_cpu(buf) >> 8;
+ *val = get_unaligned_be24(data->buf);
break;
- }
default:
return -EINVAL;
}
--
2.43.0Hi David,
kernel test robot noticed the following build errors:
[auto build test ERROR on ff0843ceb1fb11a6b73e0e77b932ef7967aecd4b]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ti-adc161s626-fix-buffer-read-on-big-endian/20260315-092711
base: ff0843ceb1fb11a6b73e0e77b932ef7967aecd4b
patch link: https://lore.kernel.org/r/20260314-iio-adc-ti-adc161s626-fix-scan-buf-v1-2-56243b11e87b%40baylibre.com
patch subject: [PATCH 2/2] iio: adc: ti-adc161s626: use DMA-safe memory for spi_read()
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20260327/202603271734.pnWuSmNr-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260327/202603271734.pnWuSmNr-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603271734.pnWuSmNr-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/iio/adc/ti-adc161s626.c: In function 'ti_adc_read_measurement':
>> drivers/iio/adc/ti-adc161s626.c:87:24: error: implicit declaration of function 'get_unaligned_be16' [-Wimplicit-function-declaration]
87 | *val = get_unaligned_be16(data->buf);
| ^~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/ti-adc161s626.c:94:24: error: implicit declaration of function 'get_unaligned_be24' [-Wimplicit-function-declaration]
94 | *val = get_unaligned_be24(data->buf);
| ^~~~~~~~~~~~~~~~~~
vim +/get_unaligned_be16 +87 drivers/iio/adc/ti-adc161s626.c
75
76 static int ti_adc_read_measurement(struct ti_adc_data *data,
77 struct iio_chan_spec const *chan, int *val)
78 {
79 int ret;
80
81 switch (data->read_size) {
82 case 2:
83 ret = spi_read(data->spi, data->buf, 2);
84 if (ret)
85 return ret;
86
> 87 *val = get_unaligned_be16(data->buf);
88 break;
89 case 3:
90 ret = spi_read(data->spi, data->buf, 3);
91 if (ret)
92 return ret;
93
> 94 *val = get_unaligned_be24(data->buf);
95 break;
96 default:
97 return -EINVAL;
98 }
99
100 *val = sign_extend32(*val >> data->shift, chan->scan_type.realbits - 1);
101
102 return 0;
103 }
104
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, Mar 27, 2026 at 05:13:16PM +0800, kernel test robot wrote: > Hi David, > > kernel test robot noticed the following build errors: > All errors (new ones prefixed by >>): > > drivers/iio/adc/ti-adc161s626.c: In function 'ti_adc_read_measurement': > >> drivers/iio/adc/ti-adc161s626.c:87:24: error: implicit declaration of function 'get_unaligned_be16' [-Wimplicit-function-declaration] > 87 | *val = get_unaligned_be16(data->buf); > | ^~~~~~~~~~~~~~~~~~ > >> drivers/iio/adc/ti-adc161s626.c:94:24: error: implicit declaration of function 'get_unaligned_be24' [-Wimplicit-function-declaration] > 94 | *val = get_unaligned_be24(data->buf); > | ^~~~~~~~~~~~~~~~~~ + linux/unaligned.h should fix this. -- With Best Regards, Andy Shevchenko
On 3/27/26 4:44 AM, Andy Shevchenko wrote: > On Fri, Mar 27, 2026 at 05:13:16PM +0800, kernel test robot wrote: >> Hi David, >> >> kernel test robot noticed the following build errors: > > >> All errors (new ones prefixed by >>): >> >> drivers/iio/adc/ti-adc161s626.c: In function 'ti_adc_read_measurement': >>>> drivers/iio/adc/ti-adc161s626.c:87:24: error: implicit declaration of function 'get_unaligned_be16' [-Wimplicit-function-declaration] >> 87 | *val = get_unaligned_be16(data->buf); >> | ^~~~~~~~~~~~~~~~~~ >>>> drivers/iio/adc/ti-adc161s626.c:94:24: error: implicit declaration of function 'get_unaligned_be24' [-Wimplicit-function-declaration] >> 94 | *val = get_unaligned_be24(data->buf); >> | ^~~~~~~~~~~~~~~~~~ > > + linux/unaligned.h should fix this. > Jonathan caught this already [1], so should be fixed already [2]. [1]: https://lore.kernel.org/linux-iio/20260321204506.4b5df02a@jic23-huawei/ [2]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=768461517a28d80fe81ea4d5d03a90cd184ea6ad
On Sat, 14 Mar 2026 18:13:32 -0500
David Lechner <dlechner@baylibre.com> wrote:
> Add a DMA-safe buffer and use it for spi_read() instead of a stack
> memory. All SPI buffers must be DMA-safe.
>
> Since we only need up to 3 bytes, we just use a u8[] instead of __be16
> and __be32 and change the conversion functions appropriately.
>
> Fixes: 4d671b71beef ("iio: adc: ti-adc161s626: add support for TI 1-channel differential ADCs")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> drivers/iio/adc/ti-adc161s626.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
> index 1d427548e0b3..6416d6a7ada0 100644
> --- a/drivers/iio/adc/ti-adc161s626.c
> +++ b/drivers/iio/adc/ti-adc161s626.c
> @@ -70,6 +70,7 @@ struct ti_adc_data {
>
> u8 read_size;
> u8 shift;
> + u8 buf[3] __aligned(IIO_DMA_MINALIGN);
On this. There is new generic infrastructure for marking these.
https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720
https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/
Would look like
__dma_from_device_group_begin();
u8 buf[3];
__dma_from_device_group_end();
Do you think we should adopt them rather than doing our own thing?
Slowly though I don't want the noise of a mass conversion.
As normal, advantage of standard infrastructure is cutting down
in subsystem specific magic.
I 'think' result is the same (though it also forces the trailing padding if anything
comes after this and needs it).
> };
>
> static int ti_adc_read_measurement(struct ti_adc_data *data,
> @@ -78,26 +79,20 @@ static int ti_adc_read_measurement(struct ti_adc_data *data,
> int ret;
>
> switch (data->read_size) {
> - case 2: {
> - __be16 buf;
> -
> - ret = spi_read(data->spi, (void *) &buf, 2);
> + case 2:
> + ret = spi_read(data->spi, data->buf, 2);
> if (ret)
> return ret;
>
> - *val = be16_to_cpu(buf);
> + *val = get_unaligned_be16(data->buf);
> break;
> - }
> - case 3: {
> - __be32 buf;
> -
> - ret = spi_read(data->spi, (void *) &buf, 3);
> + case 3:
> + ret = spi_read(data->spi, data->buf, 3);
> if (ret)
> return ret;
>
> - *val = be32_to_cpu(buf) >> 8;
> + *val = get_unaligned_be24(data->buf);
> break;
> - }
> default:
> return -EINVAL;
> }
>
On 3/16/26 1:31 PM, Jonathan Cameron wrote:
> On Sat, 14 Mar 2026 18:13:32 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
>> Add a DMA-safe buffer and use it for spi_read() instead of a stack
>> memory. All SPI buffers must be DMA-safe.
>>
...
>> diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
>> index 1d427548e0b3..6416d6a7ada0 100644
>> --- a/drivers/iio/adc/ti-adc161s626.c
>> +++ b/drivers/iio/adc/ti-adc161s626.c
>> @@ -70,6 +70,7 @@ struct ti_adc_data {
>>
>> u8 read_size;
>> u8 shift;
>> + u8 buf[3] __aligned(IIO_DMA_MINALIGN);
> On this. There is new generic infrastructure for marking these.
> https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720
> https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/
>
> Would look like
> __dma_from_device_group_begin();
> u8 buf[3];
> __dma_from_device_group_end();
>
> Do you think we should adopt them rather than doing our own thing?
I have mixed thoughts on this.
Pros:
* This would make it more obvious it should be at the end of the struct
but doesn't hurt if it isn't.
Cons:
* It is more verbose.
* There doesn't seem to be __dma_to_device_group_begin(), so it isn't
clear what we should do for tx buffers.
> Slowly though I don't want the noise of a mass conversion.
>
> As normal, advantage of standard infrastructure is cutting down
> in subsystem specific magic.
>
> I 'think' result is the same (though it also forces the trailing padding if anything
> comes after this and needs it).
On Sat, 21 Mar 2026 14:27:10 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On 3/16/26 1:31 PM, Jonathan Cameron wrote:
> > On Sat, 14 Mar 2026 18:13:32 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >
> >> Add a DMA-safe buffer and use it for spi_read() instead of a stack
> >> memory. All SPI buffers must be DMA-safe.
> >>
>
> ...
>
> >> diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
> >> index 1d427548e0b3..6416d6a7ada0 100644
> >> --- a/drivers/iio/adc/ti-adc161s626.c
> >> +++ b/drivers/iio/adc/ti-adc161s626.c
> >> @@ -70,6 +70,7 @@ struct ti_adc_data {
> >>
> >> u8 read_size;
> >> u8 shift;
> >> + u8 buf[3] __aligned(IIO_DMA_MINALIGN);
> > On this. There is new generic infrastructure for marking these.
> > https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720
> > https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/
> >
> > Would look like
> > __dma_from_device_group_begin();
> > u8 buf[3];
> > __dma_from_device_group_end();
> >
> > Do you think we should adopt them rather than doing our own thing?
>
> I have mixed thoughts on this.
>
> Pros:
> * This would make it more obvious it should be at the end of the struct
> but doesn't hurt if it isn't.
> Cons:
> * It is more verbose.
> * There doesn't seem to be __dma_to_device_group_begin(), so it isn't
> clear what we should do for tx buffers.
Agreed. Should add that. Though it would then imply that we should
treat them differently. Hopefully my understanding that we don't need
tx and rx to live in their own cachelines isn't wrong (at least
for all the stuff we care about)!
Let's let this sit for a cycle or two and see how adoption of those
new toys goes. Longer term I'd generally like to get rid of as
much that makes us special as possible.
>
> > Slowly though I don't want the noise of a mass conversion.
> >
> > As normal, advantage of standard infrastructure is cutting down
> > in subsystem specific magic.
> >
> > I 'think' result is the same (though it also forces the trailing padding if anything
> > comes after this and needs it).
>
On Mon, Mar 16, 2026 at 06:31:17PM +0000, Jonathan Cameron wrote: > On Sat, 14 Mar 2026 18:13:32 -0500 > David Lechner <dlechner@baylibre.com> wrote: ... > > u8 shift; > > + u8 buf[3] __aligned(IIO_DMA_MINALIGN); > On this. There is new generic infrastructure for marking these. > https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720 > https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/ > > Would look like > __dma_from_device_group_begin(); > u8 buf[3]; > __dma_from_device_group_end(); > > Do you think we should adopt them rather than doing our own thing? > Slowly though I don't want the noise of a mass conversion. > > As normal, advantage of standard infrastructure is cutting down > in subsystem specific magic. > > I 'think' result is the same (though it also forces the trailing padding if anything > comes after this and needs it). As I read it it will be an equivalent to u8 shift; __aligned(IIO_DMA_MINALIGN); u8 buf[3] __aligned(IIO_DMA_MINALIGN); -- With Best Regards, Andy Shevchenko
On Mon, 16 Mar 2026 21:53:30 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Mon, Mar 16, 2026 at 06:31:17PM +0000, Jonathan Cameron wrote: > > On Sat, 14 Mar 2026 18:13:32 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > ... > > > > u8 shift; > > > + u8 buf[3] __aligned(IIO_DMA_MINALIGN); > > On this. There is new generic infrastructure for marking these. > > https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720 > > https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/ > > > > Would look like > > __dma_from_device_group_begin(); > > u8 buf[3]; > > __dma_from_device_group_end(); > > > > Do you think we should adopt them rather than doing our own thing? > > Slowly though I don't want the noise of a mass conversion. > > > > As normal, advantage of standard infrastructure is cutting down > > in subsystem specific magic. > > > > I 'think' result is the same (though it also forces the trailing padding if anything > > comes after this and needs it). > > As I read it it will be an equivalent to > > u8 shift; __aligned(IIO_DMA_MINALIGN); > u8 buf[3] __aligned(IIO_DMA_MINALIGN); I think it ends up as u8 shift; //unmoved __u8 something[0] __aligned(IIO_DMA_MINALIGN); u8 buf[3]; __u8 something_else[0] __aligned(IIO_DMA_MINALING); Who doesn't like forcing alignment on 0 sized things. I assume someone who is more familiar with the C spec than I am established that works. > >
On 3/16/26 2:53 PM, Andy Shevchenko wrote: > On Mon, Mar 16, 2026 at 06:31:17PM +0000, Jonathan Cameron wrote: >> On Sat, 14 Mar 2026 18:13:32 -0500 >> David Lechner <dlechner@baylibre.com> wrote: > > ... > >>> u8 shift; >>> + u8 buf[3] __aligned(IIO_DMA_MINALIGN); >> On this. There is new generic infrastructure for marking these. >> https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720 >> https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/ >> >> Would look like >> __dma_from_device_group_begin(); >> u8 buf[3]; >> __dma_from_device_group_end(); >> >> Do you think we should adopt them rather than doing our own thing? >> Slowly though I don't want the noise of a mass conversion. >> >> As normal, advantage of standard infrastructure is cutting down >> in subsystem specific magic. >> >> I 'think' result is the same (though it also forces the trailing padding if anything >> comes after this and needs it). > > As I read it it will be an equivalent to > > u8 shift; __aligned(IIO_DMA_MINALIGN); > u8 buf[3] __aligned(IIO_DMA_MINALIGN); > > It will be: u8 shift; __u8 __cacheline_group_begin__[0] __aligned(ARCH_DMA_MINALIGN); u8 buf[3]; __u8 __cacheline_group_end__[0] __aligned(ARCH_DMA_MINALIGN); Note that ARCH_DMA_MINALIGN is not always the same as IIO_DMA_MINALIGN. IIO_DMA_MINALIGN has a minimum of 8 bytes to account for timestamp alignment. I wonder if this would add an extra ARCH_DMA_MINALIGN bytes to the struct. Or if the compiler is smart enough to see that it has 0 size on the last array and have a special case for that. And even if the 0 is handled, if someone added a new field after this, I expect the struct would grow by ARCH_DMA_MINALIGN rather than sizeof(field) bytes.
On Sat, 21 Mar 2026 14:40:35 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 3/16/26 2:53 PM, Andy Shevchenko wrote: > > On Mon, Mar 16, 2026 at 06:31:17PM +0000, Jonathan Cameron wrote: > >> On Sat, 14 Mar 2026 18:13:32 -0500 > >> David Lechner <dlechner@baylibre.com> wrote: > > > > ... > > > >>> u8 shift; > >>> + u8 buf[3] __aligned(IIO_DMA_MINALIGN); > >> On this. There is new generic infrastructure for marking these. > >> https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/dma-mapping.h#L720 > >> https://lore.kernel.org/all/01ea88055ded4d70cac70ba557680fd5fa7d9ff5.1767601130.git.mst@redhat.com/ > >> > >> Would look like > >> __dma_from_device_group_begin(); > >> u8 buf[3]; > >> __dma_from_device_group_end(); > >> > >> Do you think we should adopt them rather than doing our own thing? > >> Slowly though I don't want the noise of a mass conversion. > >> > >> As normal, advantage of standard infrastructure is cutting down > >> in subsystem specific magic. > >> > >> I 'think' result is the same (though it also forces the trailing padding if anything > >> comes after this and needs it). > > > > As I read it it will be an equivalent to > > > > u8 shift; __aligned(IIO_DMA_MINALIGN); > > u8 buf[3] __aligned(IIO_DMA_MINALIGN); > > > > > > It will be: > > u8 shift; > __u8 __cacheline_group_begin__[0] __aligned(ARCH_DMA_MINALIGN); > u8 buf[3]; > __u8 __cacheline_group_end__[0] __aligned(ARCH_DMA_MINALIGN); > > Note that ARCH_DMA_MINALIGN is not always the same as IIO_DMA_MINALIGN. > IIO_DMA_MINALIGN has a minimum of 8 bytes to account for timestamp > alignment. Good point. All the sensible arches have min 8 anyway but who knows.. > > I wonder if this would add an extra ARCH_DMA_MINALIGN bytes to > the struct. Or if the compiler is smart enough to see that it has > 0 size on the last array and have a special case for that. > My gut feeling is that it will be fine. If you have a [0] element in a flex array (the old way of doing it that recently got ripped out of the kernel) then the sizeof() the structure never included anything for that. I don't see why aligning it should matter. > And even if the 0 is handled, if someone added a new field after this, > I expect the struct would grow by ARCH_DMA_MINALIGN rather than sizeof(field) > bytes. Yes, the structure always has to be a multiple of the item with the largest alignment so anything after that forcing align will bloat by whole ARCH_DMA_MINALIGN. >
On Sat, Mar 14, 2026 at 06:13:32PM -0500, David Lechner wrote: > Add a DMA-safe buffer and use it for spi_read() instead of a stack > memory. All SPI buffers must be DMA-safe. > > Since we only need up to 3 bytes, we just use a u8[] instead of __be16 > and __be32 and change the conversion functions appropriately. Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> -- With Best Regards, Andy Shevchenko
On Mon, 16 Mar 2026 16:05:19 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Sat, Mar 14, 2026 at 06:13:32PM -0500, David Lechner wrote: > > Add a DMA-safe buffer and use it for spi_read() instead of a stack > > memory. All SPI buffers must be DMA-safe. > > > > Since we only need up to 3 bytes, we just use a u8[] instead of __be16 > > and __be32 and change the conversion functions appropriately. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > Applied and marked for stable with the following as amazingly enough I was running a config where it didn't have this via another path so the build failed. Thanks, J diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c index 833a636a29bb..be1cc2e77862 100644 --- a/drivers/iio/adc/ti-adc161s626.c +++ b/drivers/iio/adc/ti-adc161s626.c @@ -15,6 +15,7 @@ #include <linux/init.h> #include <linux/err.h> #include <linux/spi/spi.h> +#include <linux/unaligned.h> #include <linux/iio/iio.h> #include <linux/iio/trigger.h> #include <linux/iio/buffer.h>
© 2016 - 2026 Red Hat, Inc.