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

[Jonathan Cameron <jic23@kernel.org>]

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?

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

>