linux-kernel - Re: [RFC PATCH 01/12] iio: core: register chardev only if needed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Nov 2020 14:10:59 +0200
From:   Alexandru Ardelean <ardeleanalex@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Alexandru Ardelean <alexandru.ardelean@...log.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [RFC PATCH 01/12] iio: core: register chardev only if needed

On Sat, Nov 21, 2020 at 8:05 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:29 +0200
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
>
> > We only need a chardev if we need to support buffers and/or events.
> >
> > With this change, a chardev will be created only if an IIO buffer is
> > attached OR an event_interface is configured.
> >
> > Otherwise, no chardev will be created, and the IIO device will get
> > registered with the 'device_add()' call.
> >
> > Quite a lot of IIO devices don't really need a chardev, so this is a minor
> > improvement to the IIO core, as the IIO device will take up (slightly)
> > fewer resources.
> >
> > In order to not create a chardev, we mostly just need to not initialize the
> > indio_dev->dev.devt field. If that is un-initialized, cdev_device_add()
> > behaves like device_add().
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> I'll be honest. I have no idea why I didn't do this in first place!
>
> I 'think' we are safe dropping this but I suppose it's possible some
> odd code checks for the chrdev presence?

So, libiio at least doesn't rely on this being there for any odd things.
But yeah, who knows what else is out there that might.
I was also thinking of sending this separately to have this earlier
out there in case it bothers other people.
I guess I got a little mixed by other patches and re-ordered things a
few times and this remained in this series.

>
> Hopefully not though.
>
> Jonathan
>
> > ---
> >  drivers/iio/industrialio-core.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 419d6f8acc13..ca8b11541477 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = {
> >       .compat_ioctl = compat_ptr_ioctl,
> >  };
> >
> > +static const struct file_operations iio_event_fileops = {
> > +     .owner = THIS_MODULE,
> > +     .llseek = noop_llseek,
> > +     .unlocked_ioctl = iio_ioctl,
> > +     .compat_ioctl = compat_ptr_ioctl,
> > +     .open = iio_chrdev_open,
> > +     .release = iio_chrdev_release,
> > +};
> > +
> >  static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> >  {
> >       int i, j;
> > @@ -1790,6 +1799,7 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops;
> >
> >  int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >  {
> > +     struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >       int ret;
> >
> >       if (!indio_dev->info)
> > @@ -1807,9 +1817,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >       if (ret < 0)
> >               return ret;
> >
> > -     /* configure elements for the chrdev */
> > -     indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
> > -
> >       iio_device_register_debugfs(indio_dev);
> >
> >       ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> > @@ -1838,9 +1845,15 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >               indio_dev->setup_ops == NULL)
> >               indio_dev->setup_ops = &noop_ring_setup_ops;
> >
> > -     cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > +     if (indio_dev->buffer)
> > +             cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > +     else if (iio_dev_opaque->event_interface)
> > +             cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> >
> > -     indio_dev->chrdev.owner = this_mod;
> > +     if (indio_dev->buffer || iio_dev_opaque->event_interface) {
> > +             indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
> > +             indio_dev->chrdev.owner = this_mod;
> > +     }
> >
> >       ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> >       if (ret < 0)
>