[PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[David Lechner]

Use DMA-safe memory instead of stack-allocated memory for the call to
regmap_bulk_read() in the ada4250_init() function as this could be used
directly by a SPI controller.

Also remove unnecessary use of get_unaligned_le16() and explicitly
include linux/types.h e.g. for __le16 while we are fixing this up.

Fixes: 28b4c30bfa5f ("iio: amplifiers: ada4250: add support for ADA4250")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/amplifiers/ada4250.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index 74f8429d652b17b4d1f38366e23ce6a2b3e9b218..f81438460aa51ce30f8f605c60ee5be5c8c251d3 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -13,8 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
-
-#include <linux/unaligned.h>
+#include <linux/types.h>
 
 /* ADA4250 Register Map */
 #define ADA4250_REG_GAIN_MUX        0x00
@@ -63,6 +62,7 @@ struct ada4250_state {
 	u8			gain;
 	int			offset_uv;
 	bool			refbuf_en;
+	__le16			reg_val_16 __aligned(IIO_DMA_MINALIGN);
 };
 
 /* ADA4250 Current Bias Source Settings: Disabled, Bandgap Reference, AVDD */
@@ -301,7 +301,6 @@ static int ada4250_init(struct ada4250_state *st)
 {
 	int ret;
 	u16 chip_id;
-	u8 data[2] __aligned(8) = {};
 	struct spi_device *spi = st->spi;
 
 	st->refbuf_en = device_property_read_bool(&spi->dev, "adi,refbuf-enable");
@@ -326,11 +325,12 @@ static int ada4250_init(struct ada4250_state *st)
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, data, 2);
+	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, &st->reg_val_16,
+			       sizeof(st->reg_val_16));
 	if (ret)
 		return ret;
 
-	chip_id = get_unaligned_le16(data);
+	chip_id = le16_to_cpu(st->reg_val_16);
 
 	if (chip_id != ADA4250_CHIP_ID) {
 		dev_err(&spi->dev, "Invalid chip ID.\n");

---
base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
change-id: 20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-93ebb1344295

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[David Lechner]

On 4/18/25 1:58 PM, Andy Shevchenko wrote:
> 
> 
> пʼятниця, 18 квітня 2025 р. David Lechner <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
> 
>     Use DMA-safe memory instead of stack-allocated memory for the call to
>     regmap_bulk_read() in the ada4250_init() function as this could be used
>     directly by a SPI controller.
> 
> 
> Sorry, but can you elaborate more on this? If driver doesn’t override the callbacks the regmap SPI uses spi_write_then_read() which is supposed to be dma safe. 
>  
> 

Ah, I didn't dig that far down. Will send a new patch that just cleans up the
unnecessary alignment and unaligned call.

(Also can't believe you sent HTML mail!)

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 02:09:04PM -0500, David Lechner wrote:
> On 4/18/25 1:58 PM, Andy Shevchenko wrote:
> > пʼятниця, 18 квітня 2025 р. David Lechner <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
> > 
> >     Use DMA-safe memory instead of stack-allocated memory for the call to
> >     regmap_bulk_read() in the ada4250_init() function as this could be used
> >     directly by a SPI controller.
> > 
> > Sorry, but can you elaborate more on this? If driver doesn’t override the
> > callbacks the regmap SPI uses spi_write_then_read() which is supposed to be
> > dma safe. 
> 
> Ah, I didn't dig that far down. Will send a new patch that just cleans up the
> unnecessary alignment and unaligned call.

But do you have a real life issue with that? Coincidentally we discussed
similar case in another driver with colleague of mine, and he insists that it
might be still broken (somewhere).

> (Also can't believe you sent HTML mail!)

Yeah, Gmail from the phone can't actually send a plain text :-(
Very strange from Google, while I understand that the case most
likely is for business, where HTML is a standard (sic!) for emails.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[David Lechner]

On 4/19/25 10:03 AM, Andy Shevchenko wrote:
> On Fri, Apr 18, 2025 at 02:09:04PM -0500, David Lechner wrote:
>> On 4/18/25 1:58 PM, Andy Shevchenko wrote:
>>> пʼятниця, 18 квітня 2025 р. David Lechner <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
>>>
>>>     Use DMA-safe memory instead of stack-allocated memory for the call to
>>>     regmap_bulk_read() in the ada4250_init() function as this could be used
>>>     directly by a SPI controller.
>>>
>>> Sorry, but can you elaborate more on this? If driver doesn’t override the
>>> callbacks the regmap SPI uses spi_write_then_read() which is supposed to be
>>> dma safe. 
>>
>> Ah, I didn't dig that far down. Will send a new patch that just cleans up the
>> unnecessary alignment and unaligned call.
> 
> But do you have a real life issue with that? Coincidentally we discussed
> similar case in another driver with colleague of mine, and he insists that it
> might be still broken (somewhere).
> 

Some drivers that use regmap with SPI implement their own read and write
callbacks if the default one doesn't work, so maybe something like that?

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[Jonathan Cameron]

On Sat, 19 Apr 2025 13:07:25 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/19/25 10:03 AM, Andy Shevchenko wrote:
> > On Fri, Apr 18, 2025 at 02:09:04PM -0500, David Lechner wrote:  
> >> On 4/18/25 1:58 PM, Andy Shevchenko wrote:  
> >>> пʼятниця, 18 квітня 2025 р. David Lechner <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
> >>>
> >>>     Use DMA-safe memory instead of stack-allocated memory for the call to
> >>>     regmap_bulk_read() in the ada4250_init() function as this could be used
> >>>     directly by a SPI controller.
> >>>
> >>> Sorry, but can you elaborate more on this? If driver doesn’t override the
> >>> callbacks the regmap SPI uses spi_write_then_read() which is supposed to be
> >>> dma safe.   
> >>
> >> Ah, I didn't dig that far down. Will send a new patch that just cleans up the
> >> unnecessary alignment and unaligned call.  
> > 
> > But do you have a real life issue with that? Coincidentally we discussed
> > similar case in another driver with colleague of mine, and he insists that it
> > might be still broken (somewhere).
> >   
> 
> Some drivers that use regmap with SPI implement their own read and write
> callbacks if the default one doesn't work, so maybe something like that?
> 

I asked Mark about this long ago. Maybe the guidance has changed, but the request
if my memory is correct was to treat bulk regmap accesses with the same care wrt to
DMA safety that we treat direct use of the underlying bus accessors.
(unfortunately I have no idea what thread this was in!)

The reasoning I think was that it was an implementation detail whether
regmap copied the data or not for bulk accesses.  Back then I chased through
the SPI case and couldn't find a path that didn't bounce the data (though some
paths could have been made zero copy).

Anyhow, +CC Mark as maybe I remembered that wrong or his view has
changed!

Jonathan

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[Nuno Sá]

On Mon, 2025-04-21 at 11:57 +0100, Jonathan Cameron wrote:
> On Sat, 19 Apr 2025 13:07:25 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 4/19/25 10:03 AM, Andy Shevchenko wrote:
> > > On Fri, Apr 18, 2025 at 02:09:04PM -0500, David Lechner wrote:  
> > > > On 4/18/25 1:58 PM, Andy Shevchenko wrote:  
> > > > > пʼятниця, 18 квітня 2025 р. David Lechner
> > > > > <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
> > > > > 
> > > > >     Use DMA-safe memory instead of stack-allocated memory for the call
> > > > > to
> > > > >     regmap_bulk_read() in the ada4250_init() function as this could be
> > > > > used
> > > > >     directly by a SPI controller.
> > > > > 
> > > > > Sorry, but can you elaborate more on this? If driver doesn’t override
> > > > > the
> > > > > callbacks the regmap SPI uses spi_write_then_read() which is supposed
> > > > > to be
> > > > > dma safe.   
> > > > 
> > > > Ah, I didn't dig that far down. Will send a new patch that just cleans
> > > > up the
> > > > unnecessary alignment and unaligned call.  
> > > 
> > > But do you have a real life issue with that? Coincidentally we discussed
> > > similar case in another driver with colleague of mine, and he insists that
> > > it
> > > might be still broken (somewhere).
> > >   
> > 
> > Some drivers that use regmap with SPI implement their own read and write
> > callbacks if the default one doesn't work, so maybe something like that?
> > 
> 
> I asked Mark about this long ago. Maybe the guidance has changed, but the
> request
> if my memory is correct was to treat bulk regmap accesses with the same care
> wrt to
> DMA safety that we treat direct use of the underlying bus accessors.
> (unfortunately I have no idea what thread this was in!)
> 
> The reasoning I think was that it was an implementation detail whether
> regmap copied the data or not for bulk accesses.  Back then I chased through
> the SPI case and couldn't find a path that didn't bounce the data (though some
> paths could have been made zero copy).
> 

Looking on the implementation, indeed it looks like that everything should be
fine... However, IMO, David's reasoning about potential drivers using their own
regmap_bus (or just overriding read and write) is perhaps justification enough
for doing it.

- Nuno Sá 

> Anyhow, +CC Mark as maybe I remembered that wrong or his view has
> changed!
> 
> Jonathan

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[Jonathan Cameron]

On Mon, 21 Apr 2025 14:00:30 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2025-04-21 at 11:57 +0100, Jonathan Cameron wrote:
> > On Sat, 19 Apr 2025 13:07:25 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> > > On 4/19/25 10:03 AM, Andy Shevchenko wrote:  
> > > > On Fri, Apr 18, 2025 at 02:09:04PM -0500, David Lechner wrote:    
> > > > > On 4/18/25 1:58 PM, Andy Shevchenko wrote:    
> > > > > > пʼятниця, 18 квітня 2025 р. David Lechner
> > > > > > <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
> > > > > > 
> > > > > >     Use DMA-safe memory instead of stack-allocated memory for the call
> > > > > > to
> > > > > >     regmap_bulk_read() in the ada4250_init() function as this could be
> > > > > > used
> > > > > >     directly by a SPI controller.
> > > > > > 
> > > > > > Sorry, but can you elaborate more on this? If driver doesn’t override
> > > > > > the
> > > > > > callbacks the regmap SPI uses spi_write_then_read() which is supposed
> > > > > > to be
> > > > > > dma safe.     
> > > > > 
> > > > > Ah, I didn't dig that far down. Will send a new patch that just cleans
> > > > > up the
> > > > > unnecessary alignment and unaligned call.    
> > > > 
> > > > But do you have a real life issue with that? Coincidentally we discussed
> > > > similar case in another driver with colleague of mine, and he insists that
> > > > it
> > > > might be still broken (somewhere).
> > > >     
> > > 
> > > Some drivers that use regmap with SPI implement their own read and write
> > > callbacks if the default one doesn't work, so maybe something like that?
> > >   
> > 
> > I asked Mark about this long ago. Maybe the guidance has changed, but the
> > request
> > if my memory is correct was to treat bulk regmap accesses with the same care
> > wrt to
> > DMA safety that we treat direct use of the underlying bus accessors.
> > (unfortunately I have no idea what thread this was in!)
> > 
> > The reasoning I think was that it was an implementation detail whether
> > regmap copied the data or not for bulk accesses.  Back then I chased through
> > the SPI case and couldn't find a path that didn't bounce the data (though some
> > paths could have been made zero copy).
> >   
> 
> Looking on the implementation, indeed it looks like that everything should be
> fine... However, IMO, David's reasoning about potential drivers using their own
> regmap_bus (or just overriding read and write) is perhaps justification enough
> for doing it.

That's in a specific driver which is the same one allocating the buffer.
Doing it defensively in other drivers is probably a step too far if we don't
still want to do it everwhere regmap and SPI are combined.

Jonathan


> 
> - Nuno Sá 
> 
> > Anyhow, +CC Mark as maybe I remembered that wrong or his view has
> > changed!
> > 
> > Jonathan  

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[Jonathan Cameron]

On Mon, 21 Apr 2025 14:56:06 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 21 Apr 2025 14:00:30 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Mon, 2025-04-21 at 11:57 +0100, Jonathan Cameron wrote:  
> > > On Sat, 19 Apr 2025 13:07:25 -0500
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >     
> > > > On 4/19/25 10:03 AM, Andy Shevchenko wrote:    
> > > > > On Fri, Apr 18, 2025 at 02:09:04PM -0500, David Lechner wrote:      
> > > > > > On 4/18/25 1:58 PM, Andy Shevchenko wrote:      
> > > > > > > пʼятниця, 18 квітня 2025 р. David Lechner
> > > > > > > <dlechner@baylibre.com <mailto:dlechner@baylibre.com>> пише:
> > > > > > > 
> > > > > > >     Use DMA-safe memory instead of stack-allocated memory for the call
> > > > > > > to
> > > > > > >     regmap_bulk_read() in the ada4250_init() function as this could be
> > > > > > > used
> > > > > > >     directly by a SPI controller.
> > > > > > > 
> > > > > > > Sorry, but can you elaborate more on this? If driver doesn’t override
> > > > > > > the
> > > > > > > callbacks the regmap SPI uses spi_write_then_read() which is supposed
> > > > > > > to be
> > > > > > > dma safe.       
> > > > > > 
> > > > > > Ah, I didn't dig that far down. Will send a new patch that just cleans
> > > > > > up the
> > > > > > unnecessary alignment and unaligned call.      
> > > > > 
> > > > > But do you have a real life issue with that? Coincidentally we discussed
> > > > > similar case in another driver with colleague of mine, and he insists that
> > > > > it
> > > > > might be still broken (somewhere).
> > > > >       
> > > > 
> > > > Some drivers that use regmap with SPI implement their own read and write
> > > > callbacks if the default one doesn't work, so maybe something like that?
> > > >     
> > > 
> > > I asked Mark about this long ago. Maybe the guidance has changed, but the
> > > request
> > > if my memory is correct was to treat bulk regmap accesses with the same care
> > > wrt to
> > > DMA safety that we treat direct use of the underlying bus accessors.
> > > (unfortunately I have no idea what thread this was in!)
> > > 
> > > The reasoning I think was that it was an implementation detail whether
> > > regmap copied the data or not for bulk accesses.  Back then I chased through
> > > the SPI case and couldn't find a path that didn't bounce the data (though some
> > > paths could have been made zero copy).
> > >     
> > 
> > Looking on the implementation, indeed it looks like that everything should be
> > fine... However, IMO, David's reasoning about potential drivers using their own
> > regmap_bus (or just overriding read and write) is perhaps justification enough
> > for doing it.  
> 
> That's in a specific driver which is the same one allocating the buffer.
> Doing it defensively in other drivers is probably a step too far if we don't
> still want to do it everwhere regmap and SPI are combined.

Given we don't believe this to be an active bug and no reply from Mark
on any change on regmap, I'm going to queue this up the slow way and
see if anyone objects.  Queued up for 6.17.

I dropped the fixes tag and added a bit of commentary on why.

Jonathan

> 
> Jonathan
> 
> 
> > 
> > - Nuno Sá 
> >   
> > > Anyhow, +CC Mark as maybe I remembered that wrong or his view has
> > > changed!
> > > 
> > > Jonathan    
> 
> 

Re: [PATCH] iio: amplifiers: ada4250: use DMA-safe memory for regmap_bulk_read()

[Jonathan Cameron]

On Fri, 18 Apr 2025 13:16:13 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Use DMA-safe memory instead of stack-allocated memory for the call to
> regmap_bulk_read() in the ada4250_init() function as this could be used
> directly by a SPI controller.
> 
> Also remove unnecessary use of get_unaligned_le16() and explicitly
> include linux/types.h e.g. for __le16 while we are fixing this up.
> 
> Fixes: 28b4c30bfa5f ("iio: amplifiers: ada4250: add support for ADA4250")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Let's wait and see if Mark has a view on this.  The switch to an __le16 is
good either way.

Jonathan

> ---
>  drivers/iio/amplifiers/ada4250.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
> index 74f8429d652b17b4d1f38366e23ce6a2b3e9b218..f81438460aa51ce30f8f605c60ee5be5c8c251d3 100644
> --- a/drivers/iio/amplifiers/ada4250.c
> +++ b/drivers/iio/amplifiers/ada4250.c
> @@ -13,8 +13,7 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> -
> -#include <linux/unaligned.h>
> +#include <linux/types.h>
>  
>  /* ADA4250 Register Map */
>  #define ADA4250_REG_GAIN_MUX        0x00
> @@ -63,6 +62,7 @@ struct ada4250_state {
>  	u8			gain;
>  	int			offset_uv;
>  	bool			refbuf_en;
> +	__le16			reg_val_16 __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  /* ADA4250 Current Bias Source Settings: Disabled, Bandgap Reference, AVDD */
> @@ -301,7 +301,6 @@ static int ada4250_init(struct ada4250_state *st)
>  {
>  	int ret;
>  	u16 chip_id;
> -	u8 data[2] __aligned(8) = {};
>  	struct spi_device *spi = st->spi;
>  
>  	st->refbuf_en = device_property_read_bool(&spi->dev, "adi,refbuf-enable");
> @@ -326,11 +325,12 @@ static int ada4250_init(struct ada4250_state *st)
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, data, 2);
> +	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, &st->reg_val_16,
> +			       sizeof(st->reg_val_16));
>  	if (ret)
>  		return ret;
>  
> -	chip_id = get_unaligned_le16(data);
> +	chip_id = le16_to_cpu(st->reg_val_16);
>  
>  	if (chip_id != ADA4250_CHIP_ID) {
>  		dev_err(&spi->dev, "Invalid chip ID.\n");
> 
> ---
> base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
> change-id: 20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-93ebb1344295
> 
> Best regards,