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

[Jonathan Cameron <jic23@kernel.org>]

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)