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.
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
