Messages in this thread | | | Date | Fri, 6 Oct 2023 09:05:08 +0300 | From | Matti Vaittinen <> | Subject | Re: [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! ~~
| |