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@xxxxxxxxx> 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@xxxxxxxxxx>
> > > wrote:
> > > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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
Then we could use it in the same way as before... Or at the very least I would
simply make it a bit more readable for IIO (rather than the plain CLASS() call):
#define IIO_CLAIM_DIRECT(claimed_dev, dev)
CLASS(iio_claim_direct, claimed_dev)(dev)
Just some thoughts...
- Nuno Sá
