Messages in this thread | | | From | "Vaittinen, Matti" <> | Subject | Re: [PATCH v6 14/24] iio: buffer: wrap all buffer attributes into iio_dev_attr | Date | Fri, 9 Sep 2022 08:12:37 +0000 |
| |
Hi dee Ho peeps!
Disclaimer - I have no HW to test this using real in-tree drivers. If someone has a device with a variant of bmc150 or adxl372 or - it'd be nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min works with the v6.0-rc4. Maybe I am misreading code and have my own issues - in which case I apologize already now and go to the corner while being deeply ashamed :)
On 2/15/21 12:40, Alexandru Ardelean wrote: > This change wraps all buffer attributes into iio_dev_attr objects, and > assigns a reference to the IIO buffer they belong to. > > With the addition of multiple IIO buffers per one IIO device, we need a way > to know which IIO buffer is being enabled/disabled/controlled. > > We know that all buffer attributes are device_attributes.
I think this assumption is slightly unsafe. I see few drivers adding IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 add the hwfifo_watermark_min and hwfifo_watermark_max.
Long story short (or the other way around?)
I was developing a driver for ROHM/Kionix KX022A accelerometer. I did a random pick and chose the bmc150-core as a reference how others have implemented the accel drivers. During the testing I noticed that using IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere it shouldn't... Oops.
Reading the code allows me to assume the problem is wrapping the attributes to IIO_DEV_ATTRs.
static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, + struct attribute *attr) +{ + struct device_attribute *dattr = to_dev_attr(attr); + struct iio_dev_attr *iio_attr; + + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); + if (!iio_attr) + return NULL; + + iio_attr->buffer = buffer; + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));
This copy does assume all attributes are device_attrs, and does not take into account that IIO_CONST_ATTRS have the string stored in a struct iio_const_attr which is containing the dev_attr. Eg, copying in the iio_buffer_wrap_attr() does not copy the string - and later invoking the 'show' callback goes reading something else than the mentioned string because the pointer is not copied.
I believe problem has been there for a while now - introduced by this: 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr") at Feb 2021.
I don't have a fix for you as I am not sure what would be the correct fix. We _could_ go through all the IIO drivers and ensure none used IIO_CONST_ATTRs for triggered buffers. This would IMHO be just a workaround to the real problem - which is assumption that all attributes in the attribute group are dev_attrs. Such silent assumptions are fragile as we see :)
If we opt treating all attributes in the group as device_attrs also in the future, then (in my not always so humble opinion) the APIs should force this - but I'm not sure how to do that.
Best regards -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |