Re: [PATCH v2 1/6] iio: adc: ti-ads1298: fix the data read path

[Jonathan Cameron]

On Tue, 27 Oct 2026 23:19:45 -0300
Stephan Costa Barros <stephanbrrs8@xxxxxxxxx> wrote:

> On Sun, 25 Oct 2026 23:06:01 +0000, Jonathan Cameron wrote:
> > This is a lot in one patch.  Can we break some of this
> > up into smaller changes?  The whole thing is probably
> > to complex for a stable tag, so focus on the most broken
> > aspects.  
> 
> Will do: one patch per fix in v3, the most broken first.
> 
> > This I'd like in a patch that does just this and allows the commit
> > message to say why simply using a single lock for the data buffers
> > would not be cleaner.  
> 
> OK. To check the argument before I write it: a driver mutex around
> write_cmd() and the regmap accessors would also stop a register access
> from rewriting a command. It does not help with rx_buffer, which an
> spi_async() started from the hard IRQ writes, so it cannot be under a
> sleeping lock and still needs its own cacheline. Separate buffers fix
> both without a new lock. If you would still rather have the mutex for
> the command path, I'll do that.

I'm fine with the approach you have here. I just want some more reasoning
in the description as cachelines can be quite large and so we need
to have a clear justification for people thinking of backporting the
change.

> > > After STOP, the conversion in progress still raises DRDY, so an RDATA
> > > transfer can be in flight when the core frees the active scan mask.
> > > Track in the driver whether samples go to the buffer, wait in
> > > predisable for the last DRDY and its transfer, and push the sample
> > > before marking the transfer done. update_scan_mode reset the transfer
> > > counter, which let a new transfer be queued over one in flight; wait
> > > for it instead.  

Too much cropping.  I had to go find what I replied to in order to
following the discussion! Put back above, indented as if you'd left
it there

> > Complicated sounding solution. Can we claim buffered mode instead?
> > That would ensure the update_scan_mode and this don't race and mostly
> > would be taking an uncontended lock.  

> 
> I don't see how to do it where the race is. The sample is pushed from
> ads1298_rdata_complete(), the completion of that spi_async(), which the
> SPI core "invokes in a context which can't sleep".

Ah.  I'd missed that complexity and was assuming this was a normal
threaded interrupt handler.  One simplification here would be to stop using
async spi and instead just use synchronous spi in an interrupt thread which
is how most drivers handle similar flows.

> iio_device_try_claim_buffer_mode() takes mlock, and the core holds mlock
> while it calls predisable, so the completion would also wait on the
> teardown it races with.

yeah. I got this wrong in another reply I sent today. Indeed, the lock
is not conditional even though it acts like a conditional lock in the
outer call.  We need to be able to queue up multiple users of that
state on the lock so it can't be conditional wrt to mlock.

Added checking the existing places we grab the buffer claim to my todo
list for likely bugs :(


Jonathan