Re: [PATCH] iio: buffer: fix attach/detach pollfunc order

Jonathan Cameron
2020-07-15T12:11:26Z

On Wed, 15 Jul 2020 07:16:29 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The original patch was error-ed by the submitter (me) and not by the author
> (Lars).
> After looking through the discussion logs (on email), it seems that this
> order was wrong for the start, even though the order implemented in the
> drivers was correct.
>
> Discussions:
> - first RFC: https://lore.kernel.org/linux-iio/20180622135322.3459-1-alexandru.ardelean@analog.com/
> - 2nd patch: https://lore.kernel.org/linux-iio/20181219140912.22582-1-alexandru.ardelean@analog.com/
> - final patch-sets:
> https://lore.kernel.org/linux-iio/20200522104632.517470-1-alexandru.ardelean@analog.com/
> https://lore.kernel.org/linux-iio/20200525113855.178821-1-alexandru.ardelean@analog.com/
>
> The last one was applied.
>
> The idea is that pollfunc should be attached before calling the
> 'indio_dev->setup_ops->postenable' hook and should be detached after
> calling the 'indio_dev->setup_ops->predisable' hook.
>
> While the drivers were updated to take this into account, the change to the
> IIO core was somehow omitted and was made wrong.
>
> This change fixes the order to the proper form.
>
> Fixes f11d59d87b862: ("iio: Move attach/detach of the poll func to the core")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Great thanks.

Applied to the togreg branch of iio.git.

Thanks,

Jonathan

> ---
> drivers/iio/industrialio-buffer.c | 31 ++++++++++++++++++-------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 2aec8b85f40d..a7d7e5143ed2 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -971,24 +971,29 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> goto err_disable_buffers;
> }
>
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> + ret = iio_trigger_attach_poll_func(indio_dev->trig,
> + indio_dev->pollfunc);
> + if (ret)
> + goto err_disable_buffers;
> + }
> +
> if (indio_dev->setup_ops->postenable) {
> ret = indio_dev->setup_ops->postenable(indio_dev);
> if (ret) {
> dev_dbg(&indio_dev->dev,
> "Buffer not started: postenable failed (%d)\n", ret);
> - goto err_disable_buffers;
> + goto err_detach_pollfunc;
> }
> }
>
> - if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> - ret = iio_trigger_attach_poll_func(indio_dev->trig,
> - indio_dev->pollfunc);
> - if (ret)
> - goto err_disable_buffers;
> - }
> -
> return 0;
>
> +err_detach_pollfunc:
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> + iio_trigger_detach_poll_func(indio_dev->trig,
> + indio_dev->pollfunc);
> + }
> err_disable_buffers:
> list_for_each_entry_continue_reverse(buffer, &iio_dev_opaque->buffer_list,
> buffer_list)
> @@ -1014,11 +1019,6 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
> if (list_empty(&iio_dev_opaque->buffer_list))
> return 0;
>
> - if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> - iio_trigger_detach_poll_func(indio_dev->trig,
> - indio_dev->pollfunc);
> - }
> -
> /*
> * If things go wrong at some step in disable we still need to continue
> * to perform the other steps, otherwise we leave the device in a
> @@ -1032,6 +1032,11 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
> ret = ret2;
> }
>
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> + iio_trigger_detach_poll_func(indio_dev->trig,
> + indio_dev->pollfunc);
> + }
> +
> list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
> ret2 = iio_buffer_disable(buffer, indio_dev);
> if (ret2 && !ret)