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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 06 Dec 2023 13:05:53 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

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

There is a reasonably argument that partial tear down isn't a common case. So
may not be worth worrying about.

J
> 
> - Nuno Sá
>