linux-kernel - Re: [PATCH v2] 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] [day] [month] [year] [list]

Message-ID: <74cc4f24377a420953496281f9ac34648420ee36.camel@analog.com>
Date:   Thu, 16 Apr 2020 07:04:30 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2] iio: core: register chardev only if needed

On Wed, 2020-04-15 at 16:55 +0200, Lars-Peter Clausen wrote:
> [External]
> 
> On 4/15/20 3:56 PM, Ardelean, Alexandru wrote:
> > On Tue, 2020-04-14 at 19:06 +0100, Jonathan Cameron wrote:
> > > [External]
> > > 
> > > On Tue, 14 Apr 2020 11:36:56 +0300
> > > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> > > 
> > > > The final intent is to localize all buffer ops into the
> > > > industrialio-buffer.c file, to be able to add support for multiple
> > > > buffers
> > > > per IIO device.
> > > > 
> > > > We only need a chardev if we need to support buffers and/or events.
> > > > 
> > > > With this change, a chardev will be created:
> > > > 1. if there is an IIO buffer attached OR
> > > > 2. if there is an event_interface configured
> > > > 
> > > > Otherwise, no chardev will be created.
> > > > 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 fewer
> > > > resources.
> > > > 
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > > > ---
> > > > 
> > > > Changelog v1 -> v2:
> > > > * split away from series 'iio: core,buffer: re-organize chardev
> > > > creation';
> > > >    i'm getting the feeling that this has some value on it's own;
> > > >    no idea if it needs 'Fixes' tag; it is a bit fuzzy to point to a
> > > > patch
> > > >    which this would be fixed by this; i'm guessing it would be fine
> > > >    without one
> > > I'd argue it's an 'optimization' rather than a fix :)
> > > 
> > > Still looks good to me but I'd like it to sit for a little while to
> > > see if anyone points out something we are both missing!
> > > 
> > This is not good.
> > It seems that I did not properly test all cases.
> > I had to break a device to not have an event_interface to notice that the
> > sysfs
> > doesn't get instantiated either because device_add is missing.
> > 
> > Will do another try.
> 
> I think you also have to make the `indio_dev->dev.devt = ...` 
> conditional. Or conditionally use device_add() instead of device_add_cdev().
> 
> If you go for the former you need to call cdev_device_del() 
> unconditionally, for the latter call device_del() or cdev_device_del() 
> depending on whether the cdev was registered.

I was thinking of conditionally using cdev_device_add/del() somehow.
But, this complicates the rest of the series a bit.
And I am thinking of how to simplify it.

Anyway, this will go back to the series  ¯\_(ツ)_/¯

> 
> - Lars
> 
>