[PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[nattan]

From: Nattan Ferreira <nattanferreira58@gmail.com>

Use iio_device_claim_direct() to protect register access via debugfs
from conflicting with buffered capture modes. This prevents data
corruption and ensures correct device operation when users access
registers while streaming data.

Signed-off-by: Nattan Ferreira <nattanferreira58@gmail.com>
Co-developed-by: Lucas Antonio <lucasantonio.santos@usp.br>
Signed-off-by: Lucas Antonio <lucasantonio.santos@usp.br>
---
 drivers/iio/adc/ti-ads1119.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
index f120e7e21..273fb8e35 100644
--- a/drivers/iio/adc/ti-ads1119.c
+++ b/drivers/iio/adc/ti-ads1119.c
@@ -405,6 +405,9 @@ static int ads1119_debugfs_reg_access(struct iio_dev *indio_dev,
 	struct ads1119_state *st = iio_priv(indio_dev);
 	int ret;
 
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
 	if (reg > ADS1119_REG_STATUS)
 		return -EINVAL;
 
@@ -421,6 +424,8 @@ static int ads1119_debugfs_reg_access(struct iio_dev *indio_dev,
 	if (reg > ADS1119_REG_CONFIG)
 		return -EINVAL;
 
+	iio_device_release_direct(indio_dev);
+
 	return i2c_smbus_write_byte_data(st->client, ADS1119_CMD_WREG,
 					 writeval);
 }
-- 
2.34.1

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[João Paulo Gonçalves]

Hi,

> Use iio_device_claim_direct() to protect register access via debugfs
> from conflicting with buffered capture modes. This prevents data
> corruption and ensures correct device operation when users access
> registers while streaming data.
> 

I understood the problem, but I think keeping at least the possibility to
read registers even when buffered mode is enabled is still useful for
development (e.g. you can check that userspace configurations are correct
by checking the hw registers). Of course, there's a chance of losing some
samples, but debugfs is meant to be used during development/integration,
where this probably is not an issue.

So, for me, a better solution would be to avoid writing to the device
when buffered mode is enable:

diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
index 9be7676f31bb..a22d7cb5c4a0 100644
--- a/drivers/iio/adc/ti-ads1119.c
+++ b/drivers/iio/adc/ti-ads1119.c
@@ -416,6 +416,9 @@ static int ads1119_debugfs_reg_access(struct iio_dev *indio_dev,
                return 0;
        }
 
+       if(iio_buffer_enabled(indio_dev))
+               return -EBUSY;
+
        if (reg > ADS1119_REG_CONFIG)
                return -EINVAL;

Best Regards,
João Paulo

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[Francesco Dolcini]

On Sun, Apr 20, 2025 at 01:07:28PM -0300, João Paulo Gonçalves wrote:
> > Use iio_device_claim_direct() to protect register access via debugfs
> > from conflicting with buffered capture modes. This prevents data
> > corruption and ensures correct device operation when users access
> > registers while streaming data.
> > 
> 
> but debugfs is meant to be used during development/integration,
> where this probably is not an issue.

Is even worth doing any such a change? I assume Jonathan will have an
opinion on what's the expectation for an IIO driver.

Nattan, can you explain why you need such a change? What is the use
case?

Francesco

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[Jonathan Cameron]

On Sun, 20 Apr 2025 23:17:06 +0200
Francesco Dolcini <francesco@dolcini.it> wrote:

> On Sun, Apr 20, 2025 at 01:07:28PM -0300, João Paulo Gonçalves wrote:
> > > Use iio_device_claim_direct() to protect register access via debugfs
> > > from conflicting with buffered capture modes. This prevents data
> > > corruption and ensures correct device operation when users access
> > > registers while streaming data.
> > >   
> > 
> > but debugfs is meant to be used during development/integration,
> > where this probably is not an issue.  
> 
> Is even worth doing any such a change? I assume Jonathan will have an
> opinion on what's the expectation for an IIO driver.
> 
> Nattan, can you explain why you need such a change? What is the use
> case?
> 
> Francesco

For debug access it 'depends'....

Some cases are easy reasons to add this protection
1) Accesses can't happen at all because to talk to the device and read
   registers etc requires some mode change (e.g. recent drivers that
   can only access config registers when the bus is operating in particular
   modes, or where we have to use a slower SPI bus rate).
2) There are register banks involved. So a single write can leave the
   driver talking to the wrong registers...
3) Driver uses multipart reads / writes (similar to register banks)
   In most cases this is a device specific thing and should use a local
   lock to serialize accesses.  On occasion that is too complex to make
   work with debug so we restrict debugfs access in general.

Other cases are less obvious.
1) Bus traffic in general might slow down a transfer and break things
   because of timing. I.e. missed samples.  That can happen for all sorts
   of reasons anyway so should only be a momentary problem.
2) They might changes settings.

These less obvious things are a case of thinking it's a debug
access. We tend to not prevent actual deliberate state changes as that's
someone shooting them selves in the foot and they get what they deserve.

So what is the case we are protecting against here?  The description
definitely needs more information to justify this patch.

Jonathan

> 

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[Marcelo Schmitt]

...
> For debug access it 'depends'....
> 
> Some cases are easy reasons to add this protection
> 1) Accesses can't happen at all because to talk to the device and read
>    registers etc requires some mode change (e.g. recent drivers that
>    can only access config registers when the bus is operating in particular
>    modes, or where we have to use a slower SPI bus rate).
> 2) There are register banks involved. So a single write can leave the
>    driver talking to the wrong registers...
> 3) Driver uses multipart reads / writes (similar to register banks)
>    In most cases this is a device specific thing and should use a local
>    lock to serialize accesses.  On occasion that is too complex to make
>    work with debug so we restrict debugfs access in general.
> 
> Other cases are less obvious.
> 1) Bus traffic in general might slow down a transfer and break things
>    because of timing. I.e. missed samples.  That can happen for all sorts
>    of reasons anyway so should only be a momentary problem.
> 2) They might changes settings.

Does the case of configuration/setting change apply to IIO device properties?
Let's say for example a device starts running a buffered capture with some
certain input gain configuration and goes filling IIO buffer with the data
from the device. At some point while the buffered capture is running, the
user uses debugfs to change the gain configuration. The _scale attribute
will not correctly convert all buffer data to mV units. Would cases like that
be something to prevent? Or we consider those to be fine since a debug feature
was used?

Thanks,
Marcelo

> 
> These less obvious things are a case of thinking it's a debug
> access. We tend to not prevent actual deliberate state changes as that's
> someone shooting them selves in the foot and they get what they deserve.
> 
> So what is the case we are protecting against here?  The description
> definitely needs more information to justify this patch.
> 
> Jonathan
> 
> > 
> 
> 

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[Nattan Ferreira]

On Sun, Apr 20, 2025 at 6:17 PM Francesco Dolcini <francesco@dolcini.it> wrote:
>
> On Sun, Apr 20, 2025 at 01:07:28PM -0300, João Paulo Gonçalves wrote:
> > > Use iio_device_claim_direct() to protect register access via debugfs
> > > from conflicting with buffered capture modes. This prevents data
> > > corruption and ensures correct device operation when users access
> > > registers while streaming data.
> > >
> >
> > but debugfs is meant to be used during development/integration,
> > where this probably is not an issue.
>
> Is even worth doing any such a change? I assume Jonathan will have an
> opinion on what's the expectation for an IIO driver.
>
> Nattan, can you explain why you need such a change? What is the use
> case?
>
> Francesco
>

Hi Francesco and João,

Thanks for the feedback and your thoughts on this.

To clarify the use case: The primary reason for preventing both read
and write access when the buffer is active is to avoid potential data
corruption and inconsistencies. When the buffer is streaming data,
concurrent register access (either reading or writing) can lead to
unexpected behaviors, such as incorrect register values or misaligned
data. This can cause confusion and make it harder to debug or test the
driver, as well as introduce risks in production if these issues
persist unnoticed.

I understand that development and integration are critical stages, and
while it's tempting to allow register access for diagnostic purposes,
my concern is that even during this phase, uncoordinated access to
registers could result in misleading data, which could cascade into
errors or incorrect assumptions.

With that said, I do see the merit in João’s suggestion to allow read
access while blocking writes. If that aligns better with the IIO
driver expectations, I’m happy to update the patch accordingly.

Let me know your thoughts or if you think there’s a better approach here.

Best regards,
Nattan Ferreira

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[Jonathan Cameron]

On Mon, 21 Apr 2025 11:42:36 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> ...
> > For debug access it 'depends'....
> > 
> > Some cases are easy reasons to add this protection
> > 1) Accesses can't happen at all because to talk to the device and read
> >    registers etc requires some mode change (e.g. recent drivers that
> >    can only access config registers when the bus is operating in particular
> >    modes, or where we have to use a slower SPI bus rate).
> > 2) There are register banks involved. So a single write can leave the
> >    driver talking to the wrong registers...
> > 3) Driver uses multipart reads / writes (similar to register banks)
> >    In most cases this is a device specific thing and should use a local
> >    lock to serialize accesses.  On occasion that is too complex to make
> >    work with debug so we restrict debugfs access in general.
> > 
> > Other cases are less obvious.
> > 1) Bus traffic in general might slow down a transfer and break things
> >    because of timing. I.e. missed samples.  That can happen for all sorts
> >    of reasons anyway so should only be a momentary problem.
> > 2) They might changes settings.  
> 
> Does the case of configuration/setting change apply to IIO device properties?
> Let's say for example a device starts running a buffered capture with some
> certain input gain configuration and goes filling IIO buffer with the data
> from the device. At some point while the buffered capture is running, the
> user uses debugfs to change the gain configuration. The _scale attribute
> will not correctly convert all buffer data to mV units. Would cases like that
> be something to prevent? Or we consider those to be fine since a debug feature
> was used?
Fine.  This isn't really any different to them changing the scale when using
non buffered capture.

The debug interfaces in general are optional and I've always wondered if we should
have gated them more strongly.  Maybe a CONFIG_IIO_DRIVER_DEBUG or similar
to hide them all on production systems and remove the footguns.

Jonathan
 
> 
> Thanks,
> Marcelo
> 
> > 
> > These less obvious things are a case of thinking it's a debug
> > access. We tend to not prevent actual deliberate state changes as that's
> > someone shooting them selves in the foot and they get what they deserve.
> > 
> > So what is the case we are protecting against here?  The description
> > definitely needs more information to justify this patch.
> > 
> > Jonathan
> >   
> > >   
> > 
> >   

Re: [PATCH] iio: adc: ti-ads1119: Prevent concurrent access during

[Jonathan Cameron]

On Tue, 22 Apr 2025 15:04:25 -0300
Nattan Ferreira <nattanferreira58@gmail.com> wrote:

> On Sun, Apr 20, 2025 at 6:17 PM Francesco Dolcini <francesco@dolcini.it> wrote:
> >
> > On Sun, Apr 20, 2025 at 01:07:28PM -0300, João Paulo Gonçalves wrote:  
> > > > Use iio_device_claim_direct() to protect register access via debugfs
> > > > from conflicting with buffered capture modes. This prevents data
> > > > corruption and ensures correct device operation when users access
> > > > registers while streaming data.
> > > >  
> > >
> > > but debugfs is meant to be used during development/integration,
> > > where this probably is not an issue.  
> >
> > Is even worth doing any such a change? I assume Jonathan will have an
> > opinion on what's the expectation for an IIO driver.
> >
> > Nattan, can you explain why you need such a change? What is the use
> > case?
> >
> > Francesco
> >  
> 
> Hi Francesco and João,
> 
> Thanks for the feedback and your thoughts on this.
> 
> To clarify the use case: The primary reason for preventing both read
> and write access when the buffer is active is to avoid potential data
> corruption and inconsistencies. When the buffer is streaming data,
> concurrent register access (either reading or writing) can lead to
> unexpected behaviors, such as incorrect register values or misaligned
> data. This can cause confusion and make it harder to debug or test the
> driver, as well as introduce risks in production if these issues
> persist unnoticed.
> 
> I understand that development and integration are critical stages, and
> while it's tempting to allow register access for diagnostic purposes,
> my concern is that even during this phase, uncoordinated access to
> registers could result in misleading data, which could cascade into
> errors or incorrect assumptions.
> 
> With that said, I do see the merit in João’s suggestion to allow read
> access while blocking writes. If that aligns better with the IIO
> driver expectations, I’m happy to update the patch accordingly.
> 
> Let me know your thoughts or if you think there’s a better approach here.

My personal view is that these debug interfaces provide a terrible way
to shoot yourself in the foot whenever we enable them.   As such I'm not
really convinced that we need protections.   Limiting write access is
probably fine but it also an ABI change. Technically someone might be using
that interface in their userspace code and see breakage at which point we
get into  the silly situation of having to revert this hardening...

I'd be interested in a proposal to lock these down in general via a config
parameter or new drivers restricting things as you have done here.

Jonathan

> 
> Best regards,
> Nattan Ferreira