Message-ID: <fffbf40f-c2be-60d6-8a6f-4790a8e309ea@gmail.com>
Date:   Fri, 6 Oct 2023 09:05:08 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
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@...il.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@...nel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.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! ~~

Powered by blists - more mailing lists