On Fri, 13 Nov 2020 07:55:21 +0000
"Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Sent: Thursday, November 12, 2020 6:28 PM
> > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> > Cc: linux-iio@xxxxxxxxxxxxxxx; Jonathan Cameron <jic23@xxxxxxxxxx>;
> > Lars-Peter Clausen <lars@xxxxxxxxxx>; Peter Meerwald-Stadler
> > <pmeerw@xxxxxxxxxx>
> > Subject: Re: [PATCH] iio: buffer: Fix demux update
> >
> >
> > On Thu, 12 Nov 2020 15:43:22 +0100
> > Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
> >
> > > When updating the buffer demux, we will skip a scan element from
> > the
> > > device in the case `in_ind != out_ind` and we enter the while loop.
> > > in_ind should only be refreshed with `find_next_bit()` in the end of
> > the
> > > loop.
> > >
> > > Fixes: 5ada4ea9be16 ("staging:iio: add demux optionally to path from
> > device to buffer")
> > > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> >
> > Yikes that's been there a long time.
> >
> > Could you provide an example of a particular layout and the result of
> > this being wrong?
> >
>
> Hi Jonathan,
>
> Let's say:
>
> iio_dev_mask: 0x0111
> buffer_mask: 0x0100
>
> We would get out_ind = 2 and in_ind = 0 and enter the loop. In the first
> iteration we call find_next_bit() before doing the in_ind=0 computation which means we
> will skip it and go directly to bit 1... And if we continue the path flow, we see that bit 2 will
> be computed two times, so if we are lucky and scan_index0_len == scan_index2_len this
> will go unnoticed...
>
> Honestly, I didn't test this but it looks one of those things more or less clear by reading
> the code or am I missing something here?
Mostly I was wondering why it hadn't bitten us before. I think you've identified
why with your "if we are lucky and scan_index0_len == scan_index2_len" then this will
go unnoticed.
It's very rare (though not unheard of) for a device to have it's main channels
of different widths (timestamp doesn't matter for this as it is always at the
end). The demux also only kicks in if we have a restricted channel
mask (or are using a kfifo and a buffer_cb which is rather rare). I suspect
we have few if any devices that actually run into this problem.
I guess I originally tested this code with devices I had at the time and none of
them would have tripped this.
Anyhow, whilst I agree with your analysis I'd like to leave this on list for
perhaps another week before applying it on the basis I'm paranoid and would
ideally like a few more eyes on this.
Good spot!
Jonathan
>
> - Nuno Sá
>
> > Thanks,
> >
> > Jonathan
> >
