* [PATCH AUTOSEL 4.4 03/60] iio: light: isl29125: fix iio_triggered_buffer_{predisable,postenable} positions [not found] <20200618013004.610532-1-sashal@kernel.org> @ 2020-06-18 1:29 ` Sasha Levin 2020-06-19 16:31 ` Jonathan Cameron 2020-06-18 1:29 ` [PATCH AUTOSEL 4.4 23/60] iio: buffer: Don't allow buffers without any channels enabled to be activated Sasha Levin 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2020-06-18 1:29 UTC (permalink / raw) To: linux-kernel, stable Cc: Alexandru Ardelean, Jonathan Cameron, Sasha Levin, linux-iio From: Alexandru Ardelean <alexandru.ardelean@analog.com> [ Upstream commit 9b7a12c3e090cf3fba6f66f1f23abbc6e0e86021 ] 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. This change reworks the predisable/postenable hooks so that the pollfunc is attached/detached in the correct position. It also balances the calls a bit, by grouping the preenable and the iio_triggered_buffer_postenable() into a single isl29125_buffer_postenable() function. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/iio/light/isl29125.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/iio/light/isl29125.c b/drivers/iio/light/isl29125.c index e2945a20e5f6..60388a41ec8c 100644 --- a/drivers/iio/light/isl29125.c +++ b/drivers/iio/light/isl29125.c @@ -215,13 +215,24 @@ static const struct iio_info isl29125_info = { .driver_module = THIS_MODULE, }; -static int isl29125_buffer_preenable(struct iio_dev *indio_dev) +static int isl29125_buffer_postenable(struct iio_dev *indio_dev) { struct isl29125_data *data = iio_priv(indio_dev); + int err; + + err = iio_triggered_buffer_postenable(indio_dev); + if (err) + return err; data->conf1 |= ISL29125_MODE_RGB; - return i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, + err = i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, data->conf1); + if (err) { + iio_triggered_buffer_predisable(indio_dev); + return err; + } + + return 0; } static int isl29125_buffer_predisable(struct iio_dev *indio_dev) @@ -229,19 +240,18 @@ static int isl29125_buffer_predisable(struct iio_dev *indio_dev) struct isl29125_data *data = iio_priv(indio_dev); int ret; - ret = iio_triggered_buffer_predisable(indio_dev); - if (ret < 0) - return ret; - data->conf1 &= ~ISL29125_MODE_MASK; data->conf1 |= ISL29125_MODE_PD; - return i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, + ret = i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, data->conf1); + + iio_triggered_buffer_predisable(indio_dev); + + return ret; } static const struct iio_buffer_setup_ops isl29125_buffer_setup_ops = { - .preenable = isl29125_buffer_preenable, - .postenable = &iio_triggered_buffer_postenable, + .postenable = isl29125_buffer_postenable, .predisable = isl29125_buffer_predisable, }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 03/60] iio: light: isl29125: fix iio_triggered_buffer_{predisable,postenable} positions 2020-06-18 1:29 ` [PATCH AUTOSEL 4.4 03/60] iio: light: isl29125: fix iio_triggered_buffer_{predisable,postenable} positions Sasha Levin @ 2020-06-19 16:31 ` Jonathan Cameron 2020-06-22 0:07 ` Sasha Levin 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2020-06-19 16:31 UTC (permalink / raw) To: Sasha Levin; +Cc: linux-kernel, stable, Alexandru Ardelean, linux-iio On Wed, 17 Jun 2020 21:29:07 -0400 Sasha Levin <sashal@kernel.org> wrote: > From: Alexandru Ardelean <alexandru.ardelean@analog.com> > > [ Upstream commit 9b7a12c3e090cf3fba6f66f1f23abbc6e0e86021 ] > > 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. > > This change reworks the predisable/postenable hooks so that the pollfunc is > attached/detached in the correct position. > It also balances the calls a bit, by grouping the preenable and the > iio_triggered_buffer_postenable() into a single > isl29125_buffer_postenable() function. > This is really part of some rework. It doesn't 'fix' a bug as such (I think), but rather a bit of logical inconsistency. Shouldn't do any harm though beyond adding noise to stable. I added notes to some of these to mark them as not stable material, but clearly missed this one. Sorry about that. Jonathan > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > drivers/iio/light/isl29125.c | 28 +++++++++++++++++++--------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/light/isl29125.c b/drivers/iio/light/isl29125.c > index e2945a20e5f6..60388a41ec8c 100644 > --- a/drivers/iio/light/isl29125.c > +++ b/drivers/iio/light/isl29125.c > @@ -215,13 +215,24 @@ static const struct iio_info isl29125_info = { > .driver_module = THIS_MODULE, > }; > > -static int isl29125_buffer_preenable(struct iio_dev *indio_dev) > +static int isl29125_buffer_postenable(struct iio_dev *indio_dev) > { > struct isl29125_data *data = iio_priv(indio_dev); > + int err; > + > + err = iio_triggered_buffer_postenable(indio_dev); > + if (err) > + return err; > > data->conf1 |= ISL29125_MODE_RGB; > - return i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, > + err = i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, > data->conf1); > + if (err) { > + iio_triggered_buffer_predisable(indio_dev); > + return err; > + } > + > + return 0; > } > > static int isl29125_buffer_predisable(struct iio_dev *indio_dev) > @@ -229,19 +240,18 @@ static int isl29125_buffer_predisable(struct iio_dev *indio_dev) > struct isl29125_data *data = iio_priv(indio_dev); > int ret; > > - ret = iio_triggered_buffer_predisable(indio_dev); > - if (ret < 0) > - return ret; > - > data->conf1 &= ~ISL29125_MODE_MASK; > data->conf1 |= ISL29125_MODE_PD; > - return i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, > + ret = i2c_smbus_write_byte_data(data->client, ISL29125_CONF1, > data->conf1); > + > + iio_triggered_buffer_predisable(indio_dev); > + > + return ret; > } > > static const struct iio_buffer_setup_ops isl29125_buffer_setup_ops = { > - .preenable = isl29125_buffer_preenable, > - .postenable = &iio_triggered_buffer_postenable, > + .postenable = isl29125_buffer_postenable, > .predisable = isl29125_buffer_predisable, > }; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 03/60] iio: light: isl29125: fix iio_triggered_buffer_{predisable,postenable} positions 2020-06-19 16:31 ` Jonathan Cameron @ 2020-06-22 0:07 ` Sasha Levin 0 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2020-06-22 0:07 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-kernel, stable, Alexandru Ardelean, linux-iio On Fri, Jun 19, 2020 at 05:31:01PM +0100, Jonathan Cameron wrote: >On Wed, 17 Jun 2020 21:29:07 -0400 >Sasha Levin <sashal@kernel.org> wrote: > >> From: Alexandru Ardelean <alexandru.ardelean@analog.com> >> >> [ Upstream commit 9b7a12c3e090cf3fba6f66f1f23abbc6e0e86021 ] >> >> 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. >> >> This change reworks the predisable/postenable hooks so that the pollfunc is >> attached/detached in the correct position. >> It also balances the calls a bit, by grouping the preenable and the >> iio_triggered_buffer_postenable() into a single >> isl29125_buffer_postenable() function. >> > >This is really part of some rework. It doesn't 'fix' a bug >as such (I think), but rather a bit of logical inconsistency. > >Shouldn't do any harm though beyond adding noise to stable. >I added notes to some of these to mark them as not stable material, >but clearly missed this one. Sorry about that. I'll drop it, thanks! -- Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 4.4 23/60] iio: buffer: Don't allow buffers without any channels enabled to be activated [not found] <20200618013004.610532-1-sashal@kernel.org> 2020-06-18 1:29 ` [PATCH AUTOSEL 4.4 03/60] iio: light: isl29125: fix iio_triggered_buffer_{predisable,postenable} positions Sasha Levin @ 2020-06-18 1:29 ` Sasha Levin 2020-06-19 16:27 ` Jonathan Cameron 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2020-06-18 1:29 UTC (permalink / raw) To: linux-kernel, stable Cc: Lars-Peter Clausen, Alexandru Ardelean, Jonathan Cameron, Sasha Levin, linux-iio From: Lars-Peter Clausen <lars@metafoo.de> [ Upstream commit b7329249ea5b08b2a1c2c3f24a2f4c495c4f14b8 ] Before activating a buffer make sure that at least one channel is enabled. Activating a buffer with 0 channels enabled doesn't make too much sense and disallowing this case makes sure that individual driver don't have to add special case code to handle it. Currently, without this patch enabling a buffer is possible and no error is produced. With this patch -EINVAL is returned. An example of execution with this patch and some instrumented print-code: root@analog:~# cd /sys/bus/iio/devices/iio\:device3/buffer root@analog:/sys/bus/iio/devices/iio:device3/buffer# echo 1 > enable 0: iio_verify_update 748 indio_dev->masklength 2 *insert_buffer->scan_mask 00000000 1: iio_verify_update 753 2:__iio_update_buffers 1115 ret -22 3: iio_buffer_store_enable 1241 ret -22 -bash: echo: write error: Invalid argument 1, 2 & 3 are exit-error paths. 0 the first print in iio_verify_update() rergardless of error path. Without this patch (and same instrumented print-code): root@analog:~# cd /sys/bus/iio/devices/iio\:device3/buffer root@analog:/sys/bus/iio/devices/iio:device3/buffer# echo 1 > enable 0: iio_verify_update 748 indio_dev->masklength 2 *insert_buffer->scan_mask 00000000 root@analog:/sys/bus/iio/devices/iio:device3/buffer# Buffer is enabled with no error. Note from Jonathan: Probably not suitable for automatic application to stable. This has been there from the very start. It tidies up an odd corner case but won't effect any 'real' users. Fixes: 84b36ce5f79c0 ("staging:iio: Add support for multiple buffers") Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/iio/industrialio-buffer.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 864a61b05665..fea41b328ab9 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -641,6 +641,13 @@ static int iio_verify_update(struct iio_dev *indio_dev, bool scan_timestamp; unsigned int modes; + if (insert_buffer && + bitmap_empty(insert_buffer->scan_mask, indio_dev->masklength)) { + dev_dbg(&indio_dev->dev, + "At least one scan element must be enabled first\n"); + return -EINVAL; + } + memset(config, 0, sizeof(*config)); /* -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 23/60] iio: buffer: Don't allow buffers without any channels enabled to be activated 2020-06-18 1:29 ` [PATCH AUTOSEL 4.4 23/60] iio: buffer: Don't allow buffers without any channels enabled to be activated Sasha Levin @ 2020-06-19 16:27 ` Jonathan Cameron 2020-06-22 0:07 ` Sasha Levin 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2020-06-19 16:27 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Lars-Peter Clausen, Alexandru Ardelean, linux-iio On Wed, 17 Jun 2020 21:29:27 -0400 Sasha Levin <sashal@kernel.org> wrote: > From: Lars-Peter Clausen <lars@metafoo.de> > > [ Upstream commit b7329249ea5b08b2a1c2c3f24a2f4c495c4f14b8 ] > > Before activating a buffer make sure that at least one channel is enabled. > Activating a buffer with 0 channels enabled doesn't make too much sense and > disallowing this case makes sure that individual driver don't have to add > special case code to handle it. > > Currently, without this patch enabling a buffer is possible and no error is > produced. With this patch -EINVAL is returned. > > An example of execution with this patch and some instrumented print-code: > root@analog:~# cd /sys/bus/iio/devices/iio\:device3/buffer > root@analog:/sys/bus/iio/devices/iio:device3/buffer# echo 1 > enable > 0: iio_verify_update 748 indio_dev->masklength 2 *insert_buffer->scan_mask 00000000 > 1: iio_verify_update 753 > 2:__iio_update_buffers 1115 ret -22 > 3: iio_buffer_store_enable 1241 ret -22 > -bash: echo: write error: Invalid argument > 1, 2 & 3 are exit-error paths. 0 the first print in iio_verify_update() > rergardless of error path. > > Without this patch (and same instrumented print-code): > root@analog:~# cd /sys/bus/iio/devices/iio\:device3/buffer > root@analog:/sys/bus/iio/devices/iio:device3/buffer# echo 1 > enable > 0: iio_verify_update 748 indio_dev->masklength 2 *insert_buffer->scan_mask 00000000 > root@analog:/sys/bus/iio/devices/iio:device3/buffer# > Buffer is enabled with no error. > > Note from Jonathan: Probably not suitable for automatic application to stable. > This has been there from the very start. It tidies up an odd corner > case but won't effect any 'real' users. > As noted. I don't think it matters if we do apply this to stable. It closes an interface oddity rather than an actual known bug. > Fixes: 84b36ce5f79c0 ("staging:iio: Add support for multiple buffers") > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > drivers/iio/industrialio-buffer.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index 864a61b05665..fea41b328ab9 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -641,6 +641,13 @@ static int iio_verify_update(struct iio_dev *indio_dev, > bool scan_timestamp; > unsigned int modes; > > + if (insert_buffer && > + bitmap_empty(insert_buffer->scan_mask, indio_dev->masklength)) { > + dev_dbg(&indio_dev->dev, > + "At least one scan element must be enabled first\n"); > + return -EINVAL; > + } > + > memset(config, 0, sizeof(*config)); > > /* ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 4.4 23/60] iio: buffer: Don't allow buffers without any channels enabled to be activated 2020-06-19 16:27 ` Jonathan Cameron @ 2020-06-22 0:07 ` Sasha Levin 0 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2020-06-22 0:07 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-kernel, stable, Lars-Peter Clausen, Alexandru Ardelean, linux-iio On Fri, Jun 19, 2020 at 05:27:33PM +0100, Jonathan Cameron wrote: >On Wed, 17 Jun 2020 21:29:27 -0400 >Sasha Levin <sashal@kernel.org> wrote: > >> From: Lars-Peter Clausen <lars@metafoo.de> >> >> [ Upstream commit b7329249ea5b08b2a1c2c3f24a2f4c495c4f14b8 ] >> >> Before activating a buffer make sure that at least one channel is enabled. >> Activating a buffer with 0 channels enabled doesn't make too much sense and >> disallowing this case makes sure that individual driver don't have to add >> special case code to handle it. >> >> Currently, without this patch enabling a buffer is possible and no error is >> produced. With this patch -EINVAL is returned. >> >> An example of execution with this patch and some instrumented print-code: >> root@analog:~# cd /sys/bus/iio/devices/iio\:device3/buffer >> root@analog:/sys/bus/iio/devices/iio:device3/buffer# echo 1 > enable >> 0: iio_verify_update 748 indio_dev->masklength 2 *insert_buffer->scan_mask 00000000 >> 1: iio_verify_update 753 >> 2:__iio_update_buffers 1115 ret -22 >> 3: iio_buffer_store_enable 1241 ret -22 >> -bash: echo: write error: Invalid argument >> 1, 2 & 3 are exit-error paths. 0 the first print in iio_verify_update() >> rergardless of error path. >> >> Without this patch (and same instrumented print-code): >> root@analog:~# cd /sys/bus/iio/devices/iio\:device3/buffer >> root@analog:/sys/bus/iio/devices/iio:device3/buffer# echo 1 > enable >> 0: iio_verify_update 748 indio_dev->masklength 2 *insert_buffer->scan_mask 00000000 >> root@analog:/sys/bus/iio/devices/iio:device3/buffer# >> Buffer is enabled with no error. >> >> Note from Jonathan: Probably not suitable for automatic application to stable. >> This has been there from the very start. It tidies up an odd corner >> case but won't effect any 'real' users. >> > >As noted. I don't think it matters if we do apply this to stable. >It closes an interface oddity rather than an actual known bug. I'll drop it, thanks! -- Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-22 0:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200618013004.610532-1-sashal@kernel.org>
2020-06-18 1:29 ` [PATCH AUTOSEL 4.4 03/60] iio: light: isl29125: fix iio_triggered_buffer_{predisable,postenable} positions Sasha Levin
2020-06-19 16:31 ` Jonathan Cameron
2020-06-22 0:07 ` Sasha Levin
2020-06-18 1:29 ` [PATCH AUTOSEL 4.4 23/60] iio: buffer: Don't allow buffers without any channels enabled to be activated Sasha Levin
2020-06-19 16:27 ` Jonathan Cameron
2020-06-22 0:07 ` Sasha Levin
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).