diff mbox series

[02/15] iio: adc: ad799x: do not use internal iio_dev lock

Message ID 20220920112821.975359-3-nuno.sa@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Make 'mlock' really private | expand

Commit Message

Nuno Sa Sept. 20, 2022, 11:28 a.m. UTC
'mlock' was being grabbed when setting the device frequency. In order to
not introduce any functional change a new lock is added. With that in
mind, the lock also needs to be grabbed in the places where 'mlock' is.

On the other places the lock was being used, we can just drop
it since we are only doing one i2c bus read/write which is already
safe.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/ad799x.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Jonathan Cameron Sept. 24, 2022, 2:45 p.m. UTC | #1
On Tue, 20 Sep 2022 13:28:08 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> 'mlock' was being grabbed when setting the device frequency. In order to
> not introduce any functional change a new lock is added. With that in
> mind, the lock also needs to be grabbed in the places where 'mlock' is.

The usage in here is an example of why we originally decided to take mlock
private...  Annoying hard to reason about.  One key thing this description
doesn't mention is protection of st->config vs device state and I think
the original usage of mlock is partly intended to protect that.

Upshot is I'm not confident enough on this one to be happy taking it without
more head scratching or some review from others!

> 
> On the other places the lock was being used, we can just drop
> it since we are only doing one i2c bus read/write which is already
> safe.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

> ---
>  drivers/iio/adc/ad799x.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index 262bd7665b33..838ba8e77de1 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -28,6 +28,7 @@
>  #include <linux/types.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/bitops.h>
>  
>  #include <linux/iio/iio.h>
> @@ -125,6 +126,8 @@ struct ad799x_state {
>  	const struct ad799x_chip_config	*chip_config;
>  	struct regulator		*reg;
>  	struct regulator		*vref;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex			lock;
>  	unsigned			id;
>  	u16				config;
>  
> @@ -290,7 +293,9 @@ static int ad799x_read_raw(struct iio_dev *indio_dev,
>  		ret = iio_device_claim_direct_mode(indio_dev);
>  		if (ret)
>  			return ret;
> +		mutex_lock(&st->lock);

If we claim direct mode for the frequency writing we'll avoid racing with
buffers being enabled or other sysfs accesses that are claiming direct mode.

That made me think we could drop the lock, but the argument gets tricker
around st->config which is used in ad799x_scan_direct() and modified
in write_event_config() in a fashion that means it could be out of sync.
I'm not sure that matters but it is getting hard to reason about.


>  		ret = ad799x_scan_direct(st, chan->scan_index);
> +		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
>  		if (ret < 0)
> @@ -351,7 +356,8 @@ static ssize_t ad799x_write_frequency(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
> +
>  	ret = i2c_smbus_read_byte_data(st->client, AD7998_CYCLE_TMR_REG);
>  	if (ret < 0)
>  		goto error_ret_mutex;
> @@ -373,7 +379,7 @@ static ssize_t ad799x_write_frequency(struct device *dev,
>  	ret = len;
>  
>  error_ret_mutex:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  
>  	return ret;
>  }
> @@ -407,6 +413,8 @@ static int ad799x_write_event_config(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> +	mutex_lock(&st->lock);
> +
I think you do need the lock here and in other places where you want to ensure the
st->config state matches that of the device.

>  	if (state)
>  		st->config |= BIT(chan->scan_index) << AD799X_CHANNEL_SHIFT;
>  	else
> @@ -418,6 +426,7 @@ static int ad799x_write_event_config(struct iio_dev *indio_dev,
>  		st->config &= ~AD7998_ALERT_EN;
>  
>  	ret = ad799x_write_config(st, st->config);
> +	mutex_unlock(&st->lock);
>  	iio_device_release_direct_mode(indio_dev);
>  	return ret;
>  }
> @@ -454,11 +463,9 @@ static int ad799x_write_event_value(struct iio_dev *indio_dev,
>  	if (val < 0 || val > GENMASK(chan->scan_type.realbits - 1, 0))
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
>  	ret = i2c_smbus_write_word_swapped(st->client,
>  		ad799x_threshold_reg(chan, dir, info),
>  		val << chan->scan_type.shift);
> -	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret;
>  }
> @@ -473,10 +480,8 @@ static int ad799x_read_event_value(struct iio_dev *indio_dev,
>  	int ret;
>  	struct ad799x_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
>  	ret = i2c_smbus_read_word_swapped(st->client,
>  		ad799x_threshold_reg(chan, dir, info));
> -	mutex_unlock(&indio_dev->mlock);
>  	if (ret < 0)
>  		return ret;
>  	*val = (ret >> chan->scan_type.shift) &
> @@ -785,6 +790,7 @@ static int ad799x_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	mutex_init(&st->lock);
>  	/* this is only used for device removal purposes */
>  	i2c_set_clientdata(client, indio_dev);
>
Nuno Sá Sept. 26, 2022, 11:22 a.m. UTC | #2
On Sat, 2022-09-24 at 15:45 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 13:28:08 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > 'mlock' was being grabbed when setting the device frequency. In
> > order to
> > not introduce any functional change a new lock is added. With that
> > in
> > mind, the lock also needs to be grabbed in the places where 'mlock'
> > is.
> 
> The usage in here is an example of why we originally decided to take
> mlock
> private...  Annoying hard to reason about.  One key thing this
> description
> doesn't mention is protection of st->config vs device state and I
> think
> the original usage of mlock is partly intended to protect that.
> 
> Upshot is I'm not confident enough on this one to be happy taking it
> without
> more head scratching or some review from others!
> 

Yeah, this one is odd enough...

> > 
> > On the other places the lock was being used, we can just drop
> > it since we are only doing one i2c bus read/write which is already
> > safe.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> > ---
> >  drivers/iio/adc/ad799x.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> > index 262bd7665b33..838ba8e77de1 100644
> > --- a/drivers/iio/adc/ad799x.c
> > +++ b/drivers/iio/adc/ad799x.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/types.h>
> >  #include <linux/err.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/bitops.h>
> >  
> >  #include <linux/iio/iio.h>
> > @@ -125,6 +126,8 @@ struct ad799x_state {
> >         const struct ad799x_chip_config *chip_config;
> >         struct regulator                *reg;
> >         struct regulator                *vref;
> > +       /* lock to protect against multiple access to the device */
> > +       struct mutex                    lock;
> >         unsigned                        id;
> >         u16                             config;
> >  
> > @@ -290,7 +293,9 @@ static int ad799x_read_raw(struct iio_dev
> > *indio_dev,
> >                 ret = iio_device_claim_direct_mode(indio_dev);
> >                 if (ret)
> >                         return ret;
> > +               mutex_lock(&st->lock);
> 
> If we claim direct mode for the frequency writing we'll avoid racing
> with
> buffers being enabled or other sysfs accesses that are claiming
> direct mode.
> 

As you stated in some other patch, changing the frequency while
buffering is probably not a good idea (possible in some devices though)
but the main reason I haven't used the claim direct approach was
because it would change behavior and could, in theory, break some
userspace apps...

> That made me think we could drop the lock, but the argument gets
> tricker
> around st->config which is used in ad799x_scan_direct() and modified
> in write_event_config() in a fashion that means it could be out of
> sync.
> I'm not sure that matters but it is getting hard to reason about.
> 

The write_event_config() also could use some improvement... Note that
st->config is always written even if ad799x_write_config() fails (which
for some devices is possible). I know that for an i2c write to fail
that probably means we have bigger issues but that does not make it
correct :). We should only update the variable after doing the actual
configuration...

- Nuno Sá
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 262bd7665b33..838ba8e77de1 100644
--- a/drivers/iio/adc/ad799x.c
+++ b/drivers/iio/adc/ad799x.c
@@ -28,6 +28,7 @@ 
 #include <linux/types.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/bitops.h>
 
 #include <linux/iio/iio.h>
@@ -125,6 +126,8 @@  struct ad799x_state {
 	const struct ad799x_chip_config	*chip_config;
 	struct regulator		*reg;
 	struct regulator		*vref;
+	/* lock to protect against multiple access to the device */
+	struct mutex			lock;
 	unsigned			id;
 	u16				config;
 
@@ -290,7 +293,9 @@  static int ad799x_read_raw(struct iio_dev *indio_dev,
 		ret = iio_device_claim_direct_mode(indio_dev);
 		if (ret)
 			return ret;
+		mutex_lock(&st->lock);
 		ret = ad799x_scan_direct(st, chan->scan_index);
+		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
@@ -351,7 +356,8 @@  static ssize_t ad799x_write_frequency(struct device *dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
+
 	ret = i2c_smbus_read_byte_data(st->client, AD7998_CYCLE_TMR_REG);
 	if (ret < 0)
 		goto error_ret_mutex;
@@ -373,7 +379,7 @@  static ssize_t ad799x_write_frequency(struct device *dev,
 	ret = len;
 
 error_ret_mutex:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 
 	return ret;
 }
@@ -407,6 +413,8 @@  static int ad799x_write_event_config(struct iio_dev *indio_dev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&st->lock);
+
 	if (state)
 		st->config |= BIT(chan->scan_index) << AD799X_CHANNEL_SHIFT;
 	else
@@ -418,6 +426,7 @@  static int ad799x_write_event_config(struct iio_dev *indio_dev,
 		st->config &= ~AD7998_ALERT_EN;
 
 	ret = ad799x_write_config(st, st->config);
+	mutex_unlock(&st->lock);
 	iio_device_release_direct_mode(indio_dev);
 	return ret;
 }
@@ -454,11 +463,9 @@  static int ad799x_write_event_value(struct iio_dev *indio_dev,
 	if (val < 0 || val > GENMASK(chan->scan_type.realbits - 1, 0))
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
 	ret = i2c_smbus_write_word_swapped(st->client,
 		ad799x_threshold_reg(chan, dir, info),
 		val << chan->scan_type.shift);
-	mutex_unlock(&indio_dev->mlock);
 
 	return ret;
 }
@@ -473,10 +480,8 @@  static int ad799x_read_event_value(struct iio_dev *indio_dev,
 	int ret;
 	struct ad799x_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	ret = i2c_smbus_read_word_swapped(st->client,
 		ad799x_threshold_reg(chan, dir, info));
-	mutex_unlock(&indio_dev->mlock);
 	if (ret < 0)
 		return ret;
 	*val = (ret >> chan->scan_type.shift) &
@@ -785,6 +790,7 @@  static int ad799x_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
+	mutex_init(&st->lock);
 	/* this is only used for device removal purposes */
 	i2c_set_clientdata(client, indio_dev);