linux-kernel - Re: [PATCH v5 6/7] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b986f420280ae64b77c9534b856f3537bd5b29ed.camel@gmail.com>
Date: Wed, 07 May 2025 07:37:50 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Jonathan Cameron
 <jic23@...nel.org>,  Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>, Eugen Hristev
 <eugen.hristev@...aro.org>, Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>, Claudiu Beznea
 <claudiu.beznea@...on.dev>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 6/7] iio: accel: sca3300: use
 IIO_DECLARE_BUFFER_WITH_TS

On Mon, 2025-05-05 at 11:31 -0500, David Lechner wrote:
> Use IIO_DECLARE_BUFFER_WITH_TS() to declare the buffer that gets used
> with iio_push_to_buffers_with_ts(). This makes the code a bit easier to
> read and understand.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> This is an alternative to [1]. Also, this serves as a test to see if we
> can get a rule of thumb to decide how much is too much to put on the
> stack vs. needing to put the buffer in a static struct. SCA3300_SCAN_MAX
> is 7, so this add a bit over 64 bytes to the stack, make the stack now
> roughly double what it was before.

IMHO, I think that for this buffer size, having it on the stack is very reasonable.

> 
> [1]:
> https://lore.kernel.org/linux-iio/20250418-iio-prefer-aligned_s64-timestamp-v1-1-4c6080710516@baylibre.com/
> ---
>  drivers/iio/accel/sca3300.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index
> 1132bbaba75bcca525fac2f3e19f63546380fd4f..67416a406e2f43e4e417210410904d44c93111d2
> 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -58,15 +58,6 @@ enum sca3300_scan_indexes {
>  	SCA3300_SCAN_MAX
>  };
>  
> -/*
> - * Buffer size max case:
> - * Three accel channels, two bytes per channel.
> - * Temperature channel, two bytes.
> - * Three incli channels, two bytes per channel.
> - * Timestamp channel, eight bytes.
> - */
> -#define SCA3300_MAX_BUFFER_SIZE (ALIGN(sizeof(s16) * SCA3300_SCAN_MAX,
> sizeof(s64)) + sizeof(s64))
> -
>  #define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
>  	.type = IIO_ACCEL,						\
>  	.address = reg,							\
> @@ -193,9 +184,6 @@ struct sca3300_chip_info {
>   * @spi: SPI device structure
>   * @lock: Data buffer lock
>   * @chip: Sensor chip specific information
> - * @buffer: Triggered buffer:
> - *          -SCA3300: 4 channel 16-bit data + 64-bit timestamp
> - *          -SCL3300: 7 channel 16-bit data + 64-bit timestamp
>   * @txbuf: Transmit buffer
>   * @rxbuf: Receive buffer
>   */
> @@ -203,7 +191,6 @@ struct sca3300_data {
>  	struct spi_device *spi;
>  	struct mutex lock;
>  	const struct sca3300_chip_info *chip;
> -	u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
>  	u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
>  	u8 rxbuf[4];
>  };
> @@ -492,7 +479,7 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct sca3300_data *data = iio_priv(indio_dev);
>  	int bit, ret, val, i = 0;
> -	s16 *channels = (s16 *)data->buffer;
> +	IIO_DECLARE_BUFFER_WITH_TS(s16, channels, SCA3300_SCAN_MAX);

On top of what you mentioned I like this change. The pattern you're removing is the
typical one for getting into unaligned accesses.

>  
>  	iio_for_each_active_channel(indio_dev, bit) {
>  		ret = sca3300_read_reg(data, indio_dev->channels[bit].address,
> &val);
> @@ -505,8 +492,7 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
>  		channels[i++] = val;
>  	}
>  
> -	iio_push_to_buffers_with_ts(indio_dev, data->buffer,
> -				    sizeof(data->buffer),
> +	iio_push_to_buffers_with_ts(indio_dev, channels, sizeof(channels),
>  				    iio_get_time_ns(indio_dev));
>  out:
>  	iio_trigger_notify_done(indio_dev->trig);
>