Re: [PATCH 03/12] iio: add the IIO backend framework

From: Jonathan Cameron <jic23@kernel.org>
To: Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org>
Cc: <nuno.sa@analog.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-iio@vger.kernel.org,
	Olivier MOYSAN <olivier.moysan@foss.st.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Michael Hennerich <Michael.Hennerich@analog.com>
Subject: Re: [PATCH 03/12] iio: add the IIO backend framework
Date: Mon, 4 Dec 2023 15:38:55 +0000	[thread overview]
Message-ID: <20231204153855.71c9926f@jic23-huawei> (raw)
In-Reply-To: <20231121-dev-iio-backend-v1-3-6a3d542eba35@analog.com>

On Tue, 21 Nov 2023 11:20:16 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> This is a Framework to handle complex IIO aggregate devices.
> 
> The typical architecture is to have one device as the frontend device which
> can be "linked" against one or multiple backend devices. All the IIO and
> userspace interface is expected to be registers/managed by the frontend
> device which will callback into the backends when needed (to get/set
> some configuration that it does not directly control).

As this is first place backend / frontend terminology used (I think), make
sure to give an example so people understand what sorts of IP / devices thes
might be.

> 
> The basic framework interface is pretty simple:
>  - Backends should register themselves with @devm_iio_backend_register()
>  - Frontend devices should get backends with @devm_iio_backend_get()
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

Looks good to me in general.  I'll need to have a really close read though
before we merge this as there may be sticky corners! (hopefully not)

...

> +static LIST_HEAD(iio_back_list);
> +static DEFINE_MUTEX(iio_back_lock);
> +
> +/*
> + * Helper macros to properly call backend ops. The main point for these macros
> + * is to properly lock the backend mutex on every call plus checking if the
> + * backend device is still around (by looking at the *ops pointer).
If just checking if it is around rather thank looking for a bug, then
I'd suggest a lighter choice than WARN_ON_x 

Btw, there were some interesting discussions on lifetimes and consumer / provider
models at plumbers. I think https://www.youtube.com/watch?v=bHaMMnIH6AM will be
the video.   Suggested the approach of not refcounting but instead allowing for
a deliberate removal of access similar to your check on ops here (and the one
we do in core IIO for similar purposes).  Sounded interesting but I've not
explored what it would really mean to switch to that model yet.

> + */
> +#define iio_backend_op_call(back, op, args...) ({ \
> +	struct iio_backend *__back = back; \
> +	int __ret; \
> +			\
> +	guard(mutex)(&__back->lock); \
> +	if (WARN_ON_ONCE(!__back->ops)) \
> +		__ret = -ENODEV; \
> +	else if (!__back->ops->op) \
> +		__ret = -EOPNOTSUPP; \
> +	else \
> +		__ret = __back->ops->op(__back, ##args); \
> +	\
> +	__ret; \
> +})