Re: [PATCH] iio: buffer: Fix demux update

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 13 Nov 2020 07:55:21 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Sent: Thursday, November 12, 2020 6:28 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org; Jonathan Cameron <jic23@kernel.org>;
> > Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald-Stadler
> > <pmeerw@pmeerw.net>
> > Subject: Re: [PATCH] iio: buffer: Fix demux update
> > 
> > 
> > On Thu, 12 Nov 2020 15:43:22 +0100
> > Nuno Sá <nuno.sa@analog.com> 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@analog.com>  
> > 
> > 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
> >