* [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions [not found] <20190719041423.19322-1-sashal@kernel.org> @ 2019-07-19 4:14 ` Sasha Levin 2019-07-21 17:23 ` Jonathan Cameron 2019-07-19 4:14 ` [PATCH AUTOSEL 4.4 18/35] iio: iio-utils: Fix possible incorrect mask calculation Sasha Levin 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2019-07-19 4:14 UTC (permalink / raw) To: linux-kernel, stable Cc: Alexandru Ardelean, Denis Ciocca, Jonathan Cameron, Sasha Levin, linux-iio From: Alexandru Ardelean <alexandru.ardelean@analog.com> [ Upstream commit 05b8bcc96278c9ef927a6f25a98e233e55de42e1 ] The iio_triggered_buffer_{predisable,postenable} functions attach/detach the poll functions. For the predisable hook, the disable code should occur before detaching the poll func, and for the postenable hook, the poll func should be attached before the enable code. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Acked-by: Denis Ciocca <denis.ciocca@st.com> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/iio/accel/st_accel_buffer.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/iio/accel/st_accel_buffer.c b/drivers/iio/accel/st_accel_buffer.c index a1e642ee13d6..4f838277184a 100644 --- a/drivers/iio/accel/st_accel_buffer.c +++ b/drivers/iio/accel/st_accel_buffer.c @@ -46,17 +46,19 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev) goto allocate_memory_error; } - err = st_sensors_set_axis_enable(indio_dev, - (u8)indio_dev->active_scan_mask[0]); + err = iio_triggered_buffer_postenable(indio_dev); if (err < 0) goto st_accel_buffer_postenable_error; - err = iio_triggered_buffer_postenable(indio_dev); + err = st_sensors_set_axis_enable(indio_dev, + (u8)indio_dev->active_scan_mask[0]); if (err < 0) - goto st_accel_buffer_postenable_error; + goto st_sensors_set_axis_enable_error; return err; +st_sensors_set_axis_enable_error: + iio_triggered_buffer_predisable(indio_dev); st_accel_buffer_postenable_error: kfree(adata->buffer_data); allocate_memory_error: @@ -65,20 +67,22 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev) static int st_accel_buffer_predisable(struct iio_dev *indio_dev) { - int err; + int err, err2; struct st_sensor_data *adata = iio_priv(indio_dev); - err = iio_triggered_buffer_predisable(indio_dev); - if (err < 0) - goto st_accel_buffer_predisable_error; - err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS); if (err < 0) goto st_accel_buffer_predisable_error; err = st_sensors_set_enable(indio_dev, false); + if (err < 0) + goto st_accel_buffer_predisable_error; st_accel_buffer_predisable_error: + err2 = iio_triggered_buffer_predisable(indio_dev); + if (!err) + err = err2; + kfree(adata->buffer_data); return err; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions 2019-07-19 4:14 ` [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions Sasha Levin @ 2019-07-21 17:23 ` Jonathan Cameron 2019-07-22 6:47 ` Ardelean, Alexandru 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2019-07-21 17:23 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Alexandru Ardelean, Denis Ciocca, Jonathan Cameron, linux-iio On Fri, 19 Jul 2019 00:14:05 -0400 Sasha Levin <sashal@kernel.org> wrote: > From: Alexandru Ardelean <alexandru.ardelean@analog.com> > > [ Upstream commit 05b8bcc96278c9ef927a6f25a98e233e55de42e1 ] > > The iio_triggered_buffer_{predisable,postenable} functions attach/detach > the poll functions. > > For the predisable hook, the disable code should occur before detaching > the poll func, and for the postenable hook, the poll func should be > attached before the enable code. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Acked-by: Denis Ciocca <denis.ciocca@st.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> Hi Sasha, This should do any harm, but I deliberately didn't cc stable on this one. Alex, my assumption on this one is that it was fixing a logical ordering problem, but one that had no visible impact. Whilst the pollfunc will be attached too early, the trigger will be disabled for the whole of this function anyway so it shouldn't cause any visible problem. Is that a correct interpretation? There are going to be a few more similar fixes in the near future as Alex is trying to tidy up various paths so we can do a general bit of refactoring. If I'm too late for this, then not a problem, just noise in the stable release. Jonathan > --- > drivers/iio/accel/st_accel_buffer.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/accel/st_accel_buffer.c b/drivers/iio/accel/st_accel_buffer.c > index a1e642ee13d6..4f838277184a 100644 > --- a/drivers/iio/accel/st_accel_buffer.c > +++ b/drivers/iio/accel/st_accel_buffer.c > @@ -46,17 +46,19 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev) > goto allocate_memory_error; > } > > - err = st_sensors_set_axis_enable(indio_dev, > - (u8)indio_dev->active_scan_mask[0]); > + err = iio_triggered_buffer_postenable(indio_dev); > if (err < 0) > goto st_accel_buffer_postenable_error; > > - err = iio_triggered_buffer_postenable(indio_dev); > + err = st_sensors_set_axis_enable(indio_dev, > + (u8)indio_dev->active_scan_mask[0]); > if (err < 0) > - goto st_accel_buffer_postenable_error; > + goto st_sensors_set_axis_enable_error; > > return err; > > +st_sensors_set_axis_enable_error: > + iio_triggered_buffer_predisable(indio_dev); > st_accel_buffer_postenable_error: > kfree(adata->buffer_data); > allocate_memory_error: > @@ -65,20 +67,22 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev) > > static int st_accel_buffer_predisable(struct iio_dev *indio_dev) > { > - int err; > + int err, err2; > struct st_sensor_data *adata = iio_priv(indio_dev); > > - err = iio_triggered_buffer_predisable(indio_dev); > - if (err < 0) > - goto st_accel_buffer_predisable_error; > - > err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS); > if (err < 0) > goto st_accel_buffer_predisable_error; > > err = st_sensors_set_enable(indio_dev, false); > + if (err < 0) > + goto st_accel_buffer_predisable_error; > > st_accel_buffer_predisable_error: > + err2 = iio_triggered_buffer_predisable(indio_dev); > + if (!err) > + err = err2; > + > kfree(adata->buffer_data); > return err; > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions 2019-07-21 17:23 ` Jonathan Cameron @ 2019-07-22 6:47 ` Ardelean, Alexandru 2019-07-28 15:43 ` Sasha Levin 0 siblings, 1 reply; 6+ messages in thread From: Ardelean, Alexandru @ 2019-07-22 6:47 UTC (permalink / raw) To: sashal, jic23 Cc: stable, Jonathan.Cameron, linux-kernel, denis.ciocca, linux-iio On Sun, 2019-07-21 at 18:23 +0100, Jonathan Cameron wrote: > [External] > > On Fri, 19 Jul 2019 00:14:05 -0400 > Sasha Levin <sashal@kernel.org> wrote: > > > From: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > [ Upstream commit 05b8bcc96278c9ef927a6f25a98e233e55de42e1 ] > > > > The iio_triggered_buffer_{predisable,postenable} functions attach/detach > > the poll functions. > > > > For the predisable hook, the disable code should occur before detaching > > the poll func, and for the postenable hook, the poll func should be > > attached before the enable code. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Acked-by: Denis Ciocca <denis.ciocca@st.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Sasha Levin <sashal@kernel.org> > Hi Sasha, > > This should do any harm, but I deliberately didn't cc stable on > this one. > > Alex, my assumption on this one is that it was fixing a logical > ordering problem, but one that had no visible impact. > Whilst the pollfunc will be attached too early, the trigger > will be disabled for the whole of this function anyway so > it shouldn't cause any visible problem. Is that a correct interpretation? Yes. That is correct. Maybe I'll change the commit title format so that there is no confusion. It's a fix, but not in the sense that something was broken. Just something that is the realm of cleaning up. Thanks Alex > There are going to be a few more similar fixes in the near future > as Alex is trying to tidy up various paths so we can do a general > bit of refactoring. > > If I'm too late for this, then not a problem, just noise > in the stable release. > > Jonathan > > > > --- > > drivers/iio/accel/st_accel_buffer.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/iio/accel/st_accel_buffer.c b/drivers/iio/accel/st_accel_buffer.c > > index a1e642ee13d6..4f838277184a 100644 > > --- a/drivers/iio/accel/st_accel_buffer.c > > +++ b/drivers/iio/accel/st_accel_buffer.c > > @@ -46,17 +46,19 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev) > > goto allocate_memory_error; > > } > > > > - err = st_sensors_set_axis_enable(indio_dev, > > - (u8)indio_dev->active_scan_mask[0]); > > + err = iio_triggered_buffer_postenable(indio_dev); > > if (err < 0) > > goto st_accel_buffer_postenable_error; > > > > - err = iio_triggered_buffer_postenable(indio_dev); > > + err = st_sensors_set_axis_enable(indio_dev, > > + (u8)indio_dev->active_scan_mask[0]); > > if (err < 0) > > - goto st_accel_buffer_postenable_error; > > + goto st_sensors_set_axis_enable_error; > > > > return err; > > > > +st_sensors_set_axis_enable_error: > > + iio_triggered_buffer_predisable(indio_dev); > > st_accel_buffer_postenable_error: > > kfree(adata->buffer_data); > > allocate_memory_error: > > @@ -65,20 +67,22 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev) > > > > static int st_accel_buffer_predisable(struct iio_dev *indio_dev) > > { > > - int err; > > + int err, err2; > > struct st_sensor_data *adata = iio_priv(indio_dev); > > > > - err = iio_triggered_buffer_predisable(indio_dev); > > - if (err < 0) > > - goto st_accel_buffer_predisable_error; > > - > > err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS); > > if (err < 0) > > goto st_accel_buffer_predisable_error; > > > > err = st_sensors_set_enable(indio_dev, false); > > + if (err < 0) > > + goto st_accel_buffer_predisable_error; > > > > st_accel_buffer_predisable_error: > > + err2 = iio_triggered_buffer_predisable(indio_dev); > > + if (!err) > > + err = err2; > > + > > kfree(adata->buffer_data); > > return err; > > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions 2019-07-22 6:47 ` Ardelean, Alexandru @ 2019-07-28 15:43 ` Sasha Levin 0 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2019-07-28 15:43 UTC (permalink / raw) To: Ardelean, Alexandru Cc: jic23, stable, Jonathan.Cameron, linux-kernel, denis.ciocca, linux-iio On Mon, Jul 22, 2019 at 06:47:47AM +0000, Ardelean, Alexandru wrote: >On Sun, 2019-07-21 at 18:23 +0100, Jonathan Cameron wrote: >> [External] >> >> On Fri, 19 Jul 2019 00:14:05 -0400 >> Sasha Levin <sashal@kernel.org> wrote: >> >> > From: Alexandru Ardelean <alexandru.ardelean@analog.com> >> > >> > [ Upstream commit 05b8bcc96278c9ef927a6f25a98e233e55de42e1 ] >> > >> > The iio_triggered_buffer_{predisable,postenable} functions attach/detach >> > the poll functions. >> > >> > For the predisable hook, the disable code should occur before detaching >> > the poll func, and for the postenable hook, the poll func should be >> > attached before the enable code. >> > >> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> >> > Acked-by: Denis Ciocca <denis.ciocca@st.com> >> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> > Signed-off-by: Sasha Levin <sashal@kernel.org> >> Hi Sasha, >> >> This should do any harm, but I deliberately didn't cc stable on >> this one. >> >> Alex, my assumption on this one is that it was fixing a logical >> ordering problem, but one that had no visible impact. >> Whilst the pollfunc will be attached too early, the trigger >> will be disabled for the whole of this function anyway so >> it shouldn't cause any visible problem. Is that a correct interpretation? > >Yes. >That is correct. > >Maybe I'll change the commit title format so that there is no confusion. >It's a fix, but not in the sense that something was broken. >Just something that is the realm of cleaning up. I'll drop it from the queue, thanks! -- Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 4.4 18/35] iio: iio-utils: Fix possible incorrect mask calculation [not found] <20190719041423.19322-1-sashal@kernel.org> 2019-07-19 4:14 ` [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions Sasha Levin @ 2019-07-19 4:14 ` Sasha Levin 2019-07-21 17:27 ` Jonathan Cameron 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2019-07-19 4:14 UTC (permalink / raw) To: linux-kernel, stable Cc: Bastien Nocera, Jonathan Cameron, Sasha Levin, linux-iio From: Bastien Nocera <hadess@hadess.net> [ Upstream commit 208a68c8393d6041a90862992222f3d7943d44d6 ] On some machines, iio-sensor-proxy was returning all 0's for IIO sensor values. It turns out that the bits_used for this sensor is 32, which makes the mask calculation: *mask = (1 << 32) - 1; If the compiler interprets the 1 literals as 32-bit ints, it generates undefined behavior depending on compiler version and optimization level. On my system, it optimizes out the shift, so the mask value becomes *mask = (1) - 1; With a mask value of 0, iio-sensor-proxy will always return 0 for every axis. Avoid incorrect 0 values caused by compiler optimization. See original fix by Brett Dutro <brett.dutro@gmail.com> in iio-sensor-proxy: https://github.com/hadess/iio-sensor-proxy/commit/9615ceac7c134d838660e209726cd86aa2064fd3 Signed-off-by: Bastien Nocera <hadess@hadess.net> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- tools/iio/iio_utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c index 5eb6793f3972..2d0dcd6fc64c 100644 --- a/tools/iio/iio_utils.c +++ b/tools/iio/iio_utils.c @@ -163,9 +163,9 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used, *be = (endianchar == 'b'); *bytes = padint / 8; if (*bits_used == 64) - *mask = ~0; + *mask = ~(0ULL); else - *mask = (1ULL << *bits_used) - 1; + *mask = (1ULL << *bits_used) - 1ULL; *is_signed = (signchar == 's'); if (fclose(sysfsfp)) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 18/35] iio: iio-utils: Fix possible incorrect mask calculation 2019-07-19 4:14 ` [PATCH AUTOSEL 4.4 18/35] iio: iio-utils: Fix possible incorrect mask calculation Sasha Levin @ 2019-07-21 17:27 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2019-07-21 17:27 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Bastien Nocera, Jonathan Cameron, linux-iio On Fri, 19 Jul 2019 00:14:06 -0400 Sasha Levin <sashal@kernel.org> wrote: > From: Bastien Nocera <hadess@hadess.net> > > [ Upstream commit 208a68c8393d6041a90862992222f3d7943d44d6 ] > > On some machines, iio-sensor-proxy was returning all 0's for IIO sensor > values. It turns out that the bits_used for this sensor is 32, which makes > the mask calculation: > > *mask = (1 << 32) - 1; > > If the compiler interprets the 1 literals as 32-bit ints, it generates > undefined behavior depending on compiler version and optimization level. > On my system, it optimizes out the shift, so the mask value becomes > > *mask = (1) - 1; > > With a mask value of 0, iio-sensor-proxy will always return 0 for every axis. > > Avoid incorrect 0 values caused by compiler optimization. > > See original fix by Brett Dutro <brett.dutro@gmail.com> in > iio-sensor-proxy: > https://github.com/hadess/iio-sensor-proxy/commit/9615ceac7c134d838660e209726cd86aa2064fd3 > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> Good catch, I should have cc'd stable on this one. Thanks, Jonathan > --- > tools/iio/iio_utils.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c > index 5eb6793f3972..2d0dcd6fc64c 100644 > --- a/tools/iio/iio_utils.c > +++ b/tools/iio/iio_utils.c > @@ -163,9 +163,9 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used, > *be = (endianchar == 'b'); > *bytes = padint / 8; > if (*bits_used == 64) > - *mask = ~0; > + *mask = ~(0ULL); > else > - *mask = (1ULL << *bits_used) - 1; > + *mask = (1ULL << *bits_used) - 1ULL; > > *is_signed = (signchar == 's'); > if (fclose(sysfsfp)) { ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-28 15:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190719041423.19322-1-sashal@kernel.org>
2019-07-19 4:14 ` [PATCH AUTOSEL 4.4 17/35] iio: st_accel: fix iio_triggered_buffer_{pre,post}enable positions Sasha Levin
2019-07-21 17:23 ` Jonathan Cameron
2019-07-22 6:47 ` Ardelean, Alexandru
2019-07-28 15:43 ` Sasha Levin
2019-07-19 4:14 ` [PATCH AUTOSEL 4.4 18/35] iio: iio-utils: Fix possible incorrect mask calculation Sasha Levin
2019-07-21 17:27 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).