[PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The IIO channel demultiplexer code is there to deal with a mismatch between
the channels captured and those requested by user space either due to
driver supporting only particular channel combinations
(available_scan_masks) or due to multiple concurrent consumers (e.g.
userspace IIO buffered interfaces and an inkernel consumer such as a
touch screen).

Whilst this code is exercised by many drivers, a corner case has been
hiding there all along.

Consider an input of (postfix is the channel size)

a_32, b_32, c_32, d_32, ts_64

and desired output of

a32, b_32, ts_64

the current code ends up with

a32, b_32, c_32, d_32

because of a failure to iterate over the tail of unwanted channels
(here c_32 and d_32).

Fix this by adding the code to walk the channels in the gap.

Reported-by: Jianping Shen <Jianping.Shen@de.bosch.com>
Closes: https://lore.kernel.org/all/AM8PR10MB4721FB1A78F25B204BE3A26ACD5FA@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM/
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---

This is an RFT because whilst the reporter has confirmed that it works
for their case, it is touching fiddly code and I don't have the original
set of tests to hand that I used when writing that code.

Hence I'd like a lot of eyes + some testing on this.  A number of drivers
should have hit this such as some of the larger IMUs, but only with
very specific channel combinations that perhaps were never of interest
to users.

Thanks to Jiangping Shen for all their hard work figuring out what
was wrong!

Whilst this is being tested I'll try to figure out a Fixes tag.
There is some code movement so needs more digging that I have time for
today.

 drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a80f7cc25a27..d7dd9764091d 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1082,6 +1082,20 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 		out_loc += length;
 		in_loc += length;
 	}
+	/* Walk remaining bits of active_scan_mask */
+	in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
+			       in_ind + 1);
+	while (in_ind != masklength) {
+		ret = iio_storage_bytes_for_si(indio_dev, in_ind);
+		if (ret < 0)
+			goto error_clear_mux_table;
+
+		length = ret;
+		/* Make sure we are aligned */
+		in_loc = roundup(in_loc, length) + length;
+		in_ind = find_next_bit(indio_dev->active_scan_mask,
+				       masklength, in_ind + 1);
+	}
 	/* Relies on scan_timestamp being last */
 	if (buffer->scan_timestamp) {
 		ret = iio_storage_bytes_for_timestamp(indio_dev);
-- 
2.50.1

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Andy Shevchenko]

On Sat, Aug 2, 2025 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The IIO channel demultiplexer code is there to deal with a mismatch between
> the channels captured and those requested by user space either due to
> driver supporting only particular channel combinations
> (available_scan_masks) or due to multiple concurrent consumers (e.g.
> userspace IIO buffered interfaces and an inkernel consumer such as a
> touch screen).
>
> Whilst this code is exercised by many drivers, a corner case has been
> hiding there all along.
>
> Consider an input of (postfix is the channel size)

size in bits

> a_32, b_32, c_32, d_32, ts_64
>
> and desired output of
>
> a32, b_32, ts_64

You missed underscore here and below.

> the current code ends up with
>
> a32, b_32, c_32, d_32
>
> because of a failure to iterate over the tail of unwanted channels
> (here c_32 and d_32).
>
> Fix this by adding the code to walk the channels in the gap.
>
> Reported-by: Jianping Shen <Jianping.Shen@de.bosch.com>
> Closes: https://lore.kernel.org/all/AM8PR10MB4721FB1A78F25B204BE3A26ACD5FA@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM/
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Fixes?

...

> Whilst this is being tested I'll try to figure out a Fixes tag.
> There is some code movement so needs more digging that I have time for
> today.

Ah, okay.


> +       /* Walk remaining bits of active_scan_mask */
> +       in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> +                              in_ind + 1);
> +       while (in_ind != masklength) {

for_each_set_bit_from() ?

> +               ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> +               if (ret < 0)
> +                       goto error_clear_mux_table;
> +
> +               length = ret;
> +               /* Make sure we are aligned */
> +               in_loc = roundup(in_loc, length) + length;
> +               in_ind = find_next_bit(indio_dev->active_scan_mask,
> +                                      masklength, in_ind + 1);
> +       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Jonathan Cameron]

On Sun, 3 Aug 2025 21:46:50 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Aug 2, 2025 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > The IIO channel demultiplexer code is there to deal with a mismatch between
> > the channels captured and those requested by user space either due to
> > driver supporting only particular channel combinations
> > (available_scan_masks) or due to multiple concurrent consumers (e.g.
> > userspace IIO buffered interfaces and an inkernel consumer such as a
> > touch screen).
> >
> > Whilst this code is exercised by many drivers, a corner case has been
> > hiding there all along.
> >
> > Consider an input of (postfix is the channel size)  
> 
> size in bits
> 
> > a_32, b_32, c_32, d_32, ts_64
> >
> > and desired output of
> >
> > a32, b_32, ts_64  
> 
> You missed underscore here and below.
> 
> > the current code ends up with
> >
> > a32, b_32, c_32, d_32
> >
> > because of a failure to iterate over the tail of unwanted channels
> > (here c_32 and d_32).
> >
> > Fix this by adding the code to walk the channels in the gap.
> >
> > Reported-by: Jianping Shen <Jianping.Shen@de.bosch.com>
> > Closes: https://lore.kernel.org/all/AM8PR10MB4721FB1A78F25B204BE3A26ACD5FA@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM/
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Fixes?
> 
> ...
> 
> > Whilst this is being tested I'll try to figure out a Fixes tag.
> > There is some code movement so needs more digging that I have time for
> > today.  
> 
> Ah, okay.
> 
> 
> > +       /* Walk remaining bits of active_scan_mask */
> > +       in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > +                              in_ind + 1);
> > +       while (in_ind != masklength) {  
> 
> for_each_set_bit_from() ?

Would work but then takes the style away from the code above that this
effectively duplicates.  I'd need to have a closer look to see if we
can potentially convert that as well.  I don't want two very different
looking bits of code effectively doing the same thing under subtly
different constraints.

Jonathan

> 
> > +               ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > +               if (ret < 0)
> > +                       goto error_clear_mux_table;
> > +
> > +               length = ret;
> > +               /* Make sure we are aligned */
> > +               in_loc = roundup(in_loc, length) + length;
> > +               in_ind = find_next_bit(indio_dev->active_scan_mask,
> > +                                      masklength, in_ind + 1);
> > +       }  
> 
> 

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Andy Shevchenko]

On Mon, Aug 4, 2025 at 12:48 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Sun, 3 Aug 2025 21:46:50 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sat, Aug 2, 2025 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote:

...

> > > +       /* Walk remaining bits of active_scan_mask */
> > > +       in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > > +                              in_ind + 1);
> > > +       while (in_ind != masklength) {
> >
> > for_each_set_bit_from() ?
>
> Would work but then takes the style away from the code above that this
> effectively duplicates.  I'd need to have a closer look to see if we
> can potentially convert that as well.  I don't want two very different
> looking bits of code effectively doing the same thing under subtly
> different constraints.

I see. But can we have this as a fix followed by the conversion patch
in one series, so we won't forget about that?

> > > +               ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > > +               if (ret < 0)
> > > +                       goto error_clear_mux_table;
> > > +
> > > +               length = ret;
> > > +               /* Make sure we are aligned */
> > > +               in_loc = roundup(in_loc, length) + length;
> > > +               in_ind = find_next_bit(indio_dev->active_scan_mask,
> > > +                                      masklength, in_ind + 1);
> > > +       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Jonathan Cameron]

On Mon, 4 Aug 2025 14:52:23 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 4, 2025 at 12:48 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Sun, 3 Aug 2025 21:46:50 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Sat, Aug 2, 2025 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> 
> ...
> 
> > > > +       /* Walk remaining bits of active_scan_mask */
> > > > +       in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > > > +                              in_ind + 1);
> > > > +       while (in_ind != masklength) {  
> > >
> > > for_each_set_bit_from() ?  
> >
> > Would work but then takes the style away from the code above that this
> > effectively duplicates.  I'd need to have a closer look to see if we
> > can potentially convert that as well.  I don't want two very different
> > looking bits of code effectively doing the same thing under subtly
> > different constraints.  
> 
> I see. But can we have this as a fix followed by the conversion patch
> in one series, so we won't forget about that?

Maybe.  This is a nasty bug so I'm not sure I want to delay the fix
whilst I work out how to cleanup the section above.

I'll see if I can find time to work on this. The time is mostly on
building test cases rather than the code.
> 
> > > > +               ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > > > +               if (ret < 0)
> > > > +                       goto error_clear_mux_table;
> > > > +
> > > > +               length = ret;
> > > > +               /* Make sure we are aligned */
> > > > +               in_loc = roundup(in_loc, length) + length;
> > > > +               in_ind = find_next_bit(indio_dev->active_scan_mask,
> > > > +                                      masklength, in_ind + 1);
> > > > +       }  
> 
> 

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Nuno Sá]

On Sat, Aug 02, 2025 at 06:15:39PM +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> The IIO channel demultiplexer code is there to deal with a mismatch between
> the channels captured and those requested by user space either due to
> driver supporting only particular channel combinations
> (available_scan_masks) or due to multiple concurrent consumers (e.g.
> userspace IIO buffered interfaces and an inkernel consumer such as a
> touch screen).
> 
> Whilst this code is exercised by many drivers, a corner case has been
> hiding there all along.
> 
> Consider an input of (postfix is the channel size)
> 
> a_32, b_32, c_32, d_32, ts_64
> 
> and desired output of
> 
> a32, b_32, ts_64
> 
> the current code ends up with
> 
> a32, b_32, c_32, d_32
> 
> because of a failure to iterate over the tail of unwanted channels
> (here c_32 and d_32).
> 
> Fix this by adding the code to walk the channels in the gap.
> 
> Reported-by: Jianping Shen <Jianping.Shen@de.bosch.com>
> Closes: https://lore.kernel.org/all/AM8PR10MB4721FB1A78F25B204BE3A26ACD5FA@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM/
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> 
> This is an RFT because whilst the reporter has confirmed that it works
> for their case, it is touching fiddly code and I don't have the original
> set of tests to hand that I used when writing that code.
> 
> Hence I'd like a lot of eyes + some testing on this.  A number of drivers
> should have hit this such as some of the larger IMUs, but only with
> very specific channel combinations that perhaps were never of interest
> to users.
> 
> Thanks to Jiangping Shen for all their hard work figuring out what
> was wrong!
> 
> Whilst this is being tested I'll try to figure out a Fixes tag.
> There is some code movement so needs more digging that I have time for
> today.
> 
>  drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a80f7cc25a27..d7dd9764091d 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1082,6 +1082,20 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>  		out_loc += length;
>  		in_loc += length;
>  	}
> +	/* Walk remaining bits of active_scan_mask */
> +	in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> +			       in_ind + 1);

I wonder if it matters to check that in_ind + 1 is in fact lower than
masklength? Not that it will be an issue for find_next_bit() but we will
fail the expectation:

if (unlikely(__start >= sz)) [1]

And being this a sensible path, I thought it's worth (at least) questioning...

Other than that kind of nit comment, patch looks good.

[1]: https://elixir.bootlin.com/linux/v6.16/source/lib/find_bit.c#L50

Thanks!
- Nuno Sá

> +	while (in_ind != masklength) {
> +		ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> +		if (ret < 0)
> +			goto error_clear_mux_table;
> +
> +		length = ret;
> +		/* Make sure we are aligned */
> +		in_loc = roundup(in_loc, length) + length;
> +		in_ind = find_next_bit(indio_dev->active_scan_mask,
> +				       masklength, in_ind + 1);
> +	}
>  	/* Relies on scan_timestamp being last */
>  	if (buffer->scan_timestamp) {
>  		ret = iio_storage_bytes_for_timestamp(indio_dev);
> -- 
> 2.50.1
> 

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Andy Shevchenko]

On Mon, Aug 4, 2025 at 5:37 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Sat, Aug 02, 2025 at 06:15:39PM +0100, Jonathan Cameron wrote:

...

> > +     /* Walk remaining bits of active_scan_mask */
> > +     in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > +                            in_ind + 1);
>
> I wonder if it matters to check that in_ind + 1 is in fact lower than
> masklength? Not that it will be an issue for find_next_bit() but we will
> fail the expectation:
>
> if (unlikely(__start >= sz)) [1]
>
> And being this a sensible path, I thought it's worth (at least) questioning...

It doesn't matter. The find_*_bit() are all aligned to return sz for
anything "not found anymore" cases, so it will be okay.

> Other than that kind of nit comment, patch looks good.
>
> [1]: https://elixir.bootlin.com/linux/v6.16/source/lib/find_bit.c#L50

> > +     while (in_ind != masklength) {
> > +             ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > +             if (ret < 0)
> > +                     goto error_clear_mux_table;
> > +
> > +             length = ret;
> > +             /* Make sure we are aligned */
> > +             in_loc = roundup(in_loc, length) + length;
> > +             in_ind = find_next_bit(indio_dev->active_scan_mask,
> > +                                    masklength, in_ind + 1);
> > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Nuno Sá]

On Mon, Aug 04, 2025 at 06:02:22PM +0200, Andy Shevchenko wrote:
> On Mon, Aug 4, 2025 at 5:37 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Sat, Aug 02, 2025 at 06:15:39PM +0100, Jonathan Cameron wrote:
> 
> ...
> 
> > > +     /* Walk remaining bits of active_scan_mask */
> > > +     in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > > +                            in_ind + 1);
> >
> > I wonder if it matters to check that in_ind + 1 is in fact lower than
> > masklength? Not that it will be an issue for find_next_bit() but we will
> > fail the expectation:
> >
> > if (unlikely(__start >= sz)) [1]
> >
> > And being this a sensible path, I thought it's worth (at least) questioning...
> 
> It doesn't matter. The find_*_bit() are all aligned to return sz for
> anything "not found anymore" cases, so it will be okay.

I know :):

"...Not that it will be an issue for find_next_bit()..."

I was mostly worried by performance as we'll have a compiler hint that
will pretty much fail (that ´if (unlikely(__start >= sz))' for every sample we push and 
I guess the CPU will have to unroll that prediction. Maybe it will be smart enough to
adapt.

But as I said, it might be neglectable but still worth at least
questioning...

- Nuno Sá

> 
> > Other than that kind of nit comment, patch looks good.
> >
> > [1]: https://elixir.bootlin.com/linux/v6.16/source/lib/find_bit.c#L50
> 
> > > +     while (in_ind != masklength) {
> > > +             ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > > +             if (ret < 0)
> > > +                     goto error_clear_mux_table;
> > > +
> > > +             length = ret;
> > > +             /* Make sure we are aligned */
> > > +             in_loc = roundup(in_loc, length) + length;
> > > +             in_ind = find_next_bit(indio_dev->active_scan_mask,
> > > +                                    masklength, in_ind + 1);
> > > +     }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Andy Shevchenko]

On Tue, Aug 5, 2025 at 10:16 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Mon, Aug 04, 2025 at 06:02:22PM +0200, Andy Shevchenko wrote:
> > On Mon, Aug 4, 2025 at 5:37 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > On Sat, Aug 02, 2025 at 06:15:39PM +0100, Jonathan Cameron wrote:

...

> > > > +     /* Walk remaining bits of active_scan_mask */
> > > > +     in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > > > +                            in_ind + 1);
> > >
> > > I wonder if it matters to check that in_ind + 1 is in fact lower than
> > > masklength? Not that it will be an issue for find_next_bit() but we will
> > > fail the expectation:
> > >
> > > if (unlikely(__start >= sz)) [1]
> > >
> > > And being this a sensible path, I thought it's worth (at least) questioning...
> >
> > It doesn't matter. The find_*_bit() are all aligned to return sz for
> > anything "not found anymore" cases, so it will be okay.
>
> I know :):
>
> "...Not that it will be an issue for find_next_bit()..."
>
> I was mostly worried by performance as we'll have a compiler hint that
> will pretty much fail (that ´if (unlikely(__start >= sz))' for every sample we push and
> I guess the CPU will have to unroll that prediction. Maybe it will be smart enough to
> adapt.

Ah, I see now. Yeah, there might be a hint to skip the branch which is
unlikely() for.

> But as I said, it might be neglectable but still worth at least
> questioning...
>
> > > Other than that kind of nit comment, patch looks good.
> > >
> > > [1]: https://elixir.bootlin.com/linux/v6.16/source/lib/find_bit.c#L50
> >
> > > > +     while (in_ind != masklength) {
> > > > +             ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > > > +             if (ret < 0)
> > > > +                     goto error_clear_mux_table;
> > > > +
> > > > +             length = ret;
> > > > +             /* Make sure we are aligned */
> > > > +             in_loc = roundup(in_loc, length) + length;
> > > > +             in_ind = find_next_bit(indio_dev->active_scan_mask,
> > > > +                                    masklength, in_ind + 1);
> > > > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Jonathan Cameron]

On Tue, 5 Aug 2025 14:41:03 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Aug 5, 2025 at 10:16 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Mon, Aug 04, 2025 at 06:02:22PM +0200, Andy Shevchenko wrote:  
> > > On Mon, Aug 4, 2025 at 5:37 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > On Sat, Aug 02, 2025 at 06:15:39PM +0100, Jonathan Cameron wrote:  
> 
> ...
> 
> > > > > +     /* Walk remaining bits of active_scan_mask */
> > > > > +     in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > > > > +                            in_ind + 1);  
> > > >
> > > > I wonder if it matters to check that in_ind + 1 is in fact lower than
> > > > masklength? Not that it will be an issue for find_next_bit() but we will
> > > > fail the expectation:
> > > >
> > > > if (unlikely(__start >= sz)) [1]
> > > >
> > > > And being this a sensible path, I thought it's worth (at least) questioning...  
> > >
> > > It doesn't matter. The find_*_bit() are all aligned to return sz for
> > > anything "not found anymore" cases, so it will be okay.  
> >
> > I know :):
> >
> > "...Not that it will be an issue for find_next_bit()..."
> >
> > I was mostly worried by performance as we'll have a compiler hint that
> > will pretty much fail (that ´if (unlikely(__start >= sz))' for every sample we push and
> > I guess the CPU will have to unroll that prediction. Maybe it will be smart enough to
> > adapt.  
> 
> Ah, I see now. Yeah, there might be a hint to skip the branch which is
> unlikely() for.

Assuming I remember how this all works...

This doesn't happen on the fast path (pushing samples)
It's a setup activity on a buffer being enabled.  The code is
generating a table of offsets and sizes that are then used to
on every sample.  So I don't think it's worth bothering to optimize it.

Jonathan

> 
> > But as I said, it might be neglectable but still worth at least
> > questioning...
> >  
> > > > Other than that kind of nit comment, patch looks good.
> > > >
> > > > [1]: https://elixir.bootlin.com/linux/v6.16/source/lib/find_bit.c#L50  
> > >  
> > > > > +     while (in_ind != masklength) {
> > > > > +             ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > > > > +             if (ret < 0)
> > > > > +                     goto error_clear_mux_table;
> > > > > +
> > > > > +             length = ret;
> > > > > +             /* Make sure we are aligned */
> > > > > +             in_loc = roundup(in_loc, length) + length;
> > > > > +             in_ind = find_next_bit(indio_dev->active_scan_mask,
> > > > > +                                    masklength, in_ind + 1);
> > > > > +     }  
> 

Re: [PATCH RFT] iio: Fix core buffer demux failure to account for unwanted channels at tail

[Nuno Sá]

On Wed, Aug 06, 2025 at 04:36:13PM +0100, Jonathan Cameron wrote:
> On Tue, 5 Aug 2025 14:41:03 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Tue, Aug 5, 2025 at 10:16 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > On Mon, Aug 04, 2025 at 06:02:22PM +0200, Andy Shevchenko wrote:  
> > > > On Mon, Aug 4, 2025 at 5:37 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > > On Sat, Aug 02, 2025 at 06:15:39PM +0100, Jonathan Cameron wrote:  
> > 
> > ...
> > 
> > > > > > +     /* Walk remaining bits of active_scan_mask */
> > > > > > +     in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
> > > > > > +                            in_ind + 1);  
> > > > >
> > > > > I wonder if it matters to check that in_ind + 1 is in fact lower than
> > > > > masklength? Not that it will be an issue for find_next_bit() but we will
> > > > > fail the expectation:
> > > > >
> > > > > if (unlikely(__start >= sz)) [1]
> > > > >
> > > > > And being this a sensible path, I thought it's worth (at least) questioning...  
> > > >
> > > > It doesn't matter. The find_*_bit() are all aligned to return sz for
> > > > anything "not found anymore" cases, so it will be okay.  
> > >
> > > I know :):
> > >
> > > "...Not that it will be an issue for find_next_bit()..."
> > >
> > > I was mostly worried by performance as we'll have a compiler hint that
> > > will pretty much fail (that ´if (unlikely(__start >= sz))' for every sample we push and
> > > I guess the CPU will have to unroll that prediction. Maybe it will be smart enough to
> > > adapt.  
> > 
> > Ah, I see now. Yeah, there might be a hint to skip the branch which is
> > unlikely() for.
> 
> Assuming I remember how this all works...
> 
> This doesn't happen on the fast path (pushing samples)
> It's a setup activity on a buffer being enabled.  The code is
> generating a table of offsets and sizes that are then used to
> on every sample.  So I don't think it's worth bothering to optimize it.

Right! For some reason I assumes this was running on every push (which
would not be a great idea anyway we did it)

That said, the change LGTM:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> 
> Jonathan
> 
> > 
> > > But as I said, it might be neglectable but still worth at least
> > > questioning...
> > >  
> > > > > Other than that kind of nit comment, patch looks good.
> > > > >
> > > > > [1]: https://elixir.bootlin.com/linux/v6.16/source/lib/find_bit.c#L50  
> > > >  
> > > > > > +     while (in_ind != masklength) {
> > > > > > +             ret = iio_storage_bytes_for_si(indio_dev, in_ind);
> > > > > > +             if (ret < 0)
> > > > > > +                     goto error_clear_mux_table;
> > > > > > +
> > > > > > +             length = ret;
> > > > > > +             /* Make sure we are aligned */
> > > > > > +             in_loc = roundup(in_loc, length) + length;
> > > > > > +             in_ind = find_next_bit(indio_dev->active_scan_mask,
> > > > > > +                                    masklength, in_ind + 1);
> > > > > > +     }  
> > 
>