Re: [PATCH] iio: sanity check available_scan_masks array
From: Matti Vaittinen
Date: Fri Oct 06 2023 - 02:05:18 EST
On 10/5/23 18:30, Jonathan Cameron wrote:
On Tue, 3 Oct 2023 12:49:45 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxxx>
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
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! ~~