| Message ID | 20210501170121.512209-17-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: Alignment fixes part 2 - struct used to ensure alignment | expand |
On Sat, May 1, 2021 at 7:03 PM Jonathan Cameron <jic23@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > To make code more readable, use a structure to express the channel > layout and ensure the timestamp is 8 byte aligned. > > Found during an audit of all calls of uses of > iio_push_to_buffers_with_timestamp() > > Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer") > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Stephan Gerhold <stephan@gerhold.net> > Cc: Linus Walleij <linus.walleij@linaro.org> Excellent work Jonathan. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I wonder if there is some way for us to abstract this into the core so we can't get it wrong. Yours, Linus Walleij
On Wed, 5 May 2021 14:57:12 +0200 Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, May 1, 2021 at 7:03 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > To make code more readable, use a structure to express the channel > > layout and ensure the timestamp is 8 byte aligned. > > > > Found during an audit of all calls of uses of > > iio_push_to_buffers_with_timestamp() > > > > Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer") > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Stephan Gerhold <stephan@gerhold.net> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Excellent work Jonathan. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > I wonder if there is some way for us to abstract this into the core so > we can't get it wrong. Abstracting is a bit of a pain, because we end up creating unnecessary limitations on what can be done. Often having buffer a lot larger than it needs to be is sensible for example... However, I'm definitely thinking we should look at what checks we can add once all these cases are fixed and there might be a nice pattern to use for those cases where we currently have a bounce buffer anyway due to hardware restrictions. In most others, moving to the pattern where the timestamp is explicit in the structure makes it obvious (subject to the fun question of x86_32 alignment and whether that matters - we don't know of any bugs as a result but it's possible some buffer consumer will assume 8 byte alignment - hence the hardening in these cases). The size being too small case for example should be easy - we just augment iio_push_to_buffers_with_timestamp() to take the size and check it against scan_bytes. Alignment is trickier... Jonathan > Yours, > Linus Walleij
On Sat, 1 May 2021 18:01:18 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > To make code more readable, use a structure to express the channel > layout and ensure the timestamp is 8 byte aligned. > > Found during an audit of all calls of uses of > iio_push_to_buffers_with_timestamp() > > Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer") > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Stephan Gerhold <stephan@gerhold.net> > Cc: Linus Walleij <linus.walleij@linaro.org> Applied, but with a tiny tweak to make this more obviously correct. > --- > drivers/iio/magnetometer/bmc150_magn.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c > index 00f9766bad5c..dd5f80093a18 100644 > --- a/drivers/iio/magnetometer/bmc150_magn.c > +++ b/drivers/iio/magnetometer/bmc150_magn.c > @@ -138,8 +138,11 @@ struct bmc150_magn_data { > struct regmap *regmap; > struct regulator_bulk_data regulators[2]; > struct iio_mount_matrix orientation; > - /* 4 x 32 bits for x, y z, 4 bytes align, 64 bits timestamp */ > - s32 buffer[6]; > + /* Ensure timestamp is naturally aligned */ > + struct { > + s32 chans[4]; The whole point of the structure is to enforce the alignment without needing to specify padding. There are only 3 actual chans, so I've reduced this to s32 chans[3] Result is exactly the same in practice, but it's more readable. Thanks, Jonathan > + s64 timestamp __aligned(8); > + } scan; > struct iio_trigger *dready_trig; > bool dready_trigger_on; > int max_odr; > @@ -675,11 +678,11 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p) > int ret; > > mutex_lock(&data->mutex); > - ret = bmc150_magn_read_xyz(data, data->buffer); > + ret = bmc150_magn_read_xyz(data, data->scan.chans); > if (ret < 0) > goto err; > > - iio_push_to_buffers_with_timestamp(indio_dev, data->buffer, > + iio_push_to_buffers_with_timestamp(indio_dev, &data->scan, > pf->timestamp); > > err:
diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c index 00f9766bad5c..dd5f80093a18 100644 --- a/drivers/iio/magnetometer/bmc150_magn.c +++ b/drivers/iio/magnetometer/bmc150_magn.c @@ -138,8 +138,11 @@ struct bmc150_magn_data { struct regmap *regmap; struct regulator_bulk_data regulators[2]; struct iio_mount_matrix orientation; - /* 4 x 32 bits for x, y z, 4 bytes align, 64 bits timestamp */ - s32 buffer[6]; + /* Ensure timestamp is naturally aligned */ + struct { + s32 chans[4]; + s64 timestamp __aligned(8); + } scan; struct iio_trigger *dready_trig; bool dready_trigger_on; int max_odr; @@ -675,11 +678,11 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p) int ret; mutex_lock(&data->mutex); - ret = bmc150_magn_read_xyz(data, data->buffer); + ret = bmc150_magn_read_xyz(data, data->scan.chans); if (ret < 0) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, data->buffer, + iio_push_to_buffers_with_timestamp(indio_dev, &data->scan, pf->timestamp); err: