Re: [PATCH 1/2] iio: Fix scan mask subset check logic

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 29 Apr 2025 12:02:12 -0300
Fabio Estevam <festevam@gmail.com> wrote:

> From: Fabio Estevam <festevam@denx.de>
> 
> Since commit 2718f15403fb ("iio: sanity check available_scan_masks array"),
> verbose and misleading warnings are printed for devices like the MAX11601:
> 
> max1363 1-0064: available_scan_mask 8 subset of 0. Never used
> max1363 1-0064: available_scan_mask 9 subset of 0. Never used
> max1363 1-0064: available_scan_mask 10 subset of 0. Never used
> max1363 1-0064: available_scan_mask 11 subset of 0. Never used
> max1363 1-0064: available_scan_mask 12 subset of 0. Never used
> max1363 1-0064: available_scan_mask 13 subset of 0. Never used
> ...
> [warnings continue] 
> 
> Fix the available_scan_masks sanity check logic so that it
> only prints the warning when an element of available_scan_mask
> is in fact a subset of a previous one.
> 
> These warnings incorrectly report that later scan masks are subsets of
> the first one, even when they are not. The issue lies in the logic that
> checks for subset relationships between scan masks.

Add a little on why the logic is wrong would be good.

I stared at this for a while when you first reported it and
couldn't spot it..


> 
> Fix the subset detection to correctly compare each mask only
> against previous masks, and only warn when a true subset is found.
> 
> With this fix, the warning output becomes both correct and more
> informative:
> 
> max1363 1-0064: Mask 7 (0xc) is a subset of mask 6 (0xf) and will be ignored
> 
> Cc: stable@vger.kernel.org
> Fixes: 2718f15403fb ("iio: sanity check available_scan_masks array")
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
>  drivers/iio/industrialio-core.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 6a6568d4a2cb..855d5fd3e6b2 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1904,6 +1904,11 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev)
>  
>  static const struct iio_buffer_setup_ops noop_ring_setup_ops;
>  
> +static int is_subset(unsigned long a, unsigned long b)
> +{
> +	return (a & ~b) == 0;
> +}
> +
>  static void iio_sanity_check_avail_scan_masks(struct iio_dev *indio_dev)
>  {
>  	unsigned int num_masks, masklength, longs_per_mask;
> @@ -1947,21 +1952,13 @@ static void iio_sanity_check_avail_scan_masks(struct iio_dev *indio_dev)
>  	 * 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))
> +	for (i = 1; i < num_masks; ++i)
> +		for (int j = 0; j < i; ++j)
> +			if (is_subset(av_masks[i], av_masks[j]))

Why move away from the bitmap_subset() call?

There are some broken corners for large bitmaps but I'd
rather not make it worse if we can avoid it as the intent is
that we can make these larger bitmaps if needed.


>  				dev_warn(indio_dev->dev.parent,
> -					 "available_scan_mask %d subset of %d. Never used\n",
> -					 j, i);
> -		}
> -	}
> +					 "Mask %d (0x%lx) is a subset of mask %d (0x%lx) and will be ignored\n",
> +					 i, av_masks[i], j, av_masks[j]);
>  }
>  
>  /**