diff mbox series

[01/32] iio: accel: kxsd9: Fix alignment of local buffer.

Message ID 20200607155408.958437-2-jic23@kernel.org (mailing list archive)
State New, archived
Headers show
Series IIO: Fused set 1 and 2 of timestamp alignment fixes | expand

Commit Message

Jonathan Cameron June 7, 2020, 3:53 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

iio_push_to_buffers_with_timestamp assumes 8 byte alignment which
is not guaranteed by an array of smaller elements.

Note that whilst in this particular case the alignment forcing
of the ts element is not strictly necessary it acts as good
documentation.

Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/kxsd9.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Andy Shevchenko June 7, 2020, 4:05 p.m. UTC | #1 
On Sun, Jun 7, 2020 at 6:57 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> iio_push_to_buffers_with_timestamp assumes 8 byte alignment which
> is not guaranteed by an array of smaller elements.
>
> Note that whilst in this particular case the alignment forcing
> of the ts element is not strictly necessary it acts as good
> documentation.

...

> +       struct {
> +               __be16 chan[4];
> +               s64 ts __aligned(8);
> +       } hw_values;

I'm not sure what __aligned can do better here? It's naturally will be
8 alignment (struct itself due to s64 followed by 4*__be16).
Jonathan Cameron June 7, 2020, 4:34 p.m. UTC | #2 
On Sun, 7 Jun 2020 19:05:15 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 7, 2020 at 6:57 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > iio_push_to_buffers_with_timestamp assumes 8 byte alignment which
> > is not guaranteed by an array of smaller elements.
> >
> > Note that whilst in this particular case the alignment forcing
> > of the ts element is not strictly necessary it acts as good
> > documentation.  
> 
> ...
> 
> > +       struct {
> > +               __be16 chan[4];
> > +               s64 ts __aligned(8);
> > +       } hw_values;  
> 
> I'm not sure what __aligned can do better here? It's naturally will be
> 8 alignment (struct itself due to s64 followed by 4*__be16).
> 
Mainly I put it in all cases even when not needed to ensure that
copy and paste versions keep the alignment statement.  Here I agree
it does nothing but if someone adds another channel to a driver
they might miss that they also need to then add the __aligned(8)
for the timestamp.

Jonathan
diff mbox series

Patch

diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 63b1d8ee6c6f..85e3c46494d3 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -209,14 +209,20 @@  static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
 	const struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct kxsd9_state *st = iio_priv(indio_dev);
+	/*
+	 * Ensure correct positioning and alignment of timestamp.
+	 * No need to zero initialize as all elements written.
+	 */
+	struct {
+		__be16 chan[4];
+		s64 ts __aligned(8);
+	} hw_values;
 	int ret;
-	/* 4 * 16bit values AND timestamp */
-	__be16 hw_values[8];
 
 	ret = regmap_bulk_read(st->map,
 			       KXSD9_REG_X,
-			       &hw_values,
-			       8);
+			       hw_values.chan,
+			       sizeof(hw_values.chan));
 	if (ret) {
 		dev_err(st->dev,
 			"error reading data\n");
@@ -224,7 +230,7 @@  static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
 	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev,
-					   hw_values,
+					   &hw_values,
 					   iio_get_time_ns(indio_dev));
 	iio_trigger_notify_done(indio_dev->trig);