Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2023-10-24 at 13:23 +0100, Jonathan Cameron wrote:
> On Mon, 23 Oct 2023 16:58:48 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Mon, 2023-10-23 at 15:34 +0100, Jonathan Cameron wrote:
> > > On Mon, 23 Oct 2023 13:51:04 +0200
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >   
> > > > On Mon, 2023-10-23 at 10:53 +0100, Jonathan Cameron wrote:  
> > > > > On Mon, 23 Oct 2023 10:55:56 +0200
> > > > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > >     
> > > > > > On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:    
> > > > > > > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron
> > > > > > > <jic23@kernel.org>
> > > > > > > wrote:      
> > > > > > > > 
> > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > 
> > > > > > > > Allows use of:
> > > > > > > > 
> > > > > > > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > > >         if (IS_ERR(claimed_dev))
> > > > > > > >                 return PTR_ERR(claimed_dev);
> > > > > > > > 
> > > > > > > >         st = iio_priv(claimed_dev);
> > > > > > > > 
> > > > > > > > to automatically call iio_device_release_direct_mode() based on
> > > > > > > > scope.
> > > > > > > > Typically seen in combination with local device specific locks
> > > > > > > > which
> > > > > > > > are already have automated cleanup options via guard(mutex)(&st-
> > > > > > > >   
> > > > > > > > > lock)  
> > > > > > > > and scoped_guard().  Using both together allows most error
> > > > > > > > handling
> > > > > > > > to
> > > > > > > > be automated.
> > > > > > > > 
> > > > > > > > Note that whilst this pattern results in a struct iio_dev
> > > > > > > > *claimed_dev
> > > > > > > > that can be used, it is not necessary to do so as long as that
> > > > > > > > pointer
> > > > > > > > has been checked for errors as in the example.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > ---
> > > > > > > >  drivers/iio/industrialio-core.c |  4 ++++
> > > > > > > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > > > > > > >  2 files changed, 29 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > > > b/drivers/iio/industrialio-
> > > > > > > > core.c
> > > > > > > > index c77745b594bd..93bfad105eb5 100644
> > > > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > > > @@ -2065,6 +2065,10 @@
> > > > > > > > EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > > > > > > >   */
> > > > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > > > > > > >  {
> > > > > > > > +       /* Auto cleanup can result in this being called with an
> > > > > > > > ERR_PTR
> > > > > > > > */
> > > > > > > > +       if (IS_ERR(indio_dev))
> > > > > > > > +               return;
> > > > > > > > +
> > > > > > > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > > > index d0ce3b71106a..11c42170fda1 100644
> > > > > > > > --- a/include/linux/iio/iio.h
> > > > > > > > +++ b/include/linux/iio/iio.h
> > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > 
> > > > > > > >  #include <linux/device.h>
> > > > > > > >  #include <linux/cdev.h>
> > > > > > > > +#include <linux/cleanup.h>
> > > > > > > >  #include <linux/slab.h>
> > > > > > > >  #include <linux/iio/types.h>
> > > > > > > >  /* IIO TODO LIST */
> > > > > > > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct
> > > > > > > > device
> > > > > > > > *dev,
> > > > > > > > struct iio_dev *indio_dev,
> > > > > > > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64
> > > > > > > > timestamp);
> > > > > > > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > > > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > > > > > > +/*
> > > > > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > > > > + *
> > > > > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > > > + *     if (IS_ERR(claimed_dev))
> > > > > > > > + *             return PTR_ERR(claimed_dev);
> > > > > > > > + *
> > > > > > > > + *     st = iio_priv(claimed_dev);
> > > > > > > > + *     ....
> > > > > > > > + */
> > > > > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > > > > +            iio_device_release_direct_mode(_T),
> > > > > > > > +            ({
> > > > > > > > +                       struct iio_dev *dev;
> > > > > > > > +                       int d =
> > > > > > > > iio_device_claim_direct_mode(_T);
> > > > > > > > +
> > > > > > > > +                       if (d < 0)
> > > > > > > > +                               dev = ERR_PTR(d);
> > > > > > > > +                       else
> > > > > > > > +                               dev = _T;
> > > > > > > > +                       dev;
> > > > > > > > +            }),
> > > > > > > > +            struct iio_dev *_T);
> > > > > > > > +
> > > > > > > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > > > > > > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > > > > > > 
> > > > > > > > --
> > > > > > > > 2.42.0
> > > > > > > >       
> > > > > > > 
> > > > > > > What is the benefit of exposing `claimed_dev` rather than just the
> > > > > > > int
> > > > > > > return value? It seems like it just makes more noise in the error
> > > > > > > check.
> > > > > > >       
> > > > > > 
> > > > > > I don't really have a very strong opinion on this but what I really
> > > > > > don't
> > > > > > like
> > > > > > much is the pattern:
> > > > > > 
> > > > > > CLASS(type, ret), where the return value is an argument of the
> > > > > > macro...
> > > > > > It
> > > > > > would
> > > > > > be nice if we could just make it like:
> > > > > > 
> > > > > > ret = guard(type)(...); //or any other variation of the guard()
> > > > > > macro
> > > > > > if (ret) 
> > > > > > 	return ret;
> > > > > > 
> > > > > > the above could also be an error pointer or even have one variation
> > > > > > of
> > > > > > each.
> > > > > > but
> > > > > > yeah, that likely means changing the cleanup.h file and that might
> > > > > > be
> > > > > > out of
> > > > > > scope for Jonathan's patch series. 
> > > > > >     
> > > > > 
> > > > > I fully agree it's ugly and a little unintuitive but I don't see a way
> > > > > an
> > > > > "lvalue"
> > > > > can work work cleanly (due to magic types under the hood) and I
> > > > > suspect we
> > > > > will
> > > > > have to get used to this pattern.
> > > > >     
> > > > 
> > > > Yeah, given the games being played with the constructor and the _lock
> > > > definition
> > > > so we return the variable we want to "release" I agree it would be hard
> > > > to
> > > > have
> > > > anything clean and likely even harder to read (more than it is already
> > > > :)).
> > > > 
> > > > However, I think users of the cleanup.h stuff could build on top of
> > > > it...
> > > > For
> > > > instance, in our case we could have something like:
> > > > 
> > > > #define IIO_CLAIM_DIRECT(dev) 
> > > > 	int __ret = 0;
> > > > 	CLASS(iio_claim_direct, claimed_dev)(dev);
> > > > 	if ((IS_ERR(claimed_dev))
> > > > 		__ret = PTR_ERR(claimed_dev);
> > > > 	__ret  
> > > 
> > > Maybe, but we'll have to deal with people perpetually trying to brackets
> > > around
> > > the complex macro... 
> > > 
> > >   
> > 
> > Not sure what you mean here... you mean dealing with people coming up with
> > funny
> > new macros around CLASS(). In IIO, this is very specific and If I'm not
> > missing
> > anything the obvious, the above macro with give the same usage as 
> > iio_device_claim_direct_mode() but without caring about release() - so not
> > sure
> > people could be that creative :)
> Checkpatch will warn something along the lines of complex macros should be
> contained
> in brackets / or do while()
> 
> So the class would go out of scope and be freed at the end of the macro :)
> 

Dohh! Tbh, I was not  being "smart" by not putting the brackets in my example
macro. I was just making it simple. For the real thing I had the brackets in my
mind and completely forgot about the scope nature of the cleanup attr.

Anyways, I very much like all of this stuff and I'm starting to use it in all
the places I can...

- Nuno Sá