Date:   Mon, 25 Sep 2023 13:00:29 +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>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Benjamin Bara <bbara93@...il.com>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/6] iio: try searching for exact scan_mask

On 9/24/23 19:07, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:17:49 +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. This works great for most of the
>> drivers as they can sort the list of channels in the order where
>> the 'least costy' channel selections come first.
>>
>> It may be that in some cases the ordering of the list of available scan
>> masks is not thoroughly considered. We can't really try outsmarting the
>> drivers by selecting the smallest supported subset - as this might not
>> be the 'least costy one' - but we can at least try searching through the
>> list to see if we have an exactly matching mask. It should be sane
>> assumption that if the device can support reading only the exact
>> channels user is interested in, then this should be also the least costy
>> selection - and if it is not and optimization is important, then the
>> driver could consider omitting setting the 'available_scan_mask' and
>> doing demuxing - or just omitting the 'costy exact match' and providing
>> only the more efficient broader selection of channels.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> 
> Whilst I fully agree with the reasoning behind this, I'd rather we
> did an audit of drivers to find any that have a non logical order
> (one came up today in review) and fix them up.
> 
> A quick and dirty grep didn't find it to be a common problem, at least
> partly as most users of this feature only provide one valid mask.

It's always good to hear there is not many problems found :) This patch 
was not inspired by auditing the existing code - it was inspired by the 
fact that I would have wrongly ordered the available_scan_masks for 
bm1390 myself. I just happened to notice the oddity in active_scan_masks 
while I was trying to figure out if it was the driver, IIO or user-space 
code which messed my buffer when I disabled timestamps.

> The few complex corners I found appear to be fine with the expected
> shortest sequences first.
> 
> Defending against driver bugs is losing game if it makes the core
> code more complex to follow by changing stuff in non debug paths.

I think I agree, although I could argue that it depends on the amount of 
added complexity. Still ...

> One option might be to add a trivial check at iio_device_register()

... this suggestion is superior to the check added in this patch.

> that we don't have scan modes that are subsets of modes earlier in the list.
> These lists are fairly short so should be cheap to run.

Yes. And running the check at the registration phase should not be a big 
problem. And, if it appears to be a problem, then we can add a 
registration variant which omits the checks for those rare drivers which 
would _really_ be hurt by the few extra cycles spent on registration.

> That would incorporate ensuring exact matches come earlier by default.

Yes. I like the idea, wish I had invented it myself ;)

> 
> Jonathan
> 
> 
>> ---
>>   drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
>>   1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index 176d31d9f9d8..e97396623373 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -411,19 +411,32 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
>>   						const unsigned long *mask,
>>   						bool strict)
>>   {
>> +	const unsigned long *first_subset = NULL;
>> +
>>   	if (bitmap_empty(mask, masklength))
>>   		return NULL;
>> -	while (*av_masks) {
>> -		if (strict) {
>> +
>> +	if (strict) {
>> +		while (*av_masks) {
>>   			if (bitmap_equal(mask, av_masks, masklength))
>>   				return av_masks;
>> -		} else {
>> -			if (bitmap_subset(mask, av_masks, masklength))
>> -				return av_masks;
>> +
>> +			av_masks += BITS_TO_LONGS(masklength);
>>   		}
>> +
>> +		return NULL;
>> +	}
>> +	while (*av_masks) {
>> +		if (bitmap_equal(mask, av_masks, masklength))
>> +			return av_masks;
>> +
>> +		if (!first_subset && bitmap_subset(mask, av_masks, masklength))
>> +			first_subset = av_masks;
>> +
>>   		av_masks += BITS_TO_LONGS(masklength);
>>   	}
>> -	return NULL;
>> +
>> +	return first_subset;
>>   }
>>   
>>   static bool iio_validate_scan_mask(struct iio_dev *indio_dev,
> 

-- 
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