Message-ID: <20250901204640.3cee61bc@jic23-huawei>
Date: Mon, 1 Sep 2025 20:46:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Mohammad Amin Hosseini <moahmmad.hosseinii@...il.com>,
 linux-iio@...r.kernel.org, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org, lars@...afoo.de,
 Michael.Hennerich@...log.com, nuno.sa@...log.com, andy@...nel.org,
 sonic.zhang@...log.com, vapier@...too.org, dan.carpenter@...aro.org
Subject: Re: [PATCH v4] staging: iio: adc: ad7816: fix race condition in SPI
 operations

On Mon, 1 Sep 2025 12:00:44 -0500
David Lechner <dlechner@...libre.com> wrote:

> On 9/1/25 11:03 AM, Mohammad Amin Hosseini wrote:
> > The ad7816 driver lacks proper synchronization around SPI operations
> > and device state access. Concurrent access from multiple threads can
> > lead to data corruption and inconsistent device state.
> > 
> > The driver performs sequences of GPIO pin manipulations followed by
> > SPI transactions without any locking. Device state variables (mode,
> > channel_id, oti_data) are also accessed without synchronization.
> > 
> > This bug was found through manual code review using static analysis
> > techniques. The review focused on identifying unsynchronized access
> > patterns to shared resources. Key indicators were:
> > - GPIO pin state changes followed by SPI operations without atomicity
> > - Shared state variables accessed from multiple sysfs entry points
> > - No mutex or spinlock protection around sections
> > - Potential for interleaved execution in multi-threaded environments
> > 
> > The review methodology involved tracing data flow paths and identifying
> > points where concurrent access could corrupt device state or SPI
> > communication sequences.
> > 
> > Add io_lock mutex to protect:
> > - SPI transactions and GPIO sequences in read/write functions
> > - Device state variables in sysfs show/store functions
> > - Concurrent access to chip configuration
> > 
> > This prevents race conditions when multiple processes access the device
> > simultaneously through sysfs attributes or device file operations.
> > 
> > Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
> > 
> > Signed-off-by: Mohammad Amin Hosseini <moahmmad.hosseinii@...il.com>
> > 
> > ---
> > Changes in v4:
> > - Added locking to reader functions (show_mode, show_channel, show_oti)
> > - Fixed incomplete reader/writer synchronization that could still race
> > - Ensured all device state access is properly synchronized
> > - Replace sprintf() with sysfs_emit() in all sysfs show functions
> > - Use sysfs_streq() instead of strcmp() for proper input parsing
> > - Implement locked/unlocked SPI function variants to prevent deadlock
> > - Use channel snapshot to ensure atomic read operations
> > - Fix sizeof() usage in spi_read to be more explicit (sizeof(buf))
> > - Make oti write operations atomic (SPI write + shadow update under lock)
> > - Fix race condition in ad7816_set_oti() by taking channel_id snapshot under lock
> > - Fix return type consistency (ssize_t vs int) in show functions
> > - Use chip->id instead of string comparison for channel validation
> > - Add explicit cast for narrowing assignment
> > - Add default case for unknown chip ID validation
> > - Use cansleep GPIO variants in sleepable context
> > - Improve lock documentation for protected resources
> > ---  
> 
> This is way to much to do in a single patch. Also, given that this
> part is obsolete [1] and this driver is in staging, is it really
> worth all of this effort to fix it up?
> 
> [1]: https://www.analog.com/en/products/ad7816.html

Sadly not obsolete (or at least not all of the supported parts).
I checked these out the other day - the ad7817 is a production part.

Biggest issue here is slow down!  Too may versions, without
time for thorough review before another one turns up.  Aim for
at least a few days, or a 1 week between versions.

Jonathan

> 

Powered by blists - more mailing lists