Message-ID: <20251210180447.0a37c3dc@jic23-huawei>
Date: Wed, 10 Dec 2025 18:04:47 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: David Lechner <dlechner@...libre.com>, Andy Shevchenko
 <andy.shevchenko@...il.com>, Kurt Borja <kuurtb@...il.com>, Andy Shevchenko
 <andriy.shevchenko@...el.com>, Lars-Peter Clausen	 <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>, Benson Leung
 <bleung@...omium.org>, Antoniu Miclaus <antoniu.miclaus@...log.com>,
 Gwendal Grignou	 <gwendal@...omium.org>, Shrikant Raskar
 <raskar.shree97@...il.com>, Per-Daniel Olsson <perdaniel.olsson@...s.com>,
 Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko	
 <andy@...nel.org>, Guenter Roeck <groeck@...omium.org>, Jonathan Cameron	
 <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev
Subject: Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode
 locks

On Wed, 10 Dec 2025 09:17:17 +0000
Nuno Sá <noname.nuno@...il.com> wrote:

> On Tue, 2025-12-09 at 11:05 -0600, David Lechner wrote:
> > On 12/9/25 4:34 AM, Nuno Sá wrote:  
> > > On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:  
> > > > On Thu, 4 Dec 2025 17:07:28 +0200
> > > > Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> > > >   
> > > > > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@...il.com> wrote:  
> > > > > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:    
> > > > > > > 
> > > > > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > > > > add cleanup.h support for the lock API:
> > > > > > > 
> > > > > > >       iio_device_claim_{direct,buffer_mode}().    
> > > > > > 
> > > > > > We already went this patch and then reverted it. I guess before we did not had
> > > > > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > > > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > > > > > 
> > > > > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > > > > annotations much make sense if we go down this path. Unless we want to use both
> > > > > > approaches which is also questionable.    
> > > > > 
> > > > > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > > > > sent this RFC. Jonathan, what's your thoughts?  
> > > > 
> > > > I was pretty heavily involved in discussions around ACQUIRE() and it's use
> > > > in CXL and runtime PM (though that's still evolving with Rafael trying
> > > > to improve the syntax a little).  As you might guess I did have this use
> > > > in mind during those discussions.
> > > > 
> > > > As far as I know by avoiding the for loop complexity of the previous
> > > > try we made and looking (under the hood) like guard() it should be much
> > > > easier and safer to use.  Looking at this was on my list, so I'm very happy
> > > > to see this series from Kurt exploring how it would be done.
> > > > 
> > > > Sparse wise there is no support for now for any of the cleanup.h magic
> > > > other than ignoring it.  That doesn't bother me that much though as these
> > > > macros create more or less hidden local variables that are hard to mess
> > > > with in incorrect ways.
> > > > 
> > > > So in general I'm very much in favour of this for same reasons I jumped
> > > > in last time (which turned out to be premature!)
> > > > 
> > > > This will be particularly useful in avoiding the need for helper functions
> > > > in otherwise simple code flows.
> > > >   
> > > 
> > > Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> > > macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> > > I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> > > some significant work in order to make mlock private (given historical abuse of it) and this
> > > is basically making it public again. So I would like to either think a bit harder to see if we
> > > can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> > > really not pretty).
> > > 
> > > At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> > > used by drivers to protect their own internal data structures and state.
> > > 
> > > - Nuno Sá  
> > 
> > Due to the way that conditional guards only extend regular guards, I don't
> > think there is a way to not expose the basic mlock wrapper. So "don't use this
> > unless you really know what you are doing" docs seem like the best option.  
> 
> Right! I figured my first option would be very unlikely... But for the comment I hope we can
> elaborate a bit more. Like "don't use this lock to protect your own driver state/data ... you might
> need this together iio_buffer_enabled() and if for some reason you cannot use the claim helpers).
I think this problem is possible to work around.

We use wrapper macros rather than ever using the ACQUIRE macro directly
(and obscure function names to make it obvious they aren't normal :)

The underlying lock guards might be there but they'll smell so bad that anyone
using them should have at least checked why and hence seen the 'do not use this'
documentation.

Jonathan

> 
> - Nuno Sá
> 
> 

Powered by blists - more mailing lists