| Message ID | 20200503172235.207632-1-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: accel: kxsd9: Fix alignment of local buffer. | expand |
On Sun, 2020-05-03 at 18:22 +0100, 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 8 __be16. > > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/accel/kxsd9.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c > index 0b876b2dc5bd..4c42d1200914 100644 > --- a/drivers/iio/accel/kxsd9.c > +++ b/drivers/iio/accel/kxsd9.c > @@ -209,14 +209,16 @@ 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); > + struct { > + __be16 chan[4]; > + u64 ts; > + } hw_values; A nitpick from my side. Maybe a comment woudn't be that noisy. It might be not that obvious for some people why this construct is needed and it might prevent new driver's from doing the same mistake. - Nuno Sá
On Sun, 3 May 2020 20:01:56 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sun, 2020-05-03 at 18:22 +0100, 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 8 __be16. > > > > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > drivers/iio/accel/kxsd9.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c > > index 0b876b2dc5bd..4c42d1200914 100644 > > --- a/drivers/iio/accel/kxsd9.c > > +++ b/drivers/iio/accel/kxsd9.c > > @@ -209,14 +209,16 @@ 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); > > + struct { > > + __be16 chan[4]; > > + u64 ts; > > + } hw_values; > > A nitpick from my side. Maybe a comment woudn't be that noisy. It might > be not that obvious for some people why this construct is needed and it > might prevent new driver's from doing the same mistake. Good point. I'll add something like /* Ensure correct positioning and alignment of timestamp */ above the struct definition. > > - Nuno Sá > >
diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c index 0b876b2dc5bd..4c42d1200914 100644 --- a/drivers/iio/accel/kxsd9.c +++ b/drivers/iio/accel/kxsd9.c @@ -209,14 +209,16 @@ 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); + struct { + __be16 chan[4]; + u64 ts; + } 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 +226,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);