On Sun, 2020-04-12 at 14:30 +0100, Jonathan Cameron wrote:
> [External]
>
> On Tue, 7 Apr 2020 17:59:18 +0300
> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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!
>
Agreed.
Thanks for the info.
Will think about this a bit later.
Thanks
Alex
> 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:
