Re: [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers

[<Claudiu.Beznea@microchip.com>]

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! ~~