[PATCH v2] iio: adc: max1363: replace mlock with own lock

[PATCH v2] iio: adc: max1363: replace mlock with own lock

[Rohit Sarkar]

This change replaces indio_dev's mlock with the drivers own lock. In
each case the lock is needed to protect the driver's own state.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/adc/max1363.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 5c2cc61b666e..198094ad79a1 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -169,6 +169,7 @@ struct max1363_state {
 	const struct max1363_mode	*current_mode;
 	u32				requestedmask;
 	struct regulator		*reg;
+	struct mutex			lock;
 
 	/* Using monitor modes and buffer at the same time is
 	   currently not supported */
@@ -364,7 +365,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 	struct max1363_state *st = iio_priv(indio_dev);
 	struct i2c_client *client = st->client;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
 	/*
 	 * If monitor mode is enabled, the method for reading a single
 	 * channel will have to be rather different and has not yet
@@ -405,7 +406,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 	}
 	*val = data;
 error_ret:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 	return ret;
 
 }
@@ -705,9 +706,9 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
 	if (!found)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->mlock);
 	st->monitor_speed = i;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->mlock);
 
 	return 0;
 }
@@ -810,12 +811,12 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
 	int val;
 	int number = chan->channel;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->mlock);
 	if (dir == IIO_EV_DIR_FALLING)
 		val = (1 << number) & st->mask_low;
 	else
 		val = (1 << number) & st->mask_high;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->mlock);
 
 	return val;
 }
@@ -1587,6 +1588,7 @@ static int max1363_probe(struct i2c_client *client,
 
 	st = iio_priv(indio_dev);
 
+	mutex_init(&st->lock);
 	st->reg = devm_regulator_get(&client->dev, "vcc");
 	if (IS_ERR(st->reg)) {
 		ret = PTR_ERR(st->reg);
-- 
2.23.0.385.gbc12974a89

Re: [PATCH v2] iio: adc: max1363: replace mlock with own lock

[Jonathan Cameron]

On Sat, 29 Feb 2020 00:01:37 +0530
Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:

> This change replaces indio_dev's mlock with the drivers own lock. In
> each case the lock is needed to protect the driver's own state.
> 
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
So this deals with the simpler cases where we want to change the lock.
Makes sense to separate the two.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to have a poke at it. I'll do a pull request in the
next few days and when that gets taken by Greg KH these will then show
up in next and be queued up by him for the merge window that will open
in perhaps 3 weeks time.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/max1363.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 5c2cc61b666e..198094ad79a1 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -169,6 +169,7 @@ struct max1363_state {
>  	const struct max1363_mode	*current_mode;
>  	u32				requestedmask;
>  	struct regulator		*reg;
> +	struct mutex			lock;
>  
>  	/* Using monitor modes and buffer at the same time is
>  	   currently not supported */
> @@ -364,7 +365,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
>  	struct max1363_state *st = iio_priv(indio_dev);
>  	struct i2c_client *client = st->client;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
>  	/*
>  	 * If monitor mode is enabled, the method for reading a single
>  	 * channel will have to be rather different and has not yet
> @@ -405,7 +406,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
>  	}
>  	*val = data;
>  error_ret:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  	return ret;
>  
>  }
> @@ -705,9 +706,9 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
>  	if (!found)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->mlock);
>  	st->monitor_speed = i;
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->mlock);
>  
>  	return 0;
>  }
> @@ -810,12 +811,12 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
>  	int val;
>  	int number = chan->channel;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->mlock);
>  	if (dir == IIO_EV_DIR_FALLING)
>  		val = (1 << number) & st->mask_low;
>  	else
>  		val = (1 << number) & st->mask_high;
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->mlock);
>  
>  	return val;
>  }
> @@ -1587,6 +1588,7 @@ static int max1363_probe(struct i2c_client *client,
>  
>  	st = iio_priv(indio_dev);
>  
> +	mutex_init(&st->lock);
>  	st->reg = devm_regulator_get(&client->dev, "vcc");
>  	if (IS_ERR(st->reg)) {
>  		ret = PTR_ERR(st->reg);

Re: [PATCH v2] iio: adc: max1363: replace mlock with own lock

[Jonathan Cameron]

On Sat, 7 Mar 2020 13:50:50 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 29 Feb 2020 00:01:37 +0530
> Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> 
> > This change replaces indio_dev's mlock with the drivers own lock. In
> > each case the lock is needed to protect the driver's own state.
> > 
> > Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>  
> So this deals with the simpler cases where we want to change the lock.
> Makes sense to separate the two.
> 
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to have a poke at it. I'll do a pull request in the
> next few days and when that gets taken by Greg KH these will then show
> up in next and be queued up by him for the merge window that will open
> in perhaps 3 weeks time.
> 
I spoke too soon.  Local build tests hadn't finished.  Please make
sure you build test all patches before sending them.

make C=1 
which enables sparse as well as gcc errors gives.

Reverted the patch.  Please fix up and send me a v3.

  CHECK   drivers/iio/adc/max1363.c
drivers/iio/adc/max1363.c:709:23: error: no member 'mlock' in struct max1363_state
drivers/iio/adc/max1363.c:711:25: error: no member 'mlock' in struct max1363_state
drivers/iio/adc/max1363.c:814:23: error: no member 'mlock' in struct max1363_state
drivers/iio/adc/max1363.c:819:25: error: no member 'mlock' in struct max1363_state
  CC [M]  drivers/iio/adc/max1363.o
drivers/iio/adc/max1363.c: In function ‘max1363_monitor_store_freq’:
drivers/iio/adc/max1363.c:709:18: error: ‘struct max1363_state’ has no member named ‘mlock’; did you mean ‘lock’?
  709 |  mutex_lock(&st->mlock);
      |                  ^~~~~
      |                  lock
drivers/iio/adc/max1363.c:711:20: error: ‘struct max1363_state’ has no member named ‘mlock’; did you mean ‘lock’?
  711 |  mutex_unlock(&st->mlock);
      |                    ^~~~~
      |                    lock
drivers/iio/adc/max1363.c: In function ‘max1363_read_event_config’:
drivers/iio/adc/max1363.c:814:18: error: ‘struct max1363_state’ has no member named ‘mlock’; did you mean ‘lock’?
  814 |  mutex_lock(&st->mlock);
      |                  ^~~~~
      |                  lock
drivers/iio/adc/max1363.c:819:20: error: ‘struct max1363_state’ has no member named ‘mlock’; did you mean ‘lock’?
  819 |  mutex_unlock(&st->mlock);
      |                    ^~~~~
      |                    lock
make[3]: *** [scripts/Makefile.build:268: drivers/iio/adc/max1363.o] Error 1
make[2]: *** [scripts/Makefile.build:505: drivers/iio/adc] Error 2
make[1]: *** [scripts/Makefile.build:505: drivers/iio] Error 2

> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/max1363.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> > index 5c2cc61b666e..198094ad79a1 100644
> > --- a/drivers/iio/adc/max1363.c
> > +++ b/drivers/iio/adc/max1363.c
> > @@ -169,6 +169,7 @@ struct max1363_state {
> >  	const struct max1363_mode	*current_mode;
> >  	u32				requestedmask;
> >  	struct regulator		*reg;
> > +	struct mutex			lock;
> >  
> >  	/* Using monitor modes and buffer at the same time is
> >  	   currently not supported */
> > @@ -364,7 +365,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
> >  	struct max1363_state *st = iio_priv(indio_dev);
> >  	struct i2c_client *client = st->client;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	mutex_lock(&st->lock);
> >  	/*
> >  	 * If monitor mode is enabled, the method for reading a single
> >  	 * channel will have to be rather different and has not yet
> > @@ -405,7 +406,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
> >  	}
> >  	*val = data;
> >  error_ret:
> > -	mutex_unlock(&indio_dev->mlock);
> > +	mutex_unlock(&st->lock);
> >  	return ret;
> >  
> >  }
> > @@ -705,9 +706,9 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
> >  	if (!found)
> >  		return -EINVAL;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	mutex_lock(&st->mlock);
> >  	st->monitor_speed = i;
> > -	mutex_unlock(&indio_dev->mlock);
> > +	mutex_unlock(&st->mlock);
> >  
> >  	return 0;
> >  }
> > @@ -810,12 +811,12 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
> >  	int val;
> >  	int number = chan->channel;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	mutex_lock(&st->mlock);
> >  	if (dir == IIO_EV_DIR_FALLING)
> >  		val = (1 << number) & st->mask_low;
> >  	else
> >  		val = (1 << number) & st->mask_high;
> > -	mutex_unlock(&indio_dev->mlock);
> > +	mutex_unlock(&st->mlock);
> >  
> >  	return val;
> >  }
> > @@ -1587,6 +1588,7 @@ static int max1363_probe(struct i2c_client *client,
> >  
> >  	st = iio_priv(indio_dev);
> >  
> > +	mutex_init(&st->lock);
> >  	st->reg = devm_regulator_get(&client->dev, "vcc");
> >  	if (IS_ERR(st->reg)) {
> >  		ret = PTR_ERR(st->reg);  
> 

[PATCH v2] iio: adc: max1363: replace mlock with own lock

[Rohit Sarkar]

This change replaces indio_dev's mlock with the drivers own lock. In
each case the lock is needed to protect the driver's own state.

Changes from v1:
Fix indentation.
Add a mutex_init() in the probe function.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/adc/max1363.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 5c2cc61b666e..b9557f957f3c 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -169,6 +169,7 @@ struct max1363_state {
 	const struct max1363_mode	*current_mode;
 	u32				requestedmask;
 	struct regulator		*reg;
+	struct mutex lock;
 
 	/* Using monitor modes and buffer at the same time is
 	   currently not supported */
@@ -364,7 +365,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 	struct max1363_state *st = iio_priv(indio_dev);
 	struct i2c_client *client = st->client;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
 	/*
 	 * If monitor mode is enabled, the method for reading a single
 	 * channel will have to be rather different and has not yet
@@ -405,7 +406,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 	}
 	*val = data;
 error_ret:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 	return ret;
 
 }
@@ -705,9 +706,9 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
 	if (!found)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->mlock);
 	st->monitor_speed = i;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->mlock);
 
 	return 0;
 }
@@ -810,12 +811,12 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
 	int val;
 	int number = chan->channel;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->mlock);
 	if (dir == IIO_EV_DIR_FALLING)
 		val = (1 << number) & st->mask_low;
 	else
 		val = (1 << number) & st->mask_high;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->mlock);
 
 	return val;
 }
-- 
2.23.0.385.gbc12974a89

Re: [PATCH v2] iio: adc: max1363: replace mlock with own lock

[Rohit Sarkar]

+CC Jonathan and Alexandru

Re: [PATCH v2] iio: adc: max1363: replace mlock with own lock

[Ardelean, Alexandru]

On Fri, 2020-02-28 at 00:38 +0530, Rohit Sarkar wrote:
> This change replaces indio_dev's mlock with the drivers own lock. In
> each case the lock is needed to protect the driver's own state.
> 
> Changes from v1:
> Fix indentation.
> Add a mutex_init() in the probe function.
> 

This looks like the first patch.
I don't see the code changes from the first one.


> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> ---
>  drivers/iio/adc/max1363.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 5c2cc61b666e..b9557f957f3c 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -169,6 +169,7 @@ struct max1363_state {
>  	const struct max1363_mode	*current_mode;
>  	u32				requestedmask;
>  	struct regulator		*reg;
> +	struct mutex lock;
>  
>  	/* Using monitor modes and buffer at the same time is
>  	   currently not supported */
> @@ -364,7 +365,7 @@ static int max1363_read_single_chan(struct iio_dev
> *indio_dev,
>  	struct max1363_state *st = iio_priv(indio_dev);
>  	struct i2c_client *client = st->client;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
>  	/*
>  	 * If monitor mode is enabled, the method for reading a single
>  	 * channel will have to be rather different and has not yet
> @@ -405,7 +406,7 @@ static int max1363_read_single_chan(struct iio_dev
> *indio_dev,
>  	}
>  	*val = data;
>  error_ret:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  	return ret;
>  
>  }
> @@ -705,9 +706,9 @@ static ssize_t max1363_monitor_store_freq(struct device
> *dev,
>  	if (!found)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->mlock);
>  	st->monitor_speed = i;
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->mlock);
>  
>  	return 0;
>  }
> @@ -810,12 +811,12 @@ static int max1363_read_event_config(struct iio_dev
> *indio_dev,
>  	int val;
>  	int number = chan->channel;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->mlock);
>  	if (dir == IIO_EV_DIR_FALLING)
>  		val = (1 << number) & st->mask_low;
>  	else
>  		val = (1 << number) & st->mask_high;
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->mlock);
>  
>  	return val;
>  }

Re: [PATCH v2] iio: adc: max1363: replace mlock with own lock

[Rohit Sarkar]

On Fri, Feb 28, 2020 at 07:56:14AM +0000, Ardelean, Alexandru wrote:
> On Fri, 2020-02-28 at 00:38 +0530, Rohit Sarkar wrote:
> > This change replaces indio_dev's mlock with the drivers own lock. In
> > each case the lock is needed to protect the driver's own state.
> > 
> > Changes from v1:
> > Fix indentation.
> > Add a mutex_init() in the probe function.
> > 
> 
> This looks like the first patch.
> I don't see the code changes from the first one.
> 
I messed up sorry. Sent out v2 again.
Thanks,
Rohit

Re: [PATCH v2] iio: adc: max1363: replace mlock with own lock

[Jonathan Cameron]

On Fri, 28 Feb 2020 00:38:37 +0530
Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:

> This change replaces indio_dev's mlock with the drivers own lock. In
> each case the lock is needed to protect the driver's own state.
> 
> Changes from v1:
> Fix indentation.
> Add a mutex_init() in the probe function.
> 
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
Worth taking into account that perhaps some of the mlock cases aren't
actually taking it for local purposes, but rather to explicitly stop
the core from changing between buffered and polled modes (chrdev and sysfs
access).

> ---
>  drivers/iio/adc/max1363.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 5c2cc61b666e..b9557f957f3c 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -169,6 +169,7 @@ struct max1363_state {
>  	const struct max1363_mode	*current_mode;
>  	u32				requestedmask;
>  	struct regulator		*reg;
> +	struct mutex lock;

Scope of locks (what they protect) should always be described.
So please add documentation explaining exactly what this is protecting.

>  
>  	/* Using monitor modes and buffer at the same time is
>  	   currently not supported */
> @@ -364,7 +365,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
>  	struct max1363_state *st = iio_priv(indio_dev);
>  	struct i2c_client *client = st->client;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);

This one is actually about preventing state changes.  So it shouldn't be
directly accessing the lock, but it should be calling
iio_device_claim_direct_mode.  Take a look at what else that function
does.

For this driver things are more complex than normal though as we need
to prevent either switching between polled and buffer mode or
trying to sample with the monitor running.

Hence we may also need to take the local lock to protect the monitor_mode flag.



>  	/*
>  	 * If monitor mode is enabled, the method for reading a single
>  	 * channel will have to be rather different and has not yet
> @@ -405,7 +406,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
>  	}
>  	*val = data;
>  error_ret:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  	return ret;
>  
>  }
> @@ -705,9 +706,9 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
>  	if (!found)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->mlock);
>  	st->monitor_speed = i;
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->mlock);
>  
>  	return 0;
>  }
> @@ -810,12 +811,12 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
>  	int val;
>  	int number = chan->channel;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->mlock);
>  	if (dir == IIO_EV_DIR_FALLING)
>  		val = (1 << number) & st->mask_low;
>  	else
>  		val = (1 << number) & st->mask_high;
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->mlock);
>  
>  	return val;
>  }

Nothing for write_event_config?
That has the same problem that it should only be possible to change monitor mode
if we aren't running in buffered mode.  Hence it should try to claim direct
mode and if it fails return an error.

Fiddly stuff :)

Thanks,

Jonathan