Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 12 Apr 2020 14:30:08 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Alexandru Ardelean <alexandru.ardelean@analog.com>
Cc:     <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH] iio: buffer: remove null-checks for 'indio_dev->info'
Message-ID: <20200412143008.3fa722a9@archlinux>
In-Reply-To: <20200407145918.6833-1-alexandru.ardelean@analog.com>
References: <20200407145918.6833-1-alexandru.ardelean@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 7 Apr 2020 17:59:18 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Checking for 'indio_dev->info' is an impossible condition, since an IIO
> device should NOT be able to register without that information.
> The iio_device_register() function won't allow an IIO device to register if
> 'indio_dev->info' is NULL.
> 
> If that information somehow becomes NULL, then we're likely busted anyway
> and we should crash the system, if we haven't already.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
I'm glad there was a comment in there to remind me of what was going on here.

This is the result of an ancient set from (I think) Lars hardening IIO
against forced removal.

The indio_dev->info == NULL is deliberately set to true by the IIO core
during device remove in order to deal with any inflight data.

Reference counting should ensure the device doesn't go away but we need
to avoid actually doing anything if this occurs.  That pointer was a
convenient option to avoid having to add an explicit flag or 'going away'.

Now, it's possible we don't need this any more due to other changes but
I certainly don't want to remove it without that being very thoroughly
verified!

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e6fa1a4e135d..c96071bfada8 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
>  	size_t avail;
>  	int flushed = 0;
>  
> -	/* wakeup if the device was unregistered */
This comment makes it clear this isn't as 'obvious' as it might at first seem ;)

> -	if (!indio_dev->info)
> -		return true;
> -
>  	/* drain the buffer if it was disabled */
>  	if (!iio_buffer_is_active(buf)) {
>  		to_wait = min_t(size_t, to_wait, 1);
> @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
>  	size_t to_wait;
>  	int ret = 0;
>  
> -	if (!indio_dev->info)
> -		return -ENODEV;
> -
>  	if (!rb || !rb->access->read)
>  		return -EINVAL;
>  
> @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
>  
>  	add_wait_queue(&rb->pollq, &wait);
>  	do {
> -		if (!indio_dev->info) {
> -			ret = -ENODEV;
> -			break;
> -		}
> -
>  		if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) {
>  			if (signal_pending(current)) {
>  				ret = -ERESTARTSYS;
> @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp,
>  	struct iio_dev *indio_dev = filp->private_data;
>  	struct iio_buffer *rb = indio_dev->buffer;
>  
> -	if (!indio_dev->info || rb == NULL)
> +	if (rb == NULL)
>  		return 0;
>  
>  	poll_wait(filp, &rb->pollq, wait);
> @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>  		goto out_unlock;
>  	}
>  
> -	if (indio_dev->info == NULL) {
> -		ret = -ENODEV;
> -		goto out_unlock;
> -	}
> -
>  	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
>  
>  out_unlock: