[PATCH] iio: adc: max1027: allocate DMA-safe buffer

[PATCH] iio: adc: max1027: allocate DMA-safe buffer

[Marcus Folkesson]

The buffer needs to be DMA-safe when used with spi_read()

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/iio/adc/max1027.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 712fbd2b1f16..ff1f1f15a873 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -435,7 +435,7 @@ static int max1027_probe(struct spi_device *spi)
 
 	st->buffer = devm_kmalloc(&indio_dev->dev,
 				  indio_dev->num_channels * 2,
-				  GFP_KERNEL);
+				  GFP_KERNEL | GFP_DMA);
 	if (st->buffer == NULL) {
 		dev_err(&indio_dev->dev, "Can't allocate buffer\n");
 		return -ENOMEM;
-- 
2.11.0.rc2

Re: [PATCH] iio: adc: max1027: allocate DMA-safe buffer

[Jonathan Cameron]

On 09/12/16 10:24, Marcus Folkesson wrote:
> The buffer needs to be DMA-safe when used with spi_read()
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Please read the documentation in include/linux/gfp.h about GFP_DMA.

Specifically:
220  * GFP_DMA exists for historical reasons and should be avoided where possible.
221  *   The flags indicates that the caller requires that the lowest zone be
222  *   used (ZONE_DMA or 16M on x86-64). Ideally, this would be removed but
223  *   it would require careful auditing as some users really require it and
224  *   others use the flag to avoid lowmem reserves in ZONE_DMA and treat the
225  *   lowest zone as a type of emergency reserve.

Seems unlikely this applies!  This caught me by surprise as I didn't even know
that flag existed - hence I went digging.

Jonathan
> ---
>  drivers/iio/adc/max1027.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 712fbd2b1f16..ff1f1f15a873 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -435,7 +435,7 @@ static int max1027_probe(struct spi_device *spi)
>  
>  	st->buffer = devm_kmalloc(&indio_dev->dev,
>  				  indio_dev->num_channels * 2,
> -				  GFP_KERNEL);
> +				  GFP_KERNEL | GFP_DMA);
>  	if (st->buffer == NULL) {
>  		dev_err(&indio_dev->dev, "Can't allocate buffer\n");
>  		return -ENOMEM;
> 

Re: [PATCH] iio: adc: max1027: allocate DMA-safe buffer

[Marcus Folkesson]

On Sat, Dec 10, 2016 at 05:36:34PM +0000, Jonathan Cameron wrote:
> On 09/12/16 10:24, Marcus Folkesson wrote:
> > The buffer needs to be DMA-safe when used with spi_read()
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Please read the documentation in include/linux/gfp.h about GFP_DMA.
> 
> Specifically:
> 220  * GFP_DMA exists for historical reasons and should be avoided where possible.
> 221  *   The flags indicates that the caller requires that the lowest zone be
> 222  *   used (ZONE_DMA or 16M on x86-64). Ideally, this would be removed but
> 223  *   it would require careful auditing as some users really require it and
> 224  *   others use the flag to avoid lowmem reserves in ZONE_DMA and treat the
> 225  *   lowest zone as a type of emergency reserve.
> 
> Seems unlikely this applies!  This caught me by surprise as I didn't even know
> that flag existed - hence I went digging.
> 
> Jonathan

Always learn something!
I did not know it was depricated, seems like the comment about GFP_DMA was
commited just a year ago.

I had a problem with a driver on my own that by using a non
DMA-safe buffer, so I was digging around looking for similiar cases in
the kernel.

I thought other drivers (iio/common/ssp_sensors/sspi_spi.c,
input/rmi4/rmi_spi.c) was using GFP_DMA for this purpose.

Anyway, thanks.

Cheers,
Marcus Folkesson 

Re: [PATCH] iio: adc: max1027: allocate DMA-safe buffer

[Jonathan Cameron]

On 10/12/16 20:53, Marcus Folkesson wrote:
> On Sat, Dec 10, 2016 at 05:36:34PM +0000, Jonathan Cameron wrote:
>> On 09/12/16 10:24, Marcus Folkesson wrote:
>>> The buffer needs to be DMA-safe when used with spi_read()
>>>
>>> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>> Please read the documentation in include/linux/gfp.h about GFP_DMA.
>>
>> Specifically:
>> 220  * GFP_DMA exists for historical reasons and should be avoided where possible.
>> 221  *   The flags indicates that the caller requires that the lowest zone be
>> 222  *   used (ZONE_DMA or 16M on x86-64). Ideally, this would be removed but
>> 223  *   it would require careful auditing as some users really require it and
>> 224  *   others use the flag to avoid lowmem reserves in ZONE_DMA and treat the
>> 225  *   lowest zone as a type of emergency reserve.
>>
>> Seems unlikely this applies!  This caught me by surprise as I didn't even know
>> that flag existed - hence I went digging.
>>
>> Jonathan
> 
> Always learn something!
> I did not know it was depricated, seems like the comment about GFP_DMA was
> commited just a year ago.
> 
> I had a problem with a driver on my own that by using a non
> DMA-safe buffer, so I was digging around looking for similiar cases in
> the kernel.
> 
> I thought other drivers (iio/common/ssp_sensors/sspi_spi.c,
> input/rmi4/rmi_spi.c) was using GFP_DMA for this purpose.
oops. I guess that iio one snuck past us.  Karol, I doubt having this flag there
is having any effect at all on your platform.  Shall we drop it?

Marcus, it might be worth following up on the input driver as well if you would like to
do so.  I'm not so certain about what is going on in that driver as I don't know what
rmi is!

Thanks,

Jonathan
> 
> Anyway, thanks.
> 
> Cheers,
> Marcus Folkesson 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: adc: max1027: allocate DMA-safe buffer

[Lars-Peter Clausen]

On 12/10/2016 09:53 PM, Marcus Folkesson wrote:
> On Sat, Dec 10, 2016 at 05:36:34PM +0000, Jonathan Cameron wrote:
>> On 09/12/16 10:24, Marcus Folkesson wrote:
>>> The buffer needs to be DMA-safe when used with spi_read()
>>>
>>> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>> Please read the documentation in include/linux/gfp.h about GFP_DMA.
>>
>> Specifically:
>> 220  * GFP_DMA exists for historical reasons and should be avoided where possible.
>> 221  *   The flags indicates that the caller requires that the lowest zone be
>> 222  *   used (ZONE_DMA or 16M on x86-64). Ideally, this would be removed but
>> 223  *   it would require careful auditing as some users really require it and
>> 224  *   others use the flag to avoid lowmem reserves in ZONE_DMA and treat the
>> 225  *   lowest zone as a type of emergency reserve.
>>
>> Seems unlikely this applies!  This caught me by surprise as I didn't even know
>> that flag existed - hence I went digging.
>>
>> Jonathan
> 
> Always learn something!
> I did not know it was depricated, seems like the comment about GFP_DMA was
> commited just a year ago.

GFP_DMA is the lowest common denominator when it comes to allocating
buffers. Using GFP_DMA will allocate the buffer at an address that is
accessible by all DMA hardware. But for most (and pretty much all modern)
hardware this is simply not necessary since the addressable range is much
larger. On the other hand the downside of using GFP_DMA is that there is
only a very limited amount (16M at most) of memory available for GFP_DMA
allocations. This is why it is not recommend to be used for new drivers.

The problem is that when the buffer is allocated in the IIO driver we do not
know the buffer address restrictions of the SPI device. And often the DMA is
not even done by the SPI controller but a dedicated separate DMA controller.

Ideally there would be some kind of helper function that allows to allocate
a DMA-able transfer buffer for a specific SPI device.

> 
> I had a problem with a driver on my own that by using a non
> DMA-safe buffer, so I was digging around looking for similiar cases in
> the kernel.

What kind of issue was this?

> 
> I thought other drivers (iio/common/ssp_sensors/sspi_spi.c,
> input/rmi4/rmi_spi.c) was using GFP_DMA for this purpose.
> 
> Anyway, thanks.
> 
> Cheers,
> Marcus Folkesson 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>