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:4.337 / U:0.756 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site