lkml.org 
[lkml]   [2023]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] iio: sanity check available_scan_masks array
    On 10/5/23 18:30, Jonathan Cameron wrote:
    > On Tue, 3 Oct 2023 12:49:45 +0300
    > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
    >
    >> When IIO goes through the available scan masks in order to select the
    >> best suiting one, it will just accept the first listed subset of channels
    >> which meets the user's requirements. If driver lists a mask which is a
    >> subset of some of the masks previously in the array of
    >> avaliable_scan_masks, then the latter one will never be selected.
    >>
    >> Add a warning if driver registers masks which can't be used due to the
    >> available_scan_masks-array ordering.
    >>
    >> Suggested-by: Jonathan Cameron <jic23@kernel.org>
    >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
    > Hi Matti
    >
    > Thanks for doing this. A few comments inline + maybe we need to think
    > about a unit test for the matching code. I feel we aren't pushing the
    > corners of that in any drivers so far so it might bite us later.

    I am extremely conservative what comes to adding unit tests. I have seen
    some projects where the amount of existing unit test code made code
    changes very much very slow - stopping people doing any improvements.
    Basically, no one wanted to touch the existing code unless it was
    absolutely must because even a minor code change caused several tests to
    break. OTOH, that unit test setup did not only test that end result of a
    function was expected - it did also check the calls done from the
    function to be tested - checking for example that the certain prints
    appeared with certain inputs and so on. That project stopped being fun
    very quickly...

    But yes. After spending a while reading IIO code, I agree that _some_
    parts of it could benefit from a few carefully designed unit tests. (And
    sorry, I haven't checked what tests are existing already - so may be
    there already is relevant tests) :) Channel data demuxing and the mask
    handling are indeed the first to come to my mind ;) I wouldn't dare to
    touch that part without some testing.

    > Still that's a job for another day.

    Hey, we need to have something for tomorrow, right? :)

    >
    >>
    >> ---
    >> The change was suggested by Jonathan here:
    >> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/
    >> ---
    >> drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++
    >> 1 file changed, 57 insertions(+)
    >>
    >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
    >> index c77745b594bd..d4f37f4eeec0 100644
    >> --- a/drivers/iio/industrialio-core.c
    >> +++ b/drivers/iio/industrialio-core.c
    >> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev)
    >>
    >> static const struct iio_buffer_setup_ops noop_ring_setup_ops;
    >>
    >> +static void iio_sanity_check_avail_scan_masks(struct iio_dev *indio_dev)
    >> +{
    >> + unsigned int num_masks, masklength, longs_per_mask;
    >> + const unsigned long *av_masks;
    >> + int i;
    >> +
    >> + av_masks = indio_dev->available_scan_masks;
    >> + masklength = indio_dev->masklength;
    >> + longs_per_mask = BITS_TO_LONGS(masklength);
    >> +
    >> + if (bitmap_empty(av_masks, masklength))
    >> + dev_warn(indio_dev->dev.parent, "empty scan mask\n");
    >
    > They'd definitely notice this one as you'd never be able to enable the
    > buffer - if someone hasn't tested that, then meh. Still this function
    > is called sanity_check so might as well check for insanity.
    >
    >
    >> +
    >> + for (num_masks = 0; *av_masks; num_masks++)
    >
    > I think we can't just check *av_masks - need bitmap_empty() as first
    > long might be 0 but could be bits set in the next one.

    Ah. In case where we have bitmap consisting of many longs. Indeed. By
    the way, I think I stole this check from the actual matching code - we
    should probably fix it as well.

    >> + av_masks += longs_per_mask;
    > hmm. Makes me wonder if the available scan mask stuff actually works
    > for large numbers of channels (so more than one long).

    After you pointed out the problem in for-condition - it probably does
    not work for all cases.

    > I don't think
    > we have any drivers that both have large channel counts and use
    > available_scan_masks. The code is there to support matching in this
    > case but probably wants a selftest at somepoint to make sure it will work
    > if such a device comes along...
    >
    >
    >> +
    >> + if (num_masks < 2)
    >> + return;
    >
    > Not sure it's worth bothering with this early exit route. The loops
    > will be trivial anyway if num_masks == 1.

    I probably thought about the num_masks == 0 when adding this check.
    Decided we might just early exit while checking.

    >> +
    >> + av_masks = indio_dev->available_scan_masks;
    >> +
    >> + /*
    >> + * Go through all the masks from first to one before the last, and see
    >> + * that no mask found later from the available_scan_masks array is a
    >> + * subset of mask found earlier. If this happens, then the mask found
    >> + * later will never get used because scanning the array is stopped when
    >> + * the first suitable mask is found. Drivers should order the array of
    >> + * available masks in the order of preference (presumably the least
    >> + * costy to access masks first).
    >> + */
    >> + for (i = 0; i < num_masks - 1; i++) {
    >> + const unsigned long *mask1;
    >> + int j;
    >> +
    >> + mask1 = av_masks + i * longs_per_mask;
    >> + for (j = i + 1; j < num_masks; j++) {
    >> + const unsigned long *mask2;
    >> +
    >> + mask2 = av_masks + j * longs_per_mask;
    >> + if (bitmap_subset(mask2, mask1, masklength))
    >> + dev_warn(indio_dev->dev.parent,
    >> + "available_scan_mask %d subset of %d. Never used\n",
    >> + j, i);
    >> + }
    >> + }
    >> +}
    >> +
    >> int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
    >> {
    >> struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
    >> @@ -1934,6 +1981,16 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
    >> goto error_unreg_debugfs;
    >> }
    >>
    >> + /*
    >> + * In order to not wreck utter havoc we just warn for now. Might want
    >> + * to convert this to a failure after people have had time to act upon
    >> + * the warning. It'd be nice to check this earlier, but we need the
    >> + * iio_buffers_alloc_sysfs_and_mask() to have the masklength set.
    >
    > It's not going to break anyone if they get this wrong, they will just waste time
    > and possibly power reading too many channels! So warn is appropriate I think.
    >
    > I'm not sure the comment adds much in general so I'd slim it down or drop it
    > from v2.

    I'm fine with dropping the comment. My mindset is easily leaning too
    much on developing new drivers when I think of checks like this one.
    It'd be nice to get a noticeable kick immediately when developing a
    driver - but yes, one should be kicked just by the warning alone.

    >
    >> + */
    >> + if (indio_dev->available_scan_masks)
    >> + iio_sanity_check_avail_scan_masks(indio_dev);
    >> +
    > One blank line is enough ;)

    Again... Thanks!

    >> +
    >> ret = iio_device_register_sysfs(indio_dev);
    >> if (ret) {
    >> dev_err(indio_dev->dev.parent,
    >>
    >> base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec

    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: 2023-10-06 08:05    [W:2.741 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site