Re: [PATCH V2] iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Tue, 16 Apr 2019 11:19:16 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Use a heap allocated memory for the SPI transfer buffer. Using stack memory
> will not work on some architectures when using DMA.
> 
> This also avoids potential stack corruption, as it makes sure that the
> buffer is in it's own cacheline and (obviously) not on the stack.
> 
> 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>
Hi Alex,

Thanks for the update.

I'm unclear why we can't just use one large buffer for both cases?
i.e put the reg_data usecases in the buf_data buffer (renamed).

Jonathan
> ---
> Changelog v1 -> v2:
> * added ____cacheline_aligned to `buf_data`
> * extended comment about cacheline
> * added fixes tag
> 
>  drivers/iio/adc/ad_sigma_delta.c       | 22 +++++++++++-----------
>  include/linux/iio/adc/ad_sigma_delta.h |  3 ++-
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index af6cbc683214..02d9049b9218 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -58,7 +58,7 @@ EXPORT_SYMBOL_GPL(ad_sd_set_comm);
>  int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
>  	unsigned int size, unsigned int val)
>  {
> -	uint8_t *data = sigma_delta->data;
> +	uint8_t *data = sigma_delta->reg_data;
>  	struct spi_transfer t = {
>  		.tx_buf		= data,
>  		.len		= size + 1,
> @@ -102,7 +102,7 @@ EXPORT_SYMBOL_GPL(ad_sd_write_reg);
>  static int ad_sd_read_reg_raw(struct ad_sigma_delta *sigma_delta,
>  	unsigned int reg, unsigned int size, uint8_t *val)
>  {
> -	uint8_t *data = sigma_delta->data;
> +	uint8_t *data = sigma_delta->reg_data;
>  	int ret;
>  	struct spi_transfer t[] = {
>  		{
> @@ -148,24 +148,24 @@ int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta,
>  {
>  	int ret;
>  
> -	ret = ad_sd_read_reg_raw(sigma_delta, reg, size, sigma_delta->data);
> +	ret = ad_sd_read_reg_raw(sigma_delta, reg, size, sigma_delta->reg_data);
>  	if (ret < 0)
>  		goto out;
>  
>  	switch (size) {
>  	case 4:
> -		*val = get_unaligned_be32(sigma_delta->data);
> +		*val = get_unaligned_be32(sigma_delta->reg_data);
>  		break;
>  	case 3:
> -		*val = (sigma_delta->data[0] << 16) |
> -			(sigma_delta->data[1] << 8) |
> -			sigma_delta->data[2];
> +		*val = (sigma_delta->reg_data[0] << 16) |
> +			(sigma_delta->reg_data[1] << 8) |
> +			sigma_delta->reg_data[2];
>  		break;
>  	case 2:
> -		*val = get_unaligned_be16(sigma_delta->data);
> +		*val = get_unaligned_be16(sigma_delta->reg_data);
>  		break;
>  	case 1:
> -		*val = sigma_delta->data[0];
> +		*val = sigma_delta->reg_data[0];
>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -403,12 +403,12 @@ 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->buf_data;
>  	unsigned int reg_size;
>  	unsigned int data_reg;
> -	uint8_t data[16];
>  	int ret;
>  
> -	memset(data, 0x00, 16);
> +	memset(sigma_delta->buf_data, 0x00, sizeof(sigma_delta->buf_data));
>  
>  	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 6e9fb1932dde..d95216ce8a94 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -79,7 +79,8 @@ 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				reg_data[4] ____cacheline_aligned;
> +	uint8_t				buf_data[16] ____cacheline_aligned;
The usual assumption is that a device is safe against itself, so we can't
get cacheline corruption in a single action from the device to itself.
This does make me wonder why we have two buffers like this at all though!

>  };
>  
>  static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,