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

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

[Alexandru Ardelean]

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>
---
 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)
-- 
2.17.1

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

[Jonathan Cameron]

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)