linux-kernel - Re: [PATCH 1/5] iio: inkern: introduce devm_iio_map_array

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:   Sun, 5 Sep 2021 12:08:32 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <aardelean@...iqon.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-doc@...r.kernel.org, hdegoede@...hat.com, wens@...e.org
Subject: Re: [PATCH 1/5] iio: inkern: introduce
 devm_iio_map_array_register() short-hand function

On Fri, 3 Sep 2021 16:56:43 +0300
Alexandru Ardelean <aardelean@...iqon.com> wrote:

> On Fri, 3 Sept 2021 at 16:40, Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> >
> > On Fri, Sep 03, 2021 at 10:29:13AM +0300, Alexandru Ardelean wrote:  
> > > This change introduces a device-managed variant to the
> > > iio_map_array_register() function. It's a simple implementation of calling
> > > iio_map_array_register() and registering a callback to
> > > iio_map_array_unregister() with the devm_add_action_or_reset().
> > >
> > > The function uses an explicit 'dev' parameter to bind the unwinding to. It
> > > could have been implemented to implicitly use the parent of the IIO device,
> > > however it shouldn't be too expensive to callers to just specify to which
> > > device object to bind this unwind call.
> > > It would make the API a bit more flexible.  
> >
> > AFAIU this dev pointer is kinda discussable thing. What scenario do you expect
> > (have in mind) when it shouldn't use parent?  
> 
> So, this brings me back to an older discussion about devm_ when I
> thought about making some devm_ function that implicitly takes uses
> the parent of the IIO device.
> 
> Jonathan mentioned that if we go that route, maybe we should prefix it
> with iiom_ .
> But we weren't sure at the time if that makes sense.
> The idea was to bind the management of the unwinding to either the
> parent of the IIO device, or the IIO device itself (indio_dev->dev).

I'm not keen on playing the same games that say pcim does where it
effectively combines all cleanup into one devres call because that
can end up doing things in an order that isn't strict reversal if
the setup path isn't carefully organised.  It's 'clever' but to my mind
doing something similar in IIO would need to subtle bugs.

So we would simply be using iiom_ as a shortcut to say bind to the parent
device.  Might be worth considering long term but I'm not keen to do
it for a random little used function first!

Series looks good to me, but I'll let it sit a little longer before applying.

Jonathan


> 
> We kind of concluded that it may probably not be a good to hide
> anything and make standard a devm_ function with an explicit 'dev'
> object parameter.
> 
> I found a recent mention here (while searching for iiom_  on linux-iio):
> https://lore.kernel.org/linux-iio/20210313192150.74c0a91b@archlinux/
> 
> 
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >