| Message ID | 20201112144323.28887-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: buffer: Fix demux update | expand |
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? Thanks, Jonathan > --- > drivers/iio/industrialio-buffer.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index a4f6bb96d4f4..276b609d7917 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -865,12 +865,12 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev, > indio_dev->masklength, > in_ind + 1); > while (in_ind != out_ind) { > - in_ind = find_next_bit(indio_dev->active_scan_mask, > - indio_dev->masklength, > - in_ind + 1); > length = iio_storage_bytes_for_si(indio_dev, in_ind); > /* Make sure we are aligned */ > in_loc = roundup(in_loc, length) + length; > + in_ind = find_next_bit(indio_dev->active_scan_mask, > + indio_dev->masklength, > + in_ind + 1); > } > length = iio_storage_bytes_for_si(indio_dev, in_ind); > out_loc = roundup(out_loc, length);
> -----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? - Nuno Sá > Thanks, > > Jonathan >
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 > >
On Sat, 14 Nov 2020 16:26:38 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > 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. > Ah well. I'll take silence as meaning either everyone is happy or no one else is going to read it ;) Applied with a bit more text in the description to highlight that, whilst a bug, it's actually not a common situation. Given timing I've applied this to the togreg branch of iio.git ready for the next merge window. thanks, Jonathan > Good spot! > > Jonathan > > > > > - Nuno Sá > > > > > Thanks, > > > > > > Jonathan > > > >
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index a4f6bb96d4f4..276b609d7917 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -865,12 +865,12 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev, indio_dev->masklength, in_ind + 1); while (in_ind != out_ind) { - in_ind = find_next_bit(indio_dev->active_scan_mask, - indio_dev->masklength, - in_ind + 1); length = iio_storage_bytes_for_si(indio_dev, in_ind); /* Make sure we are aligned */ in_loc = roundup(in_loc, length) + length; + in_ind = find_next_bit(indio_dev->active_scan_mask, + indio_dev->masklength, + in_ind + 1); } length = iio_storage_bytes_for_si(indio_dev, in_ind); out_loc = roundup(out_loc, length);
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> --- drivers/iio/industrialio-buffer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)