diff mbox series

iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack

Message ID 20201112091050.84991-1-alexandru.ardelean@analog.com (mailing list archive)
State New
Headers show
Series iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack | expand

Commit Message

Alexandru Ardelean Nov. 12, 2020, 9:10 a.m. UTC
From: Lars-Peter Clausen <lars@metafoo.de>

Use a heap allocated memory for the SPI transfer buffer. Using stack memory
can corrupt stack memory when using DMA on some systems.

This change adds 4 bytes at the end of the current DMA buffer, which will
be used by the trigger handler.
This is required because the first 4 bytes are reserved for register data.

Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 4 ++--
 include/linux/iio/adc/ad_sigma_delta.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Lars-Peter Clausen Nov. 12, 2020, 9:54 a.m. UTC | #1
On 11/12/20 10:10 AM, Alexandru Ardelean wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
>
> Use a heap allocated memory for the SPI transfer buffer. Using stack memory
> can corrupt stack memory when using DMA on some systems.
>
> This change adds 4 bytes at the end of the current DMA buffer, which will
> be used by the trigger handler.
> This is required because the first 4 bytes are reserved for register data.
>
> Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>   drivers/iio/adc/ad_sigma_delta.c       | 4 ++--
>   include/linux/iio/adc/ad_sigma_delta.h | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 86039e9ecaca..33297f26508a 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -395,11 +395,11 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>   	struct iio_poll_func *pf = p;
>   	struct iio_dev *indio_dev = pf->indio_dev;
>   	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +	uint8_t *data = &sigma_delta->data[4];
>   	unsigned int reg_size;
>   	unsigned int data_reg;
> -	uint8_t data[16];
>   
> -	memset(data, 0x00, 16);
> +	memset(data, 0x00, 4);

Younger me didn't know what he was doing, this is wrong. We need the 
extra space for the padding and timestamp.

We also can't put the beginning of the buffer at an 4 byte offset since 
it needs to be 8 byte aligned for the timestamp.

>   
>   	reg_size = indio_dev->channels[0].scan_type.realbits +
>   			indio_dev->channels[0].scan_type.shift;
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index a3a838dcf8e4..ac4ac4752c62 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -80,7 +80,7 @@ struct ad_sigma_delta {
>   	 * DMA (thus cache coherency maintenance) requires the
>   	 * transfer buffers to live in their own cache lines.
>   	 */
> -	uint8_t				data[4] ____cacheline_aligned;
> +	uint8_t				data[8] ____cacheline_aligned;
>   };
>   
>   static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,
Alexandru Ardelean Nov. 12, 2020, 10:14 a.m. UTC | #2
On Thu, Nov 12, 2020 at 11:55 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 11/12/20 10:10 AM, Alexandru Ardelean wrote:
> > From: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Use a heap allocated memory for the SPI transfer buffer. Using stack memory
> > can corrupt stack memory when using DMA on some systems.
> >
> > This change adds 4 bytes at the end of the current DMA buffer, which will
> > be used by the trigger handler.
> > This is required because the first 4 bytes are reserved for register data.
> >
> > Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >   drivers/iio/adc/ad_sigma_delta.c       | 4 ++--
> >   include/linux/iio/adc/ad_sigma_delta.h | 2 +-
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> > index 86039e9ecaca..33297f26508a 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -395,11 +395,11 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
> >       struct iio_poll_func *pf = p;
> >       struct iio_dev *indio_dev = pf->indio_dev;
> >       struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> > +     uint8_t *data = &sigma_delta->data[4];
> >       unsigned int reg_size;
> >       unsigned int data_reg;
> > -     uint8_t data[16];
> >
> > -     memset(data, 0x00, 16);
> > +     memset(data, 0x00, 4);
>
> Younger me didn't know what he was doing, this is wrong. We need the
> extra space for the padding and timestamp.
>
> We also can't put the beginning of the buffer at an 4 byte offset since
> it needs to be 8 byte aligned for the timestamp.

I'll correct this.
I was re-spinning this out of some old patches and discussions on this
that I have.
So, then this becomes 24 bytes? Or 16?

Something like:
uint8_t                         data[24] ____cacheline_aligned;

uint8_t *data = &sigma_delta->data[8];


>
> >
> >       reg_size = indio_dev->channels[0].scan_type.realbits +
> >                       indio_dev->channels[0].scan_type.shift;
> > diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> > index a3a838dcf8e4..ac4ac4752c62 100644
> > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > @@ -80,7 +80,7 @@ struct ad_sigma_delta {
> >        * DMA (thus cache coherency maintenance) requires the
> >        * transfer buffers to live in their own cache lines.
> >        */
> > -     uint8_t                         data[4] ____cacheline_aligned;
> > +     uint8_t                         data[8] ____cacheline_aligned;
> >   };
> >
> >   static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,
>
>
Lars-Peter Clausen Nov. 12, 2020, 10:52 a.m. UTC | #3
On 11/12/20 11:14 AM, Alexandru Ardelean wrote:
> On Thu, Nov 12, 2020 at 11:55 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/12/20 10:10 AM, Alexandru Ardelean wrote:
>>> From: Lars-Peter Clausen <lars@metafoo.de>
>>>
>>> Use a heap allocated memory for the SPI transfer buffer. Using stack memory
>>> can corrupt stack memory when using DMA on some systems.
>>>
>>> This change adds 4 bytes at the end of the current DMA buffer, which will
>>> be used by the trigger handler.
>>> This is required because the first 4 bytes are reserved for register data.
>>>
>>> Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>>> ---
>>>    drivers/iio/adc/ad_sigma_delta.c       | 4 ++--
>>>    include/linux/iio/adc/ad_sigma_delta.h | 2 +-
>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
>>> index 86039e9ecaca..33297f26508a 100644
>>> --- a/drivers/iio/adc/ad_sigma_delta.c
>>> +++ b/drivers/iio/adc/ad_sigma_delta.c
>>> @@ -395,11 +395,11 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>>>        struct iio_poll_func *pf = p;
>>>        struct iio_dev *indio_dev = pf->indio_dev;
>>>        struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
>>> +     uint8_t *data = &sigma_delta->data[4];
>>>        unsigned int reg_size;
>>>        unsigned int data_reg;
>>> -     uint8_t data[16];
>>>
>>> -     memset(data, 0x00, 16);
>>> +     memset(data, 0x00, 4);
>> Younger me didn't know what he was doing, this is wrong. We need the
>> extra space for the padding and timestamp.
>>
>> We also can't put the beginning of the buffer at an 4 byte offset since
>> it needs to be 8 byte aligned for the timestamp.
> I'll correct this.
> I was re-spinning this out of some old patches and discussions on this
> that I have.
> So, then this becomes 24 bytes? Or 16?
>
> Something like:
> uint8_t                         data[24] ____cacheline_aligned;
>
> uint8_t *data = &sigma_delta->data[8];

Yes.
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 86039e9ecaca..33297f26508a 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -395,11 +395,11 @@  static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+	uint8_t *data = &sigma_delta->data[4];
 	unsigned int reg_size;
 	unsigned int data_reg;
-	uint8_t data[16];
 
-	memset(data, 0x00, 16);
+	memset(data, 0x00, 4);
 
 	reg_size = indio_dev->channels[0].scan_type.realbits +
 			indio_dev->channels[0].scan_type.shift;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index a3a838dcf8e4..ac4ac4752c62 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -80,7 +80,7 @@  struct ad_sigma_delta {
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
 	 */
-	uint8_t				data[4] ____cacheline_aligned;
+	uint8_t				data[8] ____cacheline_aligned;
 };
 
 static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,