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

[Stephan Costa Barros <stephanbrrs8@gmail.com>]

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.

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

The driver starts the read from the hard IRQ, and I'd rather not change 
that in a fix. Did you mean claiming it somewhere else? If not, I'll 
keep the flag, as small as I can make it, and explain it in the commit 
message. (The automated review found three ordering bugs in this code 
in v2; they are fixed for v3.)

> If we get a timeout, we'd not normally worry too much on old data
> on the next read. It's an error case that isn't supposed to happen so
> if we are talking stale data that is often considered fine. The
> second part may make sense - break that out as a separate patch
> so we can easily analyse it on its own.

Agreed. I'll drop the stale data part, and the unbind wait becomes its
own patch.

> > - The return value of spi_async() and the status of the RDATA message
> >   are ignored: a failure leaves the transfer counter stuck, and a failed
> >   transfer is reported as a sample. Handle both.
>
> This is fine but not burred in this giant fix patch.
>
> > - read_raw takes the sample out of rx_buffer after releasing direct
> >   mode, so a buffer enabled in between can overwrite it. Take it while
> >   direct mode is still held.
>
> This one seems valid.

Both become their own patches.

Thanks,
Stephan