Re: [PATCH v4 6/8] iio: imu: adis16607: Add FIFO-based buffered/triggered capture

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 1 Nov 2026 23:50:10 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 27 Oct 2026 09:36:47 +0000
> "Sabau, Radu bogdan" <radu.sabau@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Monday, October 26, 2026 2:09 AM    
> > 
> > ...
> >   
> > > > +static void adis16607_fifo_push_sample(struct iio_dev *indio_dev,
> > > > +				       u8 *fifo_data, s64 timestamp)
> > > > +{
> > > > +	struct adis16607 *st = iio_priv(indio_dev);
> > > > +	u16 msw, lsw;
> > > > +	s32 val24;
> > > > +	int bit, offset = 0;
> > > > +
> > > > +	memset(st->data, 0, sizeof(st->data));
> > > > +
> > > > +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev-
> > > >masklength) {    
> > > iio_for_each_active_channel()
> > > 
> > > As sashiko notes, masklength is private.
> > > 
> > > There are a load of places we need to update to stop accessing these
> > > directly but please don't add another one.
> > > 
> > > It's on my list to throw some LLM tokens ago once I get out from
> > > under review backlog.
> > > 
> > > https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20261023-
> > > adis16607-v4-0-
> > > a4d541ebfd13*40analog.com__;IyU!!A3Ni8CS0y2Y!9cguUuIwYMdnmBcoNR
> > > ye0_U99KUnJt04T6XlSnvS-9rxh5-8Y2fQXC39Mxsw3erc_JwvyFp3ssecZg$
> > > Check everything else reported.
> > > 
> > > FWIW there are also some nasty race conditions around active_scan_mask
> > > being available when the buffer is being torn down. You might
> > > want to close those off by claiming buffered mode to prevent
> > > such a transition.
> > >     
> > 
> > Agreed, I'll switch to iio_for_each_active_channel() and I'll go through the
> > rest of the Sashiko findings for v5.
> > 
> > On the active_scan_mask races, I'd like to check which path you mean.
> > In this driver the only reader of the mask is the trigger handler
> > (fifo_push_sample()), which runs as the pollfunc.
> > As far as I can tell, iio_trigger_detach_poll_func()'s free_irq() waits for it
> > to finish before iio_disable_buffers() frees the mask, so I couldn't find a
> > window where the handler sees a stale or NULL mask today.  
> 
> Ah. I may have failed to distinguish between cases with a trigger involved
> and those without.  The bug reports are probably on the ones without.
> Unfortunately I haven't found time to look at the overall issue
> in enough detail yet.
> 
> > Even so, I'd like to close this off for good, so that a later change to the
> > driver or to adislib can't introduce the race.
> > 
> > Claiming buffer mode inside the handler looks like it would deadlock
> > against iio_disable_buffers(), which holds mlock while waiting for the
> > handler in free_irq().  
> 
> We'd use a conditional claim in here - only the core code driving the state
> transitions should take a claim unconditionally (they can just grab the lock).
> If mlock is held it should fail and we just drop out of the handler having
> done nothing.  So either disable buffer wins and we drop through the handler
> quickly or it looses and the attempt to take mlock blocks in
> iio_disable_buffers()
> 
> Or am I missing something worse?

I'm indeed being dopey today.  Whilst it is a conditional claim we grab the lock
unconditionally as part of that.  Which makes sense because we want
multiple simple read the state calls to block behind each other.

> 
> > 
> > Would snapshotting the active-channel layout in update_scan_mode()
> > (called before the pollfunc is attached) and having the handler read
> > only that copy be an acceptable way to close it?  
> It would but that feels like we are just papering over a lifetime issue
> by creating another object rather than fixing it if necessary.
> 
> Anyhow, from your first comment this may all be irrelevant anyway.
> 
> Jonathan
> 
> >   
> 
>