| Message ID | 20231022154710.402590-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | IIO: Use the new cleanup.h magic | expand |
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. Also, this seems like this is a pattern that could be generalized and put in cleanup.h. For example, this pattern could be used with mutex_trylock as well. Basically we could create a variation of the current `guard` like: #define DEFINE_CHECKED_GUARD(_name, _type, _lock, _unlock) ... #define checked_guard(_name) ... To be used like: /* linux/mutex.h */ #define DEFINE_CHECKED_GUARD(mutex, struct mutex *, \ mutex_trylock(_T), mutex_unlock(_T)) /* any/driver.c */ if (!checked_guard(mutex)(&thing->lock)) return -EBUSY /* linux/iio/iio.h */ #define DEFINE_CHECKED_GUARD(iio_claim_direct, struct iio_dev *indio_dev *, \ iio_device_claim_direct_mode(_T), iio_device_release_direct_mode(_T)) /* iio/driver.c */ if (!checked_guard(iio_claim_direct)(indio_dev)) return -EBUSY
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. - Nuno Sá
On Sun, 22 Oct 2023 16:10:48 -0500 David Lechner <dlechner@baylibre.com> 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. > > Also, this seems like this is a pattern that could be generalized and > put in cleanup.h. For example, this pattern could be used with > mutex_trylock as well. > > Basically we could create a variation of the current `guard` like: > > #define DEFINE_CHECKED_GUARD(_name, _type, _lock, _unlock) ... > #define checked_guard(_name) ... > > To be used like: > > /* linux/mutex.h */ > #define DEFINE_CHECKED_GUARD(mutex, struct mutex *, \ > mutex_trylock(_T), mutex_unlock(_T)) My head hurt whilst digging through what the macros did, but I don't think this can work this simply because we need to instantiate a local variable that is then used for the cleanup. When you call guard(mutex)(&lock); Expanding to CLASS(mutex, __UNIQUE_ID(guard))(&lock) to class_mutex_t var __cleanup(class_mutex_destuctor) = class_mutex_constructor(&lock); where DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T)) to DEFINE_CLASS(mutex, struct mutex *, mutex_unlock, ({mutex_lock(_T), T;}), struct mutex *_T; to typedef struct mutex * class_mutex_t; ... Key being that it relies on the lock path returning the variable that you use as the input for the cleanup. If we return an int, we can't do that. Unless I'm missing some magic that would make it work! > > /* any/driver.c */ > if (!checked_guard(mutex)(&thing->lock)) > return -EBUSY > > /* linux/iio/iio.h */ > #define DEFINE_CHECKED_GUARD(iio_claim_direct, struct iio_dev *indio_dev *, \ > iio_device_claim_direct_mode(_T), iio_device_release_direct_mode(_T)) > > /* iio/driver.c */ > if (!checked_guard(iio_claim_direct)(indio_dev)) > return -EBUSY >
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. There are lots of other examples in kernel that are similar DECLARE_BITMAP() etc and we've kind of gotten used to those... Jonathan > - Nuno Sá > >
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 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á
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... > > 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... Maybe. I'm not sure it's worth it though. This class stuff is odd and I don't really want to hid it from people too much. Sometimes better just to make people deal with the ugly on basis they hopefully go figure out what it is doing. Jonathan > > - Nuno Sá > >
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 :). Anyways, as I started to say in my first reply, I don't feel strong about this at all, so feel free to add: Reviewed-by: Nuno Sa <nuno.sa@analog.com>
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 :) > > Anyways, as I started to say in my first reply, I don't feel strong about this > at all, so feel free to add: > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> Thanks, I'm not going to rush with this set anyway given merge window about to open However I do have a few long flights coming up so might use it in a lot more drivers for v2. Jonathan > > >
On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote: > > > +/* > > > + * 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); > > > + > 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. So I have a version for trylocks that I've not managed to send out.. it doesn't deal with arbitrary error codes, and as someone else down-thread noted, the guard() thing is not really suited for tricks like this. Notably I have a patch that converts ptrace_attach() to have a loop like: scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) { goto success; } return -ERESTARTNOINTR; success: ... return 0; And another patch that does something like: scoped_cond_guard (rwsem_read_intr, no_lock, task ? &task->signal->exec_update_lock : NULL) { if (0) { no_lock: if (task) return -EINTR; } ... } --- Subject: cleanup: Add conditional guard support From: Peter Zijlstra <peterz@infradead.org> Date: Sun Sep 17 13:22:17 CEST 2023 Adds: - DEFINE_GUARD_COND() / DEFINE_LOCK_GUARD_1_COND() to extend existing guards with conditional lock primitives, eg. mutex_trylock(), mutex_lock_interruptible(). nb. both primitives allow NULL 'locks', which cause the lock to fail (obviously). - extends scoped_guard() to not take the body when the the conditional guard 'fails'. eg. scoped_guard (mutex_intr, &task->signal_cred_guard_mutex) { ... } will only execute the body when the mutex is held. - provides scoped_cond_guard(name, label, args...); which extends scoped_guard() to jump to @label when the lock fails. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- Index: linux-2.6/include/linux/cleanup.h =================================================================== --- linux-2.6.orig/include/linux/cleanup.h +++ linux-2.6/include/linux/cleanup.h @@ -136,14 +136,35 @@ static inline class_##_name##_t class_## */ #define DEFINE_GUARD(_name, _type, _lock, _unlock) \ - DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T) + DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \ + static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \ + { return *_T; } + +#define DEFINE_GUARD_COND(_name, _ext, _condlock) \ + EXTEND_CLASS(_name, _ext, \ + ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \ + class_##_name##_t _T) \ + static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \ + { return class_##_name##_lock_ptr(_T); } + +#define _guard(_name, var) \ + class_##_name##_t var __cleanup(class_##_name##_destructor) = \ + class_##_name##_constructor #define guard(_name) \ - CLASS(_name, __UNIQUE_ID(guard)) + _guard(_name, __UNIQUE_ID(guard)) + +#define __guard_ptr(_name) class_##_name##_lock_ptr #define scoped_guard(_name, args...) \ for (CLASS(_name, scope)(args), \ - *done = NULL; !done; done = (void *)1) + *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1) + +#define scoped_cond_guard(_name, _label, args...) \ + for (CLASS(_name, scope)(args), \ + *done = NULL; !done; done = (void *)1) \ + if (!__guard_ptr(_name)(&scope)) goto _label; \ + else /* * Additional helper macros for generating lock guards with types, either for @@ -173,6 +194,11 @@ typedef struct { \ static inline void class_##_name##_destructor(class_##_name##_t *_T) \ { \ if (_T->lock) { _unlock; } \ +} \ + \ +static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \ +{ \ + return _T->lock; \ } @@ -201,4 +227,14 @@ __DEFINE_LOCK_GUARD_1(_name, _type, _loc __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__) \ __DEFINE_LOCK_GUARD_0(_name, _lock) +#define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock) \ + EXTEND_CLASS(_name, _ext, \ + ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\ + if (_T->lock && !(_condlock)) _T->lock = NULL; \ + _t; }), \ + typeof_member(class_##_name##_t, lock) l) \ + static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \ + { return class_##_name##_lock_ptr(_T); } + + #endif /* __LINUX_GUARDS_H */ Index: linux-2.6/include/linux/mutex.h =================================================================== --- linux-2.6.orig/include/linux/mutex.h +++ linux-2.6/include/linux/mutex.h @@ -221,6 +221,7 @@ extern void mutex_unlock(struct mutex *l extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock); DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T)) -DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T)) +DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T)) +DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0) #endif /* __LINUX_MUTEX_H */ Index: linux-2.6/include/linux/spinlock.h =================================================================== --- linux-2.6.orig/include/linux/spinlock.h +++ linux-2.6/include/linux/spinlock.h @@ -507,6 +507,8 @@ DEFINE_LOCK_GUARD_1(raw_spinlock, raw_sp raw_spin_lock(_T->lock), raw_spin_unlock(_T->lock)) +DEFINE_LOCK_GUARD_1_COND(raw_spinlock, _try, raw_spin_trylock(_T->lock)) + DEFINE_LOCK_GUARD_1(raw_spinlock_nested, raw_spinlock_t, raw_spin_lock_nested(_T->lock, SINGLE_DEPTH_NESTING), raw_spin_unlock(_T->lock)) @@ -515,23 +517,36 @@ DEFINE_LOCK_GUARD_1(raw_spinlock_irq, ra raw_spin_lock_irq(_T->lock), raw_spin_unlock_irq(_T->lock)) +DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq(_T->lock)) + DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t, raw_spin_lock_irqsave(_T->lock, _T->flags), raw_spin_unlock_irqrestore(_T->lock, _T->flags), unsigned long flags) +DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irqsave, _try, + raw_spin_trylock_irqsave(_T->lock, _T->flags)) + DEFINE_LOCK_GUARD_1(spinlock, spinlock_t, spin_lock(_T->lock), spin_unlock(_T->lock)) +DEFINE_LOCK_GUARD_1_COND(spinlock, _try, spin_trylock(_T->lock)) + DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t, spin_lock_irq(_T->lock), spin_unlock_irq(_T->lock)) +DEFINE_LOCK_GUARD_1_COND(spinlock_irq, _try, + spin_trylock_irq(_T->lock)) + DEFINE_LOCK_GUARD_1(spinlock_irqsave, spinlock_t, spin_lock_irqsave(_T->lock, _T->flags), spin_unlock_irqrestore(_T->lock, _T->flags), unsigned long flags) +DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try, + spin_trylock_irqsave(_T->lock, _T->flags)) + #undef __LINUX_INSIDE_SPINLOCK_H #endif /* __LINUX_SPINLOCK_H */ Index: linux-2.6/include/linux/rwsem.h =================================================================== --- linux-2.6.orig/include/linux/rwsem.h +++ linux-2.6/include/linux/rwsem.h @@ -203,11 +203,11 @@ extern void up_read(struct rw_semaphore extern void up_write(struct rw_semaphore *sem); DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T)) -DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T)) - -DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T)) -DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T)) +DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T)) +DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0) +DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T)) +DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T)) /* * downgrade write lock to read lock
On Tue, Oct 24, 2023 at 05:11:23PM +0200, Peter Zijlstra wrote: > On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote: > > > > > +/* > > > > + * 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); > > > > + > > > 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. > > So I have a version for trylocks that I've not managed to send out.. it > doesn't deal with arbitrary error codes, and as someone else down-thread > noted, the guard() thing is not really suited for tricks like this. > > Notably I have a patch that converts ptrace_attach() to have a loop like: > > scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) { > > goto success; > } > return -ERESTARTNOINTR; > > success: > ... > return 0; > > > And another patch that does something like: > > scoped_cond_guard (rwsem_read_intr, no_lock, > task ? &task->signal->exec_update_lock : NULL) { > > if (0) { > no_lock: > if (task) > return -EINTR; > } > > ... > } > Hmm, looking at: + case IIO_CHAN_INFO_RAW: { /* magic value - channel value read */ + CLASS(iio_claim_direct, claimed_dev)(indio_dev); + if (IS_ERR(claimed_dev)) + return PTR_ERR(claimed_dev); + guard(mutex)(&st->lock); + switch (chan->type) { case IIO_VOLTAGE: if (chan->output) { /* Set integer part to cached value */ *val = st->dac_val; + return IIO_VAL_INT; } else if (chan->differential) { if (chan->channel == 1) *val = st->differential_adc_val[0]; else *val = st->differential_adc_val[1]; + return IIO_VAL_INT; } else { *val = st->single_ended_adc_val; + return IIO_VAL_INT; } + case IIO_ACCEL: *val = st->accel_val; + return IIO_VAL_INT; default: + return -EINVAL; } + } And your iio_device_claim_direct_mode(), that is basically a trylock, either it succeeds (and returns 0) or fails with -EBUSY. Which means you could write your thing above like: 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 = NULL; else dev = _T; dev; }), struct iio_dev *_T); static inline void * class_iio_claim_direct_lock_ptr(class_iio_claim_direct_t *_T) { return *_T; } case IIO_CHAN_INFO_RAW: /* magic value - channel value read */ scoped_guard (iio_device_claim, indio_dev) { guard(mutex)(&st->lock); switch (chan->type) { case ..: return IIO_VAL_INT; default: return -EINVAL; } } return -EBUSY; and it would all just work, no?
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á
On Tue, 24 Oct 2023 17:28:00 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Oct 24, 2023 at 05:11:23PM +0200, Peter Zijlstra wrote: > > On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote: > > > > > > > +/* > > > > > + * 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); > > > > > + > > > > > 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. > > > > So I have a version for trylocks that I've not managed to send out.. it > > doesn't deal with arbitrary error codes, and as someone else down-thread > > noted, the guard() thing is not really suited for tricks like this. > > > > Notably I have a patch that converts ptrace_attach() to have a loop like: > > > > scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) { > > > > goto success; > > } > > return -ERESTARTNOINTR; > > > > success: > > ... > > return 0; > > > > > > And another patch that does something like: > > > > scoped_cond_guard (rwsem_read_intr, no_lock, > > task ? &task->signal->exec_update_lock : NULL) { > > > > if (0) { > > no_lock: > > if (task) > > return -EINTR; > > } > > > > ... > > } > > > > Hmm, looking at: > > + case IIO_CHAN_INFO_RAW: { /* magic value - channel value read */ > + CLASS(iio_claim_direct, claimed_dev)(indio_dev); > + if (IS_ERR(claimed_dev)) > + return PTR_ERR(claimed_dev); > + guard(mutex)(&st->lock); > + > switch (chan->type) { > case IIO_VOLTAGE: > if (chan->output) { > /* Set integer part to cached value */ > *val = st->dac_val; > + return IIO_VAL_INT; > } else if (chan->differential) { > if (chan->channel == 1) > *val = st->differential_adc_val[0]; > else > *val = st->differential_adc_val[1]; > + return IIO_VAL_INT; > } else { > *val = st->single_ended_adc_val; > + return IIO_VAL_INT; > } > + > case IIO_ACCEL: > *val = st->accel_val; > + return IIO_VAL_INT; > default: > + return -EINVAL; > } > + } > > > And your iio_device_claim_direct_mode(), that is basically a trylock, > either it succeeds (and returns 0) or fails with -EBUSY. > > Which means you could write your thing above like: > > 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 = NULL; > else > dev = _T; > dev; > }), > struct iio_dev *_T); > > static inline void * > class_iio_claim_direct_lock_ptr(class_iio_claim_direct_t *_T) > { return *_T; } > > > > case IIO_CHAN_INFO_RAW: /* magic value - channel value read */ > scoped_guard (iio_device_claim, indio_dev) { > guard(mutex)(&st->lock); > switch (chan->type) { > case ..: > return IIO_VAL_INT; > default: > return -EINVAL; > } > } > return -EBUSY; > > and it would all just work, no? With the scoped_guard change you mention in previous email, this should work. I'm not that keen on it from a readability point of view as it puts the 'good' path inline and if we do have anything to do after the scoped_guard() we need to engage in setting a condition variable to indicate we'd been in the loop. Maybe we can extend your other case though for cases where an early exit fits or make it more general to incorporate this? scoped_cond_guard_ret(mutex_intr, &mutex, -EINTR)? or (naming needs work scoped_cond_guard_call(mutex_intr, &mutex, ({ return -EINTR; })); #define scoped_cond_guard_call(_name, _call, args...) \ + for (CLASS(_name, scope)(args), \ + *done = NULL; !done; done = (void *)1) \ + if (!__guard_ptr(_name)(&scope)) _call; \ + else Totally untested but hopefully conveys the idea (which is a tiny extension of your scoped_cond_guard) Jonathan
On Sat, Oct 28, 2023 at 05:59:28PM +0100, Jonathan Cameron wrote: > #define scoped_cond_guard_call(_name, _call, args...) \ > + for (CLASS(_name, scope)(args), \ > + *done = NULL; !done; done = (void *)1) \ > + if (!__guard_ptr(_name)(&scope)) _call; \ > + else > > Totally untested but hopefully conveys the idea (which is a tiny > extension of your scoped_cond_guard) It's a statement not a call, but yeah, I've done something like this, even made one of my cases simpler too, Let me got post this stuff so we can get it merged.
On Thu, 2 Nov 2023 11:48:48 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Sat, Oct 28, 2023 at 05:59:28PM +0100, Jonathan Cameron wrote: > > > #define scoped_cond_guard_call(_name, _call, args...) \ > > + for (CLASS(_name, scope)(args), \ > > + *done = NULL; !done; done = (void *)1) \ > > + if (!__guard_ptr(_name)(&scope)) _call; \ > > + else > > > > Totally untested but hopefully conveys the idea (which is a tiny > > extension of your scoped_cond_guard) > > It's a statement not a call, but yeah, I've done something like this, > even made one of my cases simpler too, Good point. > > Let me got post this stuff so we can get it merged. > Great. I really like this whole effort, so thanks for working on it and your feedback here. Jonathan
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);