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

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>,
	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: Wed, 06 Dec 2023 13:05:53 +0100	[thread overview]
Message-ID: <bba767835e775909c6b8a3334cceeb419afef4ca.camel@gmail.com> (raw)
In-Reply-To: <20231204153855.71c9926f@jic23-huawei>

On Mon, 2023-12-04 at 15:38 +0000, Jonathan Cameron wrote:
> 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 
> 

Arguably, in here, removing a backend is the user doing something seriously wrong so
I see the splat with good eyes :D.

That said, I'm fine in turning this into a pr_warn_once()...

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

Yes, interesting talk indeed. I have been following this issue for some time now.
That's why I tried to be careful in the backend stuff (so we don't explode if a
backend is gone) even though is a much more simpler approach. But the talk mentions
three solutions and we kind of have both option C (the pointer stuff) and option A
(consumer removed on provicer unbind)
in here. option A is being given through device links with the AUTO_REMOVE_CONSUMER
flag.

And the talk actually left me thinking on that (as it's discussed in there. In our
simpler case (ADI ones), it does make sense to remove the consumer if the provider is
not there. But if we think in more advanced usecases (or maybe already in the STM
usecase) where we have a backend per data path. Does it make sense to completely
"kill" the consumer if we remove one of the data paths? Starting to think it
doesn't...

- Nuno Sá