| Message ID | 20200720152805.365344523@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi! > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > commit 5c49056ad9f3c786f7716da2dd47e4488fc6bd25 upstream. > > One of a class of bugs pointed out by Lars in a recent review. > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned > to the size of the timestamp (8 bytes). This is not guaranteed in > this driver which uses an array of smaller elements on the stack. I don't see documentation explaining alignment issues with iio_push_to_buffers_with_timestamp(). Perhaps comment near that function should explain that? And as it seems to be common problem, perhaps iio_push_to_buffers_with_timestamp should check alignment of its arguments? Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 22 Jul 2020 13:28:35 +0200 Pavel Machek <pavel@denx.de> wrote: > Hi! > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > commit 5c49056ad9f3c786f7716da2dd47e4488fc6bd25 upstream. > > > > One of a class of bugs pointed out by Lars in a recent review. > > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned > > to the size of the timestamp (8 bytes). This is not guaranteed in > > this driver which uses an array of smaller elements on the stack. > > I don't see documentation explaining alignment issues with > iio_push_to_buffers_with_timestamp(). Perhaps comment near that > function should explain that? Hi Pavel, Agreed. It's a subtle corner case (hence we missed it for years) so absolutely needs documenting. The nasty part is that we don't control the expectations of the consumers who get the data from that interface. They may make the reasonable assumption that they aren't getting unaligned data, particularly given the effort we go to in ensuring natural alignment of elements within the buffer. It's a moderately fast path so any tricks with realigning the data aren't sensible either. > > And as it seems to be common problem, perhaps > iio_push_to_buffers_with_timestamp should check alignment of its > arguments? It should indeed check this. But... The reality is that lots of platforms are fine with the alignment not being enforced. So far we have precisely one confirmed report of the issue. Until we have fixed all the users I'm not keen to add a check that will be seen to 'break' existing working systems. It's taking a while to get all these reviewed so I'm picking them up as they get sufficient eyes on them. A few drivers are more fiddly to do so we don't yet have patches on the list. I was thinking to do the documentation update and a check enforcing it in one go, but perhaps given the slow nature of getting all the users fixed we should look to document now and enforce later? Jonathan > > Thanks, > Pavel
--- a/drivers/iio/humidity/hts221.h +++ b/drivers/iio/humidity/hts221.h @@ -15,8 +15,6 @@ #include <linux/iio/iio.h> -#define HTS221_DATA_SIZE 2 - enum hts221_sensor_type { HTS221_SENSOR_H, HTS221_SENSOR_T, @@ -40,6 +38,11 @@ struct hts221_hw { bool enabled; u8 odr; + /* Ensure natural alignment of timestamp */ + struct { + __le16 channels[2]; + s64 ts __aligned(8); + } scan; }; extern const struct dev_pm_ops hts221_pm_ops; --- a/drivers/iio/humidity/hts221_buffer.c +++ b/drivers/iio/humidity/hts221_buffer.c @@ -163,7 +163,6 @@ static const struct iio_buffer_setup_ops static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) { - u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)]; struct iio_poll_func *pf = p; struct iio_dev *iio_dev = pf->indio_dev; struct hts221_hw *hw = iio_priv(iio_dev); @@ -173,18 +172,20 @@ static irqreturn_t hts221_buffer_handler /* humidity data */ ch = &iio_dev->channels[HTS221_SENSOR_H]; err = regmap_bulk_read(hw->regmap, ch->address, - buffer, HTS221_DATA_SIZE); + &hw->scan.channels[0], + sizeof(hw->scan.channels[0])); if (err < 0) goto out; /* temperature data */ ch = &iio_dev->channels[HTS221_SENSOR_T]; err = regmap_bulk_read(hw->regmap, ch->address, - buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE); + &hw->scan.channels[1], + sizeof(hw->scan.channels[1])); if (err < 0) goto out; - iio_push_to_buffers_with_timestamp(iio_dev, buffer, + iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan, iio_get_time_ns(iio_dev)); out: