lkml.org 
[lkml]   [2022]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers
Date
On 19.09.2022 11:52, Vaittinen, Matti wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 9/9/22 11:12, Vaittinen, Matti wrote:
>> 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.

I've checked it on sama5d2_xplained board on v6.0 and it returns (null) for
both hwfifo_watermark_max and hwfifo_watermark_min:

# cat hwfifo_watermark_max
(null)
# cat hwfifo_watermark_min
(null)


With your series at [1] I have:
# cat hwfifo_watermark_max
128
# cat hwfifo_watermark_min
2

Thank you,
Claudiu Beznea

[1] https://patchwork.kernel.org/project/linux-iio/list/?series=682707

> 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 :)
>
> I would like to add at least the at91-sama5d2_adc (conditonally
> registers the IIO_CONST_ATTR for triggered-buffer) to the list of
> devices that could be potentially tested. I hope some of these devices
> had a user who could either make us worried and verify my assumption -
> or make me ashamed but rest of us relieved :) Eg - I second my request
> for testing this - and add potential owners of at91-sama5d2_adc to the list.
>
>> 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.
>>
>
> and at91-sama5d2_adc
>
> //snip
>
>> 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.
>
> Yours,
> -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~

\
 
 \ /
  Last update: 2022-10-06 10:34    [W:0.143 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site