[PATCH 00/10] iio: imu: adis: cleanup lock usage

[PATCH 00/10] iio: imu: adis: cleanup lock usage

[Alexandru Ardelean]

There is a general effort for cleaning up `indio_dev->mlock` usage in IIO
drivers. As part of that some ADIS drivers also need to do that.

This changeset does a little more, by reworking the `txrx_lock` from the
ADIS library to be used as a general `state_lock` to protect state between
consecutive read/write ops.

As such, all users of the ADIS lib have been verified to have their usage
of mlock checked. Some just needed the mlock usage removed (as done in
other patches).

Alexandru Ardelean (10):
  iio: imu: adis: rename txrx_lock -> state_lock
  iio: imu: adis: add unlocked read/write function versions
  iio: imu: adis[16480]: group RW into a single lock in
    adis_enable_irq()
  iio: imu: adis: create an unlocked version of adis_check_status()
  iio: imu: adis: create an unlocked version of adis_reset()
  iio: imu: adis: protect initial startup routine with state lock
  iio: imu: adis: group single conversion under a single state lock
  iio: imu: adis16400: rework locks using ADIS library's state lock
  iio: gyro: adis16136: rework locks using ADIS library's state lock
  iio: imu: adis16480: use state lock for filter freq set

 drivers/iio/gyro/adis16136.c  |  31 ++++---
 drivers/iio/imu/adis.c        |  95 ++++++++++++----------
 drivers/iio/imu/adis16400.c   |  51 ++++++------
 drivers/iio/imu/adis16480.c   |  17 ++--
 drivers/iio/imu/adis_buffer.c |   4 +-
 include/linux/iio/imu/adis.h  | 148 ++++++++++++++++++++++++++++++++--
 6 files changed, 254 insertions(+), 92 deletions(-)

-- 
2.20.1

[PATCH 01/10] iio: imu: adis: rename txrx_lock -> state_lock

[Alexandru Ardelean]

The lock can be extended a bit to protect other elements that are not
particular to just TX/RX. Another idea would have been to just add a new
`state_lock`, but that would mean 2 locks which would be redundant, and
probably cause more potential for dead-locks.

What will be done in the next patches, will be to add some unlocked
versions for read/write_reg functions.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c        | 10 +++++-----
 drivers/iio/imu/adis_buffer.c |  4 ++--
 include/linux/iio/imu/adis.h  |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 1631c255deab..3c2d896e3a96 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
 		},
 	};
 
-	mutex_lock(&adis->txrx_lock);
+	mutex_lock(&adis->state_lock);
 
 	spi_message_init(&msg);
 
@@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
 	}
 
 out_unlock:
-	mutex_unlock(&adis->txrx_lock);
+	mutex_unlock(&adis->state_lock);
 
 	return ret;
 }
@@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
 		},
 	};
 
-	mutex_lock(&adis->txrx_lock);
+	mutex_lock(&adis->state_lock);
 	spi_message_init(&msg);
 
 	if (adis->current_page != page) {
@@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
 	}
 
 out_unlock:
-	mutex_unlock(&adis->txrx_lock);
+	mutex_unlock(&adis->state_lock);
 
 	return ret;
 }
@@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion);
 int adis_init(struct adis *adis, struct iio_dev *indio_dev,
 	struct spi_device *spi, const struct adis_data *data)
 {
-	mutex_init(&adis->txrx_lock);
+	mutex_init(&adis->state_lock);
 	adis->spi = spi;
 	adis->data = data;
 	iio_device_set_drvdata(indio_dev, adis);
diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 9ac8356d9a95..bf581a2c321d 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 		return -ENOMEM;
 
 	if (adis->data->has_paging) {
-		mutex_lock(&adis->txrx_lock);
+		mutex_lock(&adis->state_lock);
 		if (adis->current_page != 0) {
 			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
 			adis->tx[1] = 0;
@@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 
 	if (adis->data->has_paging) {
 		adis->current_page = 0;
-		mutex_unlock(&adis->txrx_lock);
+		mutex_unlock(&adis->state_lock);
 	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 4c53815bb729..3ed5eceaac2d 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -61,7 +61,7 @@ struct adis {
 	const struct adis_data	*data;
 	struct adis_burst	*burst;
 
-	struct mutex		txrx_lock;
+	struct mutex		state_lock;
 	struct spi_message	msg;
 	struct spi_transfer	*xfer;
 	unsigned int		current_page;
-- 
2.20.1

Re: [PATCH 01/10] iio: imu: adis: rename txrx_lock -> state_lock

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:03 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The lock can be extended a bit to protect other elements that are not
> particular to just TX/RX. Another idea would have been to just add a new
> `state_lock`, but that would mean 2 locks which would be redundant, and
> probably cause more potential for dead-locks.
> 
> What will be done in the next patches, will be to add some unlocked
> versions for read/write_reg functions.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Would be good to document the scope of the lock as a comment when it
is defined.  What exactly is 'state' in this case?

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis.c        | 10 +++++-----
>  drivers/iio/imu/adis_buffer.c |  4 ++--
>  include/linux/iio/imu/adis.h  |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 1631c255deab..3c2d896e3a96 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
>  		},
>  	};
>  
> -	mutex_lock(&adis->txrx_lock);
> +	mutex_lock(&adis->state_lock);
>  
>  	spi_message_init(&msg);
>  
> @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
>  	}
>  
>  out_unlock:
> -	mutex_unlock(&adis->txrx_lock);
> +	mutex_unlock(&adis->state_lock);
>  
>  	return ret;
>  }
> @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
>  		},
>  	};
>  
> -	mutex_lock(&adis->txrx_lock);
> +	mutex_lock(&adis->state_lock);
>  	spi_message_init(&msg);
>  
>  	if (adis->current_page != page) {
> @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
>  	}
>  
>  out_unlock:
> -	mutex_unlock(&adis->txrx_lock);
> +	mutex_unlock(&adis->state_lock);
>  
>  	return ret;
>  }
> @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion);
>  int adis_init(struct adis *adis, struct iio_dev *indio_dev,
>  	struct spi_device *spi, const struct adis_data *data)
>  {
> -	mutex_init(&adis->txrx_lock);
> +	mutex_init(&adis->state_lock);
>  	adis->spi = spi;
>  	adis->data = data;
>  	iio_device_set_drvdata(indio_dev, adis);
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 9ac8356d9a95..bf581a2c321d 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>  		return -ENOMEM;
>  
>  	if (adis->data->has_paging) {
> -		mutex_lock(&adis->txrx_lock);
> +		mutex_lock(&adis->state_lock);
>  		if (adis->current_page != 0) {
>  			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
>  			adis->tx[1] = 0;
> @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>  
>  	if (adis->data->has_paging) {
>  		adis->current_page = 0;
> -		mutex_unlock(&adis->txrx_lock);
> +		mutex_unlock(&adis->state_lock);
>  	}
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 4c53815bb729..3ed5eceaac2d 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -61,7 +61,7 @@ struct adis {
>  	const struct adis_data	*data;
>  	struct adis_burst	*burst;
>  
> -	struct mutex		txrx_lock;
> +	struct mutex		state_lock;
>  	struct spi_message	msg;
>  	struct spi_transfer	*xfer;
>  	unsigned int		current_page;

Re: [PATCH 01/10] iio: imu: adis: rename txrx_lock -> state_lock

[Jonathan Cameron]

On Sun, 6 Oct 2019 09:53:33 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 26 Sep 2019 14:18:03 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > The lock can be extended a bit to protect other elements that are not
> > particular to just TX/RX. Another idea would have been to just add a new
> > `state_lock`, but that would mean 2 locks which would be redundant, and
> > probably cause more potential for dead-locks.
> > 
> > What will be done in the next patches, will be to add some unlocked
> > versions for read/write_reg functions.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> 
> Would be good to document the scope of the lock as a comment when it
> is defined.  What exactly is 'state' in this case?
As this can be done as a follow up and the rest of the series is fine
as is...

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/imu/adis.c        | 10 +++++-----
> >  drivers/iio/imu/adis_buffer.c |  4 ++--
> >  include/linux/iio/imu/adis.h  |  2 +-
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > index 1631c255deab..3c2d896e3a96 100644
> > --- a/drivers/iio/imu/adis.c
> > +++ b/drivers/iio/imu/adis.c
> > @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> >  		},
> >  	};
> >  
> > -	mutex_lock(&adis->txrx_lock);
> > +	mutex_lock(&adis->state_lock);
> >  
> >  	spi_message_init(&msg);
> >  
> > @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> >  	}
> >  
> >  out_unlock:
> > -	mutex_unlock(&adis->txrx_lock);
> > +	mutex_unlock(&adis->state_lock);
> >  
> >  	return ret;
> >  }
> > @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> >  		},
> >  	};
> >  
> > -	mutex_lock(&adis->txrx_lock);
> > +	mutex_lock(&adis->state_lock);
> >  	spi_message_init(&msg);
> >  
> >  	if (adis->current_page != page) {
> > @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> >  	}
> >  
> >  out_unlock:
> > -	mutex_unlock(&adis->txrx_lock);
> > +	mutex_unlock(&adis->state_lock);
> >  
> >  	return ret;
> >  }
> > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion);
> >  int adis_init(struct adis *adis, struct iio_dev *indio_dev,
> >  	struct spi_device *spi, const struct adis_data *data)
> >  {
> > -	mutex_init(&adis->txrx_lock);
> > +	mutex_init(&adis->state_lock);
> >  	adis->spi = spi;
> >  	adis->data = data;
> >  	iio_device_set_drvdata(indio_dev, adis);
> > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> > index 9ac8356d9a95..bf581a2c321d 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
> >  		return -ENOMEM;
> >  
> >  	if (adis->data->has_paging) {
> > -		mutex_lock(&adis->txrx_lock);
> > +		mutex_lock(&adis->state_lock);
> >  		if (adis->current_page != 0) {
> >  			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> >  			adis->tx[1] = 0;
> > @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
> >  
> >  	if (adis->data->has_paging) {
> >  		adis->current_page = 0;
> > -		mutex_unlock(&adis->txrx_lock);
> > +		mutex_unlock(&adis->state_lock);
> >  	}
> >  
> >  	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > index 4c53815bb729..3ed5eceaac2d 100644
> > --- a/include/linux/iio/imu/adis.h
> > +++ b/include/linux/iio/imu/adis.h
> > @@ -61,7 +61,7 @@ struct adis {
> >  	const struct adis_data	*data;
> >  	struct adis_burst	*burst;
> >  
> > -	struct mutex		txrx_lock;
> > +	struct mutex		state_lock;
> >  	struct spi_message	msg;
> >  	struct spi_transfer	*xfer;
> >  	unsigned int		current_page;  
> 

Re: [PATCH 01/10] iio: imu: adis: rename txrx_lock -> state_lock

[Ardelean, Alexandru]

On Sun, 2019-10-06 at 10:06 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Sun, 6 Oct 2019 09:53:33 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Thu, 26 Sep 2019 14:18:03 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > 
> > > The lock can be extended a bit to protect other elements that are not
> > > particular to just TX/RX. Another idea would have been to just add a
> > > new
> > > `state_lock`, but that would mean 2 locks which would be redundant,
> > > and
> > > probably cause more potential for dead-locks.
> > > 
> > > What will be done in the next patches, will be to add some unlocked
> > > versions for read/write_reg functions.
> > > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > 
> > Would be good to document the scope of the lock as a comment when it
> > is defined.  What exactly is 'state' in this case?
> As this can be done as a follow up and the rest of the series is fine
> as is...
> 

Will do.

> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
> 
> Thanks,
> 
> Jonathan
> 
> > Thanks,
> > 
> > Jonathan
> > 
> > > ---
> > >  drivers/iio/imu/adis.c        | 10 +++++-----
> > >  drivers/iio/imu/adis_buffer.c |  4 ++--
> > >  include/linux/iio/imu/adis.h  |  2 +-
> > >  3 files changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > index 1631c255deab..3c2d896e3a96 100644
> > > --- a/drivers/iio/imu/adis.c
> > > +++ b/drivers/iio/imu/adis.c
> > > @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > >  		},
> > >  	};
> > >  
> > > -	mutex_lock(&adis->txrx_lock);
> > > +	mutex_lock(&adis->state_lock);
> > >  
> > >  	spi_message_init(&msg);
> > >  
> > > @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > int reg,
> > >  	}
> > >  
> > >  out_unlock:
> > > -	mutex_unlock(&adis->txrx_lock);
> > > +	mutex_unlock(&adis->state_lock);
> > >  
> > >  	return ret;
> > >  }
> > > @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int
> > > reg,
> > >  		},
> > >  	};
> > >  
> > > -	mutex_lock(&adis->txrx_lock);
> > > +	mutex_lock(&adis->state_lock);
> > >  	spi_message_init(&msg);
> > >  
> > >  	if (adis->current_page != page) {
> > > @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int
> > > reg,
> > >  	}
> > >  
> > >  out_unlock:
> > > -	mutex_unlock(&adis->txrx_lock);
> > > +	mutex_unlock(&adis->state_lock);
> > >  
> > >  	return ret;
> > >  }
> > > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion);
> > >  int adis_init(struct adis *adis, struct iio_dev *indio_dev,
> > >  	struct spi_device *spi, const struct adis_data *data)
> > >  {
> > > -	mutex_init(&adis->txrx_lock);
> > > +	mutex_init(&adis->state_lock);
> > >  	adis->spi = spi;
> > >  	adis->data = data;
> > >  	iio_device_set_drvdata(indio_dev, adis);
> > > diff --git a/drivers/iio/imu/adis_buffer.c
> > > b/drivers/iio/imu/adis_buffer.c
> > > index 9ac8356d9a95..bf581a2c321d 100644
> > > --- a/drivers/iio/imu/adis_buffer.c
> > > +++ b/drivers/iio/imu/adis_buffer.c
> > > @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq,
> > > void *p)
> > >  		return -ENOMEM;
> > >  
> > >  	if (adis->data->has_paging) {
> > > -		mutex_lock(&adis->txrx_lock);
> > > +		mutex_lock(&adis->state_lock);
> > >  		if (adis->current_page != 0) {
> > >  			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> > >  			adis->tx[1] = 0;
> > > @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq,
> > > void *p)
> > >  
> > >  	if (adis->data->has_paging) {
> > >  		adis->current_page = 0;
> > > -		mutex_unlock(&adis->txrx_lock);
> > > +		mutex_unlock(&adis->state_lock);
> > >  	}
> > >  
> > >  	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > > diff --git a/include/linux/iio/imu/adis.h
> > > b/include/linux/iio/imu/adis.h
> > > index 4c53815bb729..3ed5eceaac2d 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -61,7 +61,7 @@ struct adis {
> > >  	const struct adis_data	*data;
> > >  	struct adis_burst	*burst;
> > >  
> > > -	struct mutex		txrx_lock;
> > > +	struct mutex		state_lock;
> > >  	struct spi_message	msg;
> > >  	struct spi_transfer	*xfer;
> > >  	unsigned int		current_page;  

[PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Alexandru Ardelean]

This will allow more flexible control to group reads & writes into a single
lock (particularly the state_lock).

The end-goal is to remove the indio_dev->mlock usage, and the simplest fix
would have been to just add another lock, which would not be a good idea on
the long-run.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c       |  34 +++++------
 include/linux/iio/imu/adis.h | 114 ++++++++++++++++++++++++++++++++++-
 2 files changed, 128 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 3c2d896e3a96..4f3be011c898 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -26,7 +26,14 @@
 #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
 #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
 
-int adis_write_reg(struct adis *adis, unsigned int reg,
+/**
+ * __adis_write_reg() - write N bytes to register (unlocked version)
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @value: The value to write to device (up to 4 bytes)
+ * @size: The size of the @value (in bytes)
+ */
+int __adis_write_reg(struct adis *adis, unsigned int reg,
 	unsigned int value, unsigned int size)
 {
 	unsigned int page = reg / ADIS_PAGE_SIZE;
@@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
 		},
 	};
 
-	mutex_lock(&adis->state_lock);
-
 	spi_message_init(&msg);
 
 	if (adis->current_page != page) {
@@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
 		adis->tx[3] = value & 0xff;
 		break;
 	default:
-		ret = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	xfers[size].cs_change = 0;
@@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
 		adis->current_page = page;
 	}
 
-out_unlock:
-	mutex_unlock(&adis->state_lock);
-
 	return ret;
 }
-EXPORT_SYMBOL_GPL(adis_write_reg);
+EXPORT_SYMBOL_GPL(__adis_write_reg);
 
 /**
- * adis_read_reg() - read 2 bytes from a 16-bit register
+ * __adis_read_reg() - read N bytes from register (unlocked version)
  * @adis: The adis device
  * @reg: The address of the lower of the two registers
  * @val: The value read back from the device
+ * @size: The size of the @val buffer
  */
-int adis_read_reg(struct adis *adis, unsigned int reg,
+int __adis_read_reg(struct adis *adis, unsigned int reg,
 	unsigned int *val, unsigned int size)
 {
 	unsigned int page = reg / ADIS_PAGE_SIZE;
@@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
 		spi_message_add_tail(&xfers[3], &msg);
 		break;
 	default:
-		ret = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	ret = spi_sync(adis->spi, &msg);
 	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to read register 0x%02X: %d\n",
 				reg, ret);
-		goto out_unlock;
+		return ret;
 	} else {
 		adis->current_page = page;
 	}
@@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
 		break;
 	}
 
-out_unlock:
-	mutex_unlock(&adis->state_lock);
-
 	return ret;
 }
-EXPORT_SYMBOL_GPL(adis_read_reg);
+EXPORT_SYMBOL_GPL(__adis_read_reg);
 
 #ifdef CONFIG_DEBUG_FS
 
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 3ed5eceaac2d..3a028c40e04e 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
 	struct spi_device *spi, const struct adis_data *data);
 int adis_reset(struct adis *adis);
 
-int adis_write_reg(struct adis *adis, unsigned int reg,
+int __adis_write_reg(struct adis *adis, unsigned int reg,
 	unsigned int val, unsigned int size);
-int adis_read_reg(struct adis *adis, unsigned int reg,
+int __adis_read_reg(struct adis *adis, unsigned int reg,
 	unsigned int *val, unsigned int size);
 
+/**
+ * __adis_write_reg_8() - Write single byte to a register (unlocked version)
+ * @adis: The adis device
+ * @reg: The address of the register to be written
+ * @value: The value to write
+ */
+static inline int __adis_write_reg_8(struct adis *adis, unsigned int reg,
+	uint8_t val)
+{
+	return __adis_write_reg(adis, reg, val, 1);
+}
+
+/**
+ * __adis_write_reg_16() - Write 2 bytes to a pair of registers (unlocked version)
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @value: Value to be written
+ */
+static inline int __adis_write_reg_16(struct adis *adis, unsigned int reg,
+	uint16_t val)
+{
+	return __adis_write_reg(adis, reg, val, 2);
+}
+
+/**
+ * __adis_write_reg_32() - write 4 bytes to four registers (unlocked version)
+ * @adis: The adis device
+ * @reg: The address of the lower of the four register
+ * @value: Value to be written
+ */
+static inline int __adis_write_reg_32(struct adis *adis, unsigned int reg,
+	uint32_t val)
+{
+	return __adis_write_reg(adis, reg, val, 4);
+}
+
+/**
+ * __adis_read_reg_16() - read 2 bytes from a 16-bit register (unlocked version)
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @val: The value read back from the device
+ */
+static inline int __adis_read_reg_16(struct adis *adis, unsigned int reg,
+	uint16_t *val)
+{
+	unsigned int tmp;
+	int ret;
+
+	ret = __adis_read_reg(adis, reg, &tmp, 2);
+	*val = tmp;
+
+	return ret;
+}
+
+/**
+ * __adis_read_reg_32() - read 4 bytes from a 32-bit register (unlocked version)
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @val: The value read back from the device
+ */
+static inline int __adis_read_reg_32(struct adis *adis, unsigned int reg,
+	uint32_t *val)
+{
+	unsigned int tmp;
+	int ret;
+
+	ret = __adis_read_reg(adis, reg, &tmp, 4);
+	*val = tmp;
+
+	return ret;
+}
+
+/**
+ * adis_write_reg() - write N bytes to register
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @value: The value to write to device (up to 4 bytes)
+ * @size: The size of the @value (in bytes)
+ */
+static inline int adis_write_reg(struct adis *adis, unsigned int reg,
+	unsigned int val, unsigned int size)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	ret = __adis_write_reg(adis, reg, val, size);
+	mutex_unlock(&adis->state_lock);
+
+	return ret;
+}
+
+/**
+ * adis_read_reg() - read N bytes from register
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @val: The value read back from the device
+ * @size: The size of the @val buffer
+ */
+static int adis_read_reg(struct adis *adis, unsigned int reg,
+	unsigned int *val, unsigned int size)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	ret = __adis_read_reg(adis, reg, val, size);
+	mutex_unlock(&adis->state_lock);
+
+	return ret;
+}
+
 /**
  * adis_write_reg_8() - Write single byte to a register
  * @adis: The adis device
-- 
2.20.1

Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:04 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This will allow more flexible control to group reads & writes into a single
> lock (particularly the state_lock).
> 
> The end-goal is to remove the indio_dev->mlock usage, and the simplest fix
> would have been to just add another lock, which would not be a good idea on
> the long-run.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing etc.

Jonathan

> ---
>  drivers/iio/imu/adis.c       |  34 +++++------
>  include/linux/iio/imu/adis.h | 114 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 128 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 3c2d896e3a96..4f3be011c898 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -26,7 +26,14 @@
>  #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
>  #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
>  
> -int adis_write_reg(struct adis *adis, unsigned int reg,
> +/**
> + * __adis_write_reg() - write N bytes to register (unlocked version)
> + * @adis: The adis device
> + * @reg: The address of the lower of the two registers
> + * @value: The value to write to device (up to 4 bytes)
> + * @size: The size of the @value (in bytes)
> + */
> +int __adis_write_reg(struct adis *adis, unsigned int reg,
>  	unsigned int value, unsigned int size)
>  {
>  	unsigned int page = reg / ADIS_PAGE_SIZE;
> @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
>  		},
>  	};
>  
> -	mutex_lock(&adis->state_lock);
> -
>  	spi_message_init(&msg);
>  
>  	if (adis->current_page != page) {
> @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
>  		adis->tx[3] = value & 0xff;
>  		break;
>  	default:
> -		ret = -EINVAL;
> -		goto out_unlock;
> +		return -EINVAL;
>  	}
>  
>  	xfers[size].cs_change = 0;
> @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
>  		adis->current_page = page;
>  	}
>  
> -out_unlock:
> -	mutex_unlock(&adis->state_lock);
> -
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(adis_write_reg);
> +EXPORT_SYMBOL_GPL(__adis_write_reg);
>  
>  /**
> - * adis_read_reg() - read 2 bytes from a 16-bit register
> + * __adis_read_reg() - read N bytes from register (unlocked version)
>   * @adis: The adis device
>   * @reg: The address of the lower of the two registers
>   * @val: The value read back from the device
> + * @size: The size of the @val buffer
>   */
> -int adis_read_reg(struct adis *adis, unsigned int reg,
> +int __adis_read_reg(struct adis *adis, unsigned int reg,
>  	unsigned int *val, unsigned int size)
>  {
>  	unsigned int page = reg / ADIS_PAGE_SIZE;
> @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
>  		spi_message_add_tail(&xfers[3], &msg);
>  		break;
>  	default:
> -		ret = -EINVAL;
> -		goto out_unlock;
> +		return -EINVAL;
>  	}
>  
>  	ret = spi_sync(adis->spi, &msg);
>  	if (ret) {
>  		dev_err(&adis->spi->dev, "Failed to read register 0x%02X: %d\n",
>  				reg, ret);
> -		goto out_unlock;
> +		return ret;
>  	} else {
>  		adis->current_page = page;
>  	}
> @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
>  		break;
>  	}
>  
> -out_unlock:
> -	mutex_unlock(&adis->state_lock);
> -
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(adis_read_reg);
> +EXPORT_SYMBOL_GPL(__adis_read_reg);
>  
>  #ifdef CONFIG_DEBUG_FS
>  
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 3ed5eceaac2d..3a028c40e04e 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
>  	struct spi_device *spi, const struct adis_data *data);
>  int adis_reset(struct adis *adis);
>  
> -int adis_write_reg(struct adis *adis, unsigned int reg,
> +int __adis_write_reg(struct adis *adis, unsigned int reg,
>  	unsigned int val, unsigned int size);
> -int adis_read_reg(struct adis *adis, unsigned int reg,
> +int __adis_read_reg(struct adis *adis, unsigned int reg,
>  	unsigned int *val, unsigned int size);
>  
> +/**
> + * __adis_write_reg_8() - Write single byte to a register (unlocked version)
> + * @adis: The adis device
> + * @reg: The address of the register to be written
> + * @value: The value to write
> + */
> +static inline int __adis_write_reg_8(struct adis *adis, unsigned int reg,
> +	uint8_t val)
> +{
> +	return __adis_write_reg(adis, reg, val, 1);
> +}
> +
> +/**
> + * __adis_write_reg_16() - Write 2 bytes to a pair of registers (unlocked version)
> + * @adis: The adis device
> + * @reg: The address of the lower of the two registers
> + * @value: Value to be written
> + */
> +static inline int __adis_write_reg_16(struct adis *adis, unsigned int reg,
> +	uint16_t val)
> +{
> +	return __adis_write_reg(adis, reg, val, 2);
> +}
> +
> +/**
> + * __adis_write_reg_32() - write 4 bytes to four registers (unlocked version)
> + * @adis: The adis device
> + * @reg: The address of the lower of the four register
> + * @value: Value to be written
> + */
> +static inline int __adis_write_reg_32(struct adis *adis, unsigned int reg,
> +	uint32_t val)
> +{
> +	return __adis_write_reg(adis, reg, val, 4);
> +}
> +
> +/**
> + * __adis_read_reg_16() - read 2 bytes from a 16-bit register (unlocked version)
> + * @adis: The adis device
> + * @reg: The address of the lower of the two registers
> + * @val: The value read back from the device
> + */
> +static inline int __adis_read_reg_16(struct adis *adis, unsigned int reg,
> +	uint16_t *val)
> +{
> +	unsigned int tmp;
> +	int ret;
> +
> +	ret = __adis_read_reg(adis, reg, &tmp, 2);
> +	*val = tmp;
> +
> +	return ret;
> +}
> +
> +/**
> + * __adis_read_reg_32() - read 4 bytes from a 32-bit register (unlocked version)
> + * @adis: The adis device
> + * @reg: The address of the lower of the two registers
> + * @val: The value read back from the device
> + */
> +static inline int __adis_read_reg_32(struct adis *adis, unsigned int reg,
> +	uint32_t *val)
> +{
> +	unsigned int tmp;
> +	int ret;
> +
> +	ret = __adis_read_reg(adis, reg, &tmp, 4);
> +	*val = tmp;
> +
> +	return ret;
> +}
> +
> +/**
> + * adis_write_reg() - write N bytes to register
> + * @adis: The adis device
> + * @reg: The address of the lower of the two registers
> + * @value: The value to write to device (up to 4 bytes)
> + * @size: The size of the @value (in bytes)
> + */
> +static inline int adis_write_reg(struct adis *adis, unsigned int reg,
> +	unsigned int val, unsigned int size)
> +{
> +	int ret;
> +
> +	mutex_lock(&adis->state_lock);
> +	ret = __adis_write_reg(adis, reg, val, size);
> +	mutex_unlock(&adis->state_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * adis_read_reg() - read N bytes from register
> + * @adis: The adis device
> + * @reg: The address of the lower of the two registers
> + * @val: The value read back from the device
> + * @size: The size of the @val buffer
> + */
> +static int adis_read_reg(struct adis *adis, unsigned int reg,
> +	unsigned int *val, unsigned int size)
> +{
> +	int ret;
> +
> +	mutex_lock(&adis->state_lock);
> +	ret = __adis_read_reg(adis, reg, val, size);
> +	mutex_unlock(&adis->state_lock);
> +
> +	return ret;
> +}
> +
>  /**
>   * adis_write_reg_8() - Write single byte to a register
>   * @adis: The adis device

Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Jonathan Cameron]

On Sun, 6 Oct 2019 10:12:01 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 26 Sep 2019 14:18:04 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This will allow more flexible control to group reads & writes into a single
> > lock (particularly the state_lock).
> > 
> > The end-goal is to remove the indio_dev->mlock usage, and the simplest fix
> > would have been to just add another lock, which would not be a good idea on
> > the long-run.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> Applied to the togreg branch of iio.git and pushed out as testing etc.
> 
> Jonathan
> 
0-day found a potential issue (kind of) in the read functions.

> > ---
> >  drivers/iio/imu/adis.c       |  34 +++++------
> >  include/linux/iio/imu/adis.h | 114 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 128 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > index 3c2d896e3a96..4f3be011c898 100644
> > --- a/drivers/iio/imu/adis.c
> > +++ b/drivers/iio/imu/adis.c
> > @@ -26,7 +26,14 @@
> >  #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> >  #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
> >  
> > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > +/**
> > + * __adis_write_reg() - write N bytes to register (unlocked version)
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the two registers
> > + * @value: The value to write to device (up to 4 bytes)
> > + * @size: The size of the @value (in bytes)
> > + */
> > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> >  	unsigned int value, unsigned int size)
> >  {
> >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> >  		},
> >  	};
> >  
> > -	mutex_lock(&adis->state_lock);
> > -
> >  	spi_message_init(&msg);
> >  
> >  	if (adis->current_page != page) {
> > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> >  		adis->tx[3] = value & 0xff;
> >  		break;
> >  	default:
> > -		ret = -EINVAL;
> > -		goto out_unlock;
> > +		return -EINVAL;
> >  	}
> >  
> >  	xfers[size].cs_change = 0;
> > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> >  		adis->current_page = page;
> >  	}
> >  
> > -out_unlock:
> > -	mutex_unlock(&adis->state_lock);
> > -
> >  	return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> >  
> >  /**
> > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > + * __adis_read_reg() - read N bytes from register (unlocked version)
> >   * @adis: The adis device
> >   * @reg: The address of the lower of the two registers
> >   * @val: The value read back from the device
> > + * @size: The size of the @val buffer
> >   */
> > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> >  	unsigned int *val, unsigned int size)
> >  {
> >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> >  		spi_message_add_tail(&xfers[3], &msg);
> >  		break;
> >  	default:
> > -		ret = -EINVAL;
> > -		goto out_unlock;
> > +		return -EINVAL;
> >  	}
> >  
> >  	ret = spi_sync(adis->spi, &msg);
> >  	if (ret) {
> >  		dev_err(&adis->spi->dev, "Failed to read register 0x%02X: %d\n",
> >  				reg, ret);
> > -		goto out_unlock;
> > +		return ret;
> >  	} else {
> >  		adis->current_page = page;
> >  	}
> > @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> >  		break;
> >  	}
> >  
> > -out_unlock:
> > -	mutex_unlock(&adis->state_lock);
> > -
> >  	return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(adis_read_reg);
> > +EXPORT_SYMBOL_GPL(__adis_read_reg);
> >  
> >  #ifdef CONFIG_DEBUG_FS
> >  
> > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > index 3ed5eceaac2d..3a028c40e04e 100644
> > --- a/include/linux/iio/imu/adis.h
> > +++ b/include/linux/iio/imu/adis.h
> > @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
> >  	struct spi_device *spi, const struct adis_data *data);
> >  int adis_reset(struct adis *adis);
> >  
> > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> >  	unsigned int val, unsigned int size);
> > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> >  	unsigned int *val, unsigned int size);
> >  
> > +/**
> > + * __adis_write_reg_8() - Write single byte to a register (unlocked version)
> > + * @adis: The adis device
> > + * @reg: The address of the register to be written
> > + * @value: The value to write
> > + */
> > +static inline int __adis_write_reg_8(struct adis *adis, unsigned int reg,
> > +	uint8_t val)
> > +{
> > +	return __adis_write_reg(adis, reg, val, 1);
> > +}
> > +
> > +/**
> > + * __adis_write_reg_16() - Write 2 bytes to a pair of registers (unlocked version)
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the two registers
> > + * @value: Value to be written
> > + */
> > +static inline int __adis_write_reg_16(struct adis *adis, unsigned int reg,
> > +	uint16_t val)
> > +{
> > +	return __adis_write_reg(adis, reg, val, 2);
> > +}
> > +
> > +/**
> > + * __adis_write_reg_32() - write 4 bytes to four registers (unlocked version)
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the four register
> > + * @value: Value to be written
> > + */
> > +static inline int __adis_write_reg_32(struct adis *adis, unsigned int reg,
> > +	uint32_t val)
> > +{
> > +	return __adis_write_reg(adis, reg, val, 4);
> > +}
> > +
> > +/**
> > + * __adis_read_reg_16() - read 2 bytes from a 16-bit register (unlocked version)
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the two registers
> > + * @val: The value read back from the device
> > + */
> > +static inline int __adis_read_reg_16(struct adis *adis, unsigned int reg,
> > +	uint16_t *val)
> > +{
> > +	unsigned int tmp;
> > +	int ret;
> > +
> > +	ret = __adis_read_reg(adis, reg, &tmp, 2);
Zero day isn't happy that this can use tmp without it actually being set in the
__adis_read_reg.

I've added
	if (ret)
		return ret;
> > +	*val = tmp;
> > +
> > +	return ret;
and changed this to return 0;

Same in the 32bit case below.

Hmm. There are quite a few sparse warnings in the adis16400 if anyone fancies
cleaning them up ;)

Thanks,

Jonathan

> > +}
> > +
> > +/**
> > + * __adis_read_reg_32() - read 4 bytes from a 32-bit register (unlocked version)
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the two registers
> > + * @val: The value read back from the device
> > + */
> > +static inline int __adis_read_reg_32(struct adis *adis, unsigned int reg,
> > +	uint32_t *val)
> > +{
> > +	unsigned int tmp;
> > +	int ret;
> > +
> > +	ret = __adis_read_reg(adis, reg, &tmp, 4);
> > +	*val = tmp;
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * adis_write_reg() - write N bytes to register
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the two registers
> > + * @value: The value to write to device (up to 4 bytes)
> > + * @size: The size of the @value (in bytes)
> > + */
> > +static inline int adis_write_reg(struct adis *adis, unsigned int reg,
> > +	unsigned int val, unsigned int size)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&adis->state_lock);
> > +	ret = __adis_write_reg(adis, reg, val, size);
> > +	mutex_unlock(&adis->state_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * adis_read_reg() - read N bytes from register
> > + * @adis: The adis device
> > + * @reg: The address of the lower of the two registers
> > + * @val: The value read back from the device
> > + * @size: The size of the @val buffer
> > + */
> > +static int adis_read_reg(struct adis *adis, unsigned int reg,
> > +	unsigned int *val, unsigned int size)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&adis->state_lock);
> > +	ret = __adis_read_reg(adis, reg, val, size);
> > +	mutex_unlock(&adis->state_lock);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * adis_write_reg_8() - Write single byte to a register
> >   * @adis: The adis device  
> 

Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Ardelean, Alexandru]

On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:
> On Sun, 6 Oct 2019 10:12:01 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Thu, 26 Sep 2019 14:18:04 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > 
> > > This will allow more flexible control to group reads & writes into a
> > > single
> > > lock (particularly the state_lock).
> > > 
> > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > simplest fix
> > > would have been to just add another lock, which would not be a good
> > > idea on
> > > the long-run.
> > > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > Applied to the togreg branch of iio.git and pushed out as testing etc.
> > 
> > Jonathan
> > 
> 0-day found a potential issue (kind of) in the read functions.
> 
> > > ---
> > >  drivers/iio/imu/adis.c       |  34 +++++------
> > >  include/linux/iio/imu/adis.h | 114
> > > ++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 128 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > index 3c2d896e3a96..4f3be011c898 100644
> > > --- a/drivers/iio/imu/adis.c
> > > +++ b/drivers/iio/imu/adis.c
> > > @@ -26,7 +26,14 @@
> > >  #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> > >  #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
> > >  
> > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > +/**
> > > + * __adis_write_reg() - write N bytes to register (unlocked version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @value: The value to write to device (up to 4 bytes)
> > > + * @size: The size of the @value (in bytes)
> > > + */
> > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > >  	unsigned int value, unsigned int size)
> > >  {
> > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > >  		},
> > >  	};
> > >  
> > > -	mutex_lock(&adis->state_lock);
> > > -
> > >  	spi_message_init(&msg);
> > >  
> > >  	if (adis->current_page != page) {
> > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > >  		adis->tx[3] = value & 0xff;
> > >  		break;
> > >  	default:
> > > -		ret = -EINVAL;
> > > -		goto out_unlock;
> > > +		return -EINVAL;
> > >  	}
> > >  
> > >  	xfers[size].cs_change = 0;
> > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis, unsigned
> > > int reg,
> > >  		adis->current_page = page;
> > >  	}
> > >  
> > > -out_unlock:
> > > -	mutex_unlock(&adis->state_lock);
> > > -
> > >  	return ret;
> > >  }
> > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > >  
> > >  /**
> > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > + * __adis_read_reg() - read N bytes from register (unlocked version)
> > >   * @adis: The adis device
> > >   * @reg: The address of the lower of the two registers
> > >   * @val: The value read back from the device
> > > + * @size: The size of the @val buffer
> > >   */
> > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > >  	unsigned int *val, unsigned int size)
> > >  {
> > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis, unsigned
> > > int reg,
> > >  		spi_message_add_tail(&xfers[3], &msg);
> > >  		break;
> > >  	default:
> > > -		ret = -EINVAL;
> > > -		goto out_unlock;
> > > +		return -EINVAL;
> > >  	}
> > >  
> > >  	ret = spi_sync(adis->spi, &msg);
> > >  	if (ret) {
> > >  		dev_err(&adis->spi->dev, "Failed to read register 0x%02X:
> > > %d\n",
> > >  				reg, ret);
> > > -		goto out_unlock;
> > > +		return ret;
> > >  	} else {
> > >  		adis->current_page = page;
> > >  	}
> > > @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis, unsigned
> > > int reg,
> > >  		break;
> > >  	}
> > >  
> > > -out_unlock:
> > > -	mutex_unlock(&adis->state_lock);
> > > -
> > >  	return ret;
> > >  }
> > > -EXPORT_SYMBOL_GPL(adis_read_reg);
> > > +EXPORT_SYMBOL_GPL(__adis_read_reg);
> > >  
> > >  #ifdef CONFIG_DEBUG_FS
> > >  
> > > diff --git a/include/linux/iio/imu/adis.h
> > > b/include/linux/iio/imu/adis.h
> > > index 3ed5eceaac2d..3a028c40e04e 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct iio_dev
> > > *indio_dev,
> > >  	struct spi_device *spi, const struct adis_data *data);
> > >  int adis_reset(struct adis *adis);
> > >  
> > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > >  	unsigned int val, unsigned int size);
> > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > >  	unsigned int *val, unsigned int size);
> > >  
> > > +/**
> > > + * __adis_write_reg_8() - Write single byte to a register (unlocked
> > > version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the register to be written
> > > + * @value: The value to write
> > > + */
> > > +static inline int __adis_write_reg_8(struct adis *adis, unsigned int
> > > reg,
> > > +	uint8_t val)
> > > +{
> > > +	return __adis_write_reg(adis, reg, val, 1);
> > > +}
> > > +
> > > +/**
> > > + * __adis_write_reg_16() - Write 2 bytes to a pair of registers
> > > (unlocked version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @value: Value to be written
> > > + */
> > > +static inline int __adis_write_reg_16(struct adis *adis, unsigned
> > > int reg,
> > > +	uint16_t val)
> > > +{
> > > +	return __adis_write_reg(adis, reg, val, 2);
> > > +}
> > > +
> > > +/**
> > > + * __adis_write_reg_32() - write 4 bytes to four registers (unlocked
> > > version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the four register
> > > + * @value: Value to be written
> > > + */
> > > +static inline int __adis_write_reg_32(struct adis *adis, unsigned
> > > int reg,
> > > +	uint32_t val)
> > > +{
> > > +	return __adis_write_reg(adis, reg, val, 4);
> > > +}
> > > +
> > > +/**
> > > + * __adis_read_reg_16() - read 2 bytes from a 16-bit register
> > > (unlocked version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @val: The value read back from the device
> > > + */
> > > +static inline int __adis_read_reg_16(struct adis *adis, unsigned int
> > > reg,
> > > +	uint16_t *val)
> > > +{
> > > +	unsigned int tmp;
> > > +	int ret;
> > > +
> > > +	ret = __adis_read_reg(adis, reg, &tmp, 2);
> Zero day isn't happy that this can use tmp without it actually being set
> in the
> __adis_read_reg.
> 
> I've added
> 	if (ret)
> 		return ret;
> > > +	*val = tmp;
> > > +
> > > +	return ret;
> and changed this to return 0;
> 
> Same in the 32bit case below.
> 
> Hmm. There are quite a few sparse warnings in the adis16400 if anyone
> fancies
> cleaning them up ;)

Well, I've been planning to setup sparse as part of our CI.
I guess this is another nudge in that direction.

Thanks
Alex

> 
> Thanks,
> 
> Jonathan
> 
> > > +}
> > > +
> > > +/**
> > > + * __adis_read_reg_32() - read 4 bytes from a 32-bit register
> > > (unlocked version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @val: The value read back from the device
> > > + */
> > > +static inline int __adis_read_reg_32(struct adis *adis, unsigned int
> > > reg,
> > > +	uint32_t *val)
> > > +{
> > > +	unsigned int tmp;
> > > +	int ret;
> > > +
> > > +	ret = __adis_read_reg(adis, reg, &tmp, 4);
> > > +	*val = tmp;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * adis_write_reg() - write N bytes to register
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @value: The value to write to device (up to 4 bytes)
> > > + * @size: The size of the @value (in bytes)
> > > + */
> > > +static inline int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > > +	unsigned int val, unsigned int size)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&adis->state_lock);
> > > +	ret = __adis_write_reg(adis, reg, val, size);
> > > +	mutex_unlock(&adis->state_lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * adis_read_reg() - read N bytes from register
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @val: The value read back from the device
> > > + * @size: The size of the @val buffer
> > > + */
> > > +static int adis_read_reg(struct adis *adis, unsigned int reg,
> > > +	unsigned int *val, unsigned int size)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&adis->state_lock);
> > > +	ret = __adis_read_reg(adis, reg, val, size);
> > > +	mutex_unlock(&adis->state_lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  /**
> > >   * adis_write_reg_8() - Write single byte to a register
> > >   * @adis: The adis device  

Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Ardelean, Alexandru]

On Tue, 2019-10-08 at 06:54 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:
> > On Sun, 6 Oct 2019 10:12:01 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> > 
> > > On Thu, 26 Sep 2019 14:18:04 +0300
> > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > > 
> > > > This will allow more flexible control to group reads & writes into
> > > > a
> > > > single
> > > > lock (particularly the state_lock).
> > > > 
> > > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > > simplest fix
> > > > would have been to just add another lock, which would not be a good
> > > > idea on
> > > > the long-run.
> > > > 
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > > Applied to the togreg branch of iio.git and pushed out as testing
> > > etc.
> > > 
> > > Jonathan
> > > 
> > 0-day found a potential issue (kind of) in the read functions.
> > 
> > > > ---
> > > >  drivers/iio/imu/adis.c       |  34 +++++------
> > > >  include/linux/iio/imu/adis.h | 114
> > > > ++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 128 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > > index 3c2d896e3a96..4f3be011c898 100644
> > > > --- a/drivers/iio/imu/adis.c
> > > > +++ b/drivers/iio/imu/adis.c
> > > > @@ -26,7 +26,14 @@
> > > >  #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> > > >  #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
> > > >  
> > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > +/**
> > > > + * __adis_write_reg() - write N bytes to register (unlocked
> > > > version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @value: The value to write to device (up to 4 bytes)
> > > > + * @size: The size of the @value (in bytes)
> > > > + */
> > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > >  	unsigned int value, unsigned int size)
> > > >  {
> > > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > >  		},
> > > >  	};
> > > >  
> > > > -	mutex_lock(&adis->state_lock);
> > > > -
> > > >  	spi_message_init(&msg);
> > > >  
> > > >  	if (adis->current_page != page) {
> > > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > >  		adis->tx[3] = value & 0xff;
> > > >  		break;
> > > >  	default:
> > > > -		ret = -EINVAL;
> > > > -		goto out_unlock;
> > > > +		return -EINVAL;
> > > >  	}
> > > >  
> > > >  	xfers[size].cs_change = 0;
> > > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis,
> > > > unsigned
> > > > int reg,
> > > >  		adis->current_page = page;
> > > >  	}
> > > >  
> > > > -out_unlock:
> > > > -	mutex_unlock(&adis->state_lock);
> > > > -
> > > >  	return ret;
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > > >  
> > > >  /**
> > > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > > + * __adis_read_reg() - read N bytes from register (unlocked
> > > > version)
> > > >   * @adis: The adis device
> > > >   * @reg: The address of the lower of the two registers
> > > >   * @val: The value read back from the device
> > > > + * @size: The size of the @val buffer
> > > >   */
> > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > >  	unsigned int *val, unsigned int size)
> > > >  {
> > > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis, unsigned
> > > > int reg,
> > > >  		spi_message_add_tail(&xfers[3], &msg);
> > > >  		break;
> > > >  	default:
> > > > -		ret = -EINVAL;
> > > > -		goto out_unlock;
> > > > +		return -EINVAL;
> > > >  	}
> > > >  
> > > >  	ret = spi_sync(adis->spi, &msg);
> > > >  	if (ret) {
> > > >  		dev_err(&adis->spi->dev, "Failed to read register
> > > > 0x%02X:
> > > > %d\n",
> > > >  				reg, ret);
> > > > -		goto out_unlock;
> > > > +		return ret;
> > > >  	} else {
> > > >  		adis->current_page = page;
> > > >  	}
> > > > @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis, unsigned
> > > > int reg,
> > > >  		break;
> > > >  	}
> > > >  
> > > > -out_unlock:
> > > > -	mutex_unlock(&adis->state_lock);
> > > > -
> > > >  	return ret;
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(adis_read_reg);
> > > > +EXPORT_SYMBOL_GPL(__adis_read_reg);
> > > >  
> > > >  #ifdef CONFIG_DEBUG_FS
> > > >  
> > > > diff --git a/include/linux/iio/imu/adis.h
> > > > b/include/linux/iio/imu/adis.h
> > > > index 3ed5eceaac2d..3a028c40e04e 100644
> > > > --- a/include/linux/iio/imu/adis.h
> > > > +++ b/include/linux/iio/imu/adis.h
> > > > @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct
> > > > iio_dev
> > > > *indio_dev,
> > > >  	struct spi_device *spi, const struct adis_data *data);
> > > >  int adis_reset(struct adis *adis);
> > > >  
> > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > >  	unsigned int val, unsigned int size);
> > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > >  	unsigned int *val, unsigned int size);
> > > >  
> > > > +/**
> > > > + * __adis_write_reg_8() - Write single byte to a register
> > > > (unlocked
> > > > version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the register to be written
> > > > + * @value: The value to write
> > > > + */
> > > > +static inline int __adis_write_reg_8(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > > +	uint8_t val)
> > > > +{
> > > > +	return __adis_write_reg(adis, reg, val, 1);
> > > > +}
> > > > +
> > > > +/**
> > > > + * __adis_write_reg_16() - Write 2 bytes to a pair of registers
> > > > (unlocked version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @value: Value to be written
> > > > + */
> > > > +static inline int __adis_write_reg_16(struct adis *adis, unsigned
> > > > int reg,
> > > > +	uint16_t val)
> > > > +{
> > > > +	return __adis_write_reg(adis, reg, val, 2);
> > > > +}
> > > > +
> > > > +/**
> > > > + * __adis_write_reg_32() - write 4 bytes to four registers
> > > > (unlocked
> > > > version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the four register
> > > > + * @value: Value to be written
> > > > + */
> > > > +static inline int __adis_write_reg_32(struct adis *adis, unsigned
> > > > int reg,
> > > > +	uint32_t val)
> > > > +{
> > > > +	return __adis_write_reg(adis, reg, val, 4);
> > > > +}
> > > > +
> > > > +/**
> > > > + * __adis_read_reg_16() - read 2 bytes from a 16-bit register
> > > > (unlocked version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @val: The value read back from the device
> > > > + */
> > > > +static inline int __adis_read_reg_16(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > > +	uint16_t *val)
> > > > +{
> > > > +	unsigned int tmp;
> > > > +	int ret;
> > > > +
> > > > +	ret = __adis_read_reg(adis, reg, &tmp, 2);
> > Zero day isn't happy that this can use tmp without it actually being
> > set
> > in the
> > __adis_read_reg.
> > 
> > I've added
> > 	if (ret)
> > 		return ret;
> > > > +	*val = tmp;
> > > > +
> > > > +	return ret;
> > and changed this to return 0;
> > 
> > Same in the 32bit case below.
> > 
> > Hmm. There are quite a few sparse warnings in the adis16400 if anyone
> > fancies
> > cleaning them up ;)
> 
> Well, I've been planning to setup sparse as part of our CI.
> I guess this is another nudge in that direction.

I did find some warnings.
But they seem to be reported by GCC 8.
Not sure if GCC 7 reported them.
I re-installed my laptop [changed to another one], so maybe my default is
now 8; or maybe I did not notice this initially when I built it and picked
it up from our master branch.

Sparse reports nothing for the IMU drivers.
Should I try something newer?

sparse --version
0.6.0 (Ubuntu: 0.6.0-3)

-----------------------------------------------------------------
ARCH=arm CROSS_COMPILE=~/work/linux/gcc-linaro-5.5.0-2017.10-x86_64_arm-
linux-gnueabi/bin/arm-linux-gnueabi- make C=2 drivers/iio/imu/
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CHECK   drivers/iio/imu/adis16400.c
  CHECK   drivers/iio/imu/adis16460.c
  CHECK   drivers/iio/imu/adis16480.c
  CHECK   drivers/iio/imu/adis.c
  CHECK   drivers/iio/imu/adis_trigger.c
  CHECK   drivers/iio/imu/adis_buffer.c
-----------------------------------------------------------------

> 
> Thanks
> Alex
> 
> > Thanks,
> > 
> > Jonathan
> > 
> > > > +}
> > > > +
> > > > +/**
> > > > + * __adis_read_reg_32() - read 4 bytes from a 32-bit register
> > > > (unlocked version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @val: The value read back from the device
> > > > + */
> > > > +static inline int __adis_read_reg_32(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > > +	uint32_t *val)
> > > > +{
> > > > +	unsigned int tmp;
> > > > +	int ret;
> > > > +
> > > > +	ret = __adis_read_reg(adis, reg, &tmp, 4);
> > > > +	*val = tmp;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * adis_write_reg() - write N bytes to register
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @value: The value to write to device (up to 4 bytes)
> > > > + * @size: The size of the @value (in bytes)
> > > > + */
> > > > +static inline int adis_write_reg(struct adis *adis, unsigned int
> > > > reg,
> > > > +	unsigned int val, unsigned int size)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&adis->state_lock);
> > > > +	ret = __adis_write_reg(adis, reg, val, size);
> > > > +	mutex_unlock(&adis->state_lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * adis_read_reg() - read N bytes from register
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @val: The value read back from the device
> > > > + * @size: The size of the @val buffer
> > > > + */
> > > > +static int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > +	unsigned int *val, unsigned int size)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&adis->state_lock);
> > > > +	ret = __adis_read_reg(adis, reg, val, size);
> > > > +	mutex_unlock(&adis->state_lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  /**
> > > >   * adis_write_reg_8() - Write single byte to a register
> > > >   * @adis: The adis device  

Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Ardelean, Alexandru]

On Tue, 2019-10-08 at 08:47 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Tue, 2019-10-08 at 06:54 +0000, Ardelean, Alexandru wrote:
> > [External]
> > 
> > On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:
> > > On Sun, 6 Oct 2019 10:12:01 +0100
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > > 
> > > > On Thu, 26 Sep 2019 14:18:04 +0300
> > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > > > 
> > > > > This will allow more flexible control to group reads & writes
> > > > > into
> > > > > a
> > > > > single
> > > > > lock (particularly the state_lock).
> > > > > 
> > > > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > > > simplest fix
> > > > > would have been to just add another lock, which would not be a
> > > > > good
> > > > > idea on
> > > > > the long-run.
> > > > > 
> > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > >   
> > > > Applied to the togreg branch of iio.git and pushed out as testing
> > > > etc.
> > > > 
> > > > Jonathan
> > > > 
> > > 0-day found a potential issue (kind of) in the read functions.
> > > 
> > > > > ---
> > > > >  drivers/iio/imu/adis.c       |  34 +++++------
> > > > >  include/linux/iio/imu/adis.h | 114
> > > > > ++++++++++++++++++++++++++++++++++-
> > > > >  2 files changed, 128 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > > > index 3c2d896e3a96..4f3be011c898 100644
> > > > > --- a/drivers/iio/imu/adis.c
> > > > > +++ b/drivers/iio/imu/adis.c
> > > > > @@ -26,7 +26,14 @@
> > > > >  #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> > > > >  #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
> > > > >  
> > > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > +/**
> > > > > + * __adis_write_reg() - write N bytes to register (unlocked
> > > > > version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @value: The value to write to device (up to 4 bytes)
> > > > > + * @size: The size of the @value (in bytes)
> > > > > + */
> > > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > > >  	unsigned int value, unsigned int size)
> > > > >  {
> > > > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > >  		},
> > > > >  	};
> > > > >  
> > > > > -	mutex_lock(&adis->state_lock);
> > > > > -
> > > > >  	spi_message_init(&msg);
> > > > >  
> > > > >  	if (adis->current_page != page) {
> > > > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > >  		adis->tx[3] = value & 0xff;
> > > > >  		break;
> > > > >  	default:
> > > > > -		ret = -EINVAL;
> > > > > -		goto out_unlock;
> > > > > +		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > >  	xfers[size].cs_change = 0;
> > > > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis,
> > > > > unsigned
> > > > > int reg,
> > > > >  		adis->current_page = page;
> > > > >  	}
> > > > >  
> > > > > -out_unlock:
> > > > > -	mutex_unlock(&adis->state_lock);
> > > > > -
> > > > >  	return ret;
> > > > >  }
> > > > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > > > >  
> > > > >  /**
> > > > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > > > + * __adis_read_reg() - read N bytes from register (unlocked
> > > > > version)
> > > > >   * @adis: The adis device
> > > > >   * @reg: The address of the lower of the two registers
> > > > >   * @val: The value read back from the device
> > > > > + * @size: The size of the @val buffer
> > > > >   */
> > > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > > >  	unsigned int *val, unsigned int size)
> > > > >  {
> > > > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > > @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis,
> > > > > unsigned
> > > > > int reg,
> > > > >  		spi_message_add_tail(&xfers[3], &msg);
> > > > >  		break;
> > > > >  	default:
> > > > > -		ret = -EINVAL;
> > > > > -		goto out_unlock;
> > > > > +		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > >  	ret = spi_sync(adis->spi, &msg);
> > > > >  	if (ret) {
> > > > >  		dev_err(&adis->spi->dev, "Failed to read register
> > > > > 0x%02X:
> > > > > %d\n",
> > > > >  				reg, ret);
> > > > > -		goto out_unlock;
> > > > > +		return ret;
> > > > >  	} else {
> > > > >  		adis->current_page = page;
> > > > >  	}
> > > > > @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis,
> > > > > unsigned
> > > > > int reg,
> > > > >  		break;
> > > > >  	}
> > > > >  
> > > > > -out_unlock:
> > > > > -	mutex_unlock(&adis->state_lock);
> > > > > -
> > > > >  	return ret;
> > > > >  }
> > > > > -EXPORT_SYMBOL_GPL(adis_read_reg);
> > > > > +EXPORT_SYMBOL_GPL(__adis_read_reg);
> > > > >  
> > > > >  #ifdef CONFIG_DEBUG_FS
> > > > >  
> > > > > diff --git a/include/linux/iio/imu/adis.h
> > > > > b/include/linux/iio/imu/adis.h
> > > > > index 3ed5eceaac2d..3a028c40e04e 100644
> > > > > --- a/include/linux/iio/imu/adis.h
> > > > > +++ b/include/linux/iio/imu/adis.h
> > > > > @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct
> > > > > iio_dev
> > > > > *indio_dev,
> > > > >  	struct spi_device *spi, const struct adis_data *data);
> > > > >  int adis_reset(struct adis *adis);
> > > > >  
> > > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > > >  	unsigned int val, unsigned int size);
> > > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > > >  	unsigned int *val, unsigned int size);
> > > > >  
> > > > > +/**
> > > > > + * __adis_write_reg_8() - Write single byte to a register
> > > > > (unlocked
> > > > > version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the register to be written
> > > > > + * @value: The value to write
> > > > > + */
> > > > > +static inline int __adis_write_reg_8(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > > +	uint8_t val)
> > > > > +{
> > > > > +	return __adis_write_reg(adis, reg, val, 1);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * __adis_write_reg_16() - Write 2 bytes to a pair of registers
> > > > > (unlocked version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @value: Value to be written
> > > > > + */
> > > > > +static inline int __adis_write_reg_16(struct adis *adis,
> > > > > unsigned
> > > > > int reg,
> > > > > +	uint16_t val)
> > > > > +{
> > > > > +	return __adis_write_reg(adis, reg, val, 2);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * __adis_write_reg_32() - write 4 bytes to four registers
> > > > > (unlocked
> > > > > version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the four register
> > > > > + * @value: Value to be written
> > > > > + */
> > > > > +static inline int __adis_write_reg_32(struct adis *adis,
> > > > > unsigned
> > > > > int reg,
> > > > > +	uint32_t val)
> > > > > +{
> > > > > +	return __adis_write_reg(adis, reg, val, 4);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * __adis_read_reg_16() - read 2 bytes from a 16-bit register
> > > > > (unlocked version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @val: The value read back from the device
> > > > > + */
> > > > > +static inline int __adis_read_reg_16(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > > +	uint16_t *val)
> > > > > +{
> > > > > +	unsigned int tmp;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = __adis_read_reg(adis, reg, &tmp, 2);
> > > Zero day isn't happy that this can use tmp without it actually being
> > > set
> > > in the
> > > __adis_read_reg.
> > > 
> > > I've added
> > > 	if (ret)
> > > 		return ret;
> > > > > +	*val = tmp;
> > > > > +
> > > > > +	return ret;
> > > and changed this to return 0;
> > > 
> > > Same in the 32bit case below.
> > > 
> > > Hmm. There are quite a few sparse warnings in the adis16400 if anyone
> > > fancies
> > > cleaning them up ;)
> > 
> > Well, I've been planning to setup sparse as part of our CI.
> > I guess this is another nudge in that direction.
> 
> I did find some warnings.
> But they seem to be reported by GCC 8.
> Not sure if GCC 7 reported them.
> I re-installed my laptop [changed to another one], so maybe my default is
> now 8; or maybe I did not notice this initially when I built it and
> picked
> it up from our master branch.
> 
> Sparse reports nothing for the IMU drivers.
> Should I try something newer?
> 
> sparse --version
> 0.6.0 (Ubuntu: 0.6.0-3)
> 
> -----------------------------------------------------------------
> ARCH=arm CROSS_COMPILE=~/work/linux/gcc-linaro-5.5.0-2017.10-x86_64_arm-
> linux-gnueabi/bin/arm-linux-gnueabi- make C=2 drivers/iio/imu/

Wait.
That looks like GCC 5.5
So, maybe I was an idiot.

Oh well  ¯\_(ツ)_/¯
The faults of multi-tasking

>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   CALL    scripts/atomic/check-atomics.sh
>   CHECK   drivers/iio/imu/adis16400.c
>   CHECK   drivers/iio/imu/adis16460.c
>   CHECK   drivers/iio/imu/adis16480.c
>   CHECK   drivers/iio/imu/adis.c
>   CHECK   drivers/iio/imu/adis_trigger.c
>   CHECK   drivers/iio/imu/adis_buffer.c
> -----------------------------------------------------------------
> 
> > Thanks
> > Alex
> > 
> > > Thanks,
> > > 
> > > Jonathan
> > > 
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * __adis_read_reg_32() - read 4 bytes from a 32-bit register
> > > > > (unlocked version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @val: The value read back from the device
> > > > > + */
> > > > > +static inline int __adis_read_reg_32(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > > +	uint32_t *val)
> > > > > +{
> > > > > +	unsigned int tmp;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = __adis_read_reg(adis, reg, &tmp, 4);
> > > > > +	*val = tmp;
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * adis_write_reg() - write N bytes to register
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @value: The value to write to device (up to 4 bytes)
> > > > > + * @size: The size of the @value (in bytes)
> > > > > + */
> > > > > +static inline int adis_write_reg(struct adis *adis, unsigned int
> > > > > reg,
> > > > > +	unsigned int val, unsigned int size)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	mutex_lock(&adis->state_lock);
> > > > > +	ret = __adis_write_reg(adis, reg, val, size);
> > > > > +	mutex_unlock(&adis->state_lock);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * adis_read_reg() - read N bytes from register
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @val: The value read back from the device
> > > > > + * @size: The size of the @val buffer
> > > > > + */
> > > > > +static int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > +	unsigned int *val, unsigned int size)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	mutex_lock(&adis->state_lock);
> > > > > +	ret = __adis_read_reg(adis, reg, val, size);
> > > > > +	mutex_unlock(&adis->state_lock);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * adis_write_reg_8() - Write single byte to a register
> > > > >   * @adis: The adis device  

Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions

[Jonathan Cameron]

On Tue, 8 Oct 2019 08:58:44 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Tue, 2019-10-08 at 08:47 +0000, Ardelean, Alexandru wrote:
> > [External]
> > 
> > On Tue, 2019-10-08 at 06:54 +0000, Ardelean, Alexandru wrote:  
> > > [External]
> > > 
> > > On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:  
> > > > On Sun, 6 Oct 2019 10:12:01 +0100
> > > > Jonathan Cameron <jic23@kernel.org> wrote:
> > > >   
> > > > > On Thu, 26 Sep 2019 14:18:04 +0300
> > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > > > >   
> > > > > > This will allow more flexible control to group reads & writes
> > > > > > into
> > > > > > a
> > > > > > single
> > > > > > lock (particularly the state_lock).
> > > > > > 
> > > > > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > > > > simplest fix
> > > > > > would have been to just add another lock, which would not be a
> > > > > > good
> > > > > > idea on
> > > > > > the long-run.
> > > > > > 
> > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > >     
> > > > > Applied to the togreg branch of iio.git and pushed out as testing
> > > > > etc.
> > > > > 
> > > > > Jonathan
> > > > >   
> > > > 0-day found a potential issue (kind of) in the read functions.
> > > >   
> > > > > > ---
> > > > > >  drivers/iio/imu/adis.c       |  34 +++++------
> > > > > >  include/linux/iio/imu/adis.h | 114
> > > > > > ++++++++++++++++++++++++++++++++++-
> > > > > >  2 files changed, 128 insertions(+), 20 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > > > > index 3c2d896e3a96..4f3be011c898 100644
> > > > > > --- a/drivers/iio/imu/adis.c
> > > > > > +++ b/drivers/iio/imu/adis.c
> > > > > > @@ -26,7 +26,14 @@
> > > > > >  #define ADIS_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> > > > > >  #define ADIS_GLOB_CMD_SW_RESET		BIT(7)
> > > > > >  
> > > > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > > +/**
> > > > > > + * __adis_write_reg() - write N bytes to register (unlocked
> > > > > > version)
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the two registers
> > > > > > + * @value: The value to write to device (up to 4 bytes)
> > > > > > + * @size: The size of the @value (in bytes)
> > > > > > + */
> > > > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > >  	unsigned int value, unsigned int size)
> > > > > >  {
> > > > > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > > > int
> > > > > > reg,
> > > > > >  		},
> > > > > >  	};
> > > > > >  
> > > > > > -	mutex_lock(&adis->state_lock);
> > > > > > -
> > > > > >  	spi_message_init(&msg);
> > > > > >  
> > > > > >  	if (adis->current_page != page) {
> > > > > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > > > int
> > > > > > reg,
> > > > > >  		adis->tx[3] = value & 0xff;
> > > > > >  		break;
> > > > > >  	default:
> > > > > > -		ret = -EINVAL;
> > > > > > -		goto out_unlock;
> > > > > > +		return -EINVAL;
> > > > > >  	}
> > > > > >  
> > > > > >  	xfers[size].cs_change = 0;
> > > > > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis,
> > > > > > unsigned
> > > > > > int reg,
> > > > > >  		adis->current_page = page;
> > > > > >  	}
> > > > > >  
> > > > > > -out_unlock:
> > > > > > -	mutex_unlock(&adis->state_lock);
> > > > > > -
> > > > > >  	return ret;
> > > > > >  }
> > > > > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > > > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > > > > >  
> > > > > >  /**
> > > > > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > > > > + * __adis_read_reg() - read N bytes from register (unlocked
> > > > > > version)
> > > > > >   * @adis: The adis device
> > > > > >   * @reg: The address of the lower of the two registers
> > > > > >   * @val: The value read back from the device
> > > > > > + * @size: The size of the @val buffer
> > > > > >   */
> > > > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > >  	unsigned int *val, unsigned int size)
> > > > > >  {
> > > > > >  	unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > > > @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis,
> > > > > > unsigned
> > > > > > int reg,
> > > > > >  		spi_message_add_tail(&xfers[3], &msg);
> > > > > >  		break;
> > > > > >  	default:
> > > > > > -		ret = -EINVAL;
> > > > > > -		goto out_unlock;
> > > > > > +		return -EINVAL;
> > > > > >  	}
> > > > > >  
> > > > > >  	ret = spi_sync(adis->spi, &msg);
> > > > > >  	if (ret) {
> > > > > >  		dev_err(&adis->spi->dev, "Failed to read register
> > > > > > 0x%02X:
> > > > > > %d\n",
> > > > > >  				reg, ret);
> > > > > > -		goto out_unlock;
> > > > > > +		return ret;
> > > > > >  	} else {
> > > > > >  		adis->current_page = page;
> > > > > >  	}
> > > > > > @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis,
> > > > > > unsigned
> > > > > > int reg,
> > > > > >  		break;
> > > > > >  	}
> > > > > >  
> > > > > > -out_unlock:
> > > > > > -	mutex_unlock(&adis->state_lock);
> > > > > > -
> > > > > >  	return ret;
> > > > > >  }
> > > > > > -EXPORT_SYMBOL_GPL(adis_read_reg);
> > > > > > +EXPORT_SYMBOL_GPL(__adis_read_reg);
> > > > > >  
> > > > > >  #ifdef CONFIG_DEBUG_FS
> > > > > >  
> > > > > > diff --git a/include/linux/iio/imu/adis.h
> > > > > > b/include/linux/iio/imu/adis.h
> > > > > > index 3ed5eceaac2d..3a028c40e04e 100644
> > > > > > --- a/include/linux/iio/imu/adis.h
> > > > > > +++ b/include/linux/iio/imu/adis.h
> > > > > > @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct
> > > > > > iio_dev
> > > > > > *indio_dev,
> > > > > >  	struct spi_device *spi, const struct adis_data *data);
> > > > > >  int adis_reset(struct adis *adis);
> > > > > >  
> > > > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > >  	unsigned int val, unsigned int size);
> > > > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > >  	unsigned int *val, unsigned int size);
> > > > > >  
> > > > > > +/**
> > > > > > + * __adis_write_reg_8() - Write single byte to a register
> > > > > > (unlocked
> > > > > > version)
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the register to be written
> > > > > > + * @value: The value to write
> > > > > > + */
> > > > > > +static inline int __adis_write_reg_8(struct adis *adis, unsigned
> > > > > > int
> > > > > > reg,
> > > > > > +	uint8_t val)
> > > > > > +{
> > > > > > +	return __adis_write_reg(adis, reg, val, 1);
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * __adis_write_reg_16() - Write 2 bytes to a pair of registers
> > > > > > (unlocked version)
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the two registers
> > > > > > + * @value: Value to be written
> > > > > > + */
> > > > > > +static inline int __adis_write_reg_16(struct adis *adis,
> > > > > > unsigned
> > > > > > int reg,
> > > > > > +	uint16_t val)
> > > > > > +{
> > > > > > +	return __adis_write_reg(adis, reg, val, 2);
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * __adis_write_reg_32() - write 4 bytes to four registers
> > > > > > (unlocked
> > > > > > version)
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the four register
> > > > > > + * @value: Value to be written
> > > > > > + */
> > > > > > +static inline int __adis_write_reg_32(struct adis *adis,
> > > > > > unsigned
> > > > > > int reg,
> > > > > > +	uint32_t val)
> > > > > > +{
> > > > > > +	return __adis_write_reg(adis, reg, val, 4);
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * __adis_read_reg_16() - read 2 bytes from a 16-bit register
> > > > > > (unlocked version)
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the two registers
> > > > > > + * @val: The value read back from the device
> > > > > > + */
> > > > > > +static inline int __adis_read_reg_16(struct adis *adis, unsigned
> > > > > > int
> > > > > > reg,
> > > > > > +	uint16_t *val)
> > > > > > +{
> > > > > > +	unsigned int tmp;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = __adis_read_reg(adis, reg, &tmp, 2);  
> > > > Zero day isn't happy that this can use tmp without it actually being
> > > > set
> > > > in the
> > > > __adis_read_reg.
> > > > 
> > > > I've added
> > > > 	if (ret)
> > > > 		return ret;  
> > > > > > +	*val = tmp;
> > > > > > +
> > > > > > +	return ret;  
> > > > and changed this to return 0;
> > > > 
> > > > Same in the 32bit case below.
> > > > 
> > > > Hmm. There are quite a few sparse warnings in the adis16400 if anyone
> > > > fancies
> > > > cleaning them up ;)  
> > > 
> > > Well, I've been planning to setup sparse as part of our CI.
> > > I guess this is another nudge in that direction.  
> > 
> > I did find some warnings.
> > But they seem to be reported by GCC 8.
> > Not sure if GCC 7 reported them.
> > I re-installed my laptop [changed to another one], so maybe my default is
> > now 8; or maybe I did not notice this initially when I built it and
> > picked
> > it up from our master branch.
> > 
> > Sparse reports nothing for the IMU drivers.
> > Should I try something newer?
Nah. I just misread which thing was producing them whilst doing a
build with sparse and gcc.

Oops.

Thanks,

Jonathan

> > 
> > sparse --version
> > 0.6.0 (Ubuntu: 0.6.0-3)
> > 
> > -----------------------------------------------------------------
> > ARCH=arm CROSS_COMPILE=~/work/linux/gcc-linaro-5.5.0-2017.10-x86_64_arm-
> > linux-gnueabi/bin/arm-linux-gnueabi- make C=2 drivers/iio/imu/  
> 
> Wait.
> That looks like GCC 5.5
> So, maybe I was an idiot.
> 
> Oh well  ¯\_(ツ)_/¯
> The faults of multi-tasking
> 
> >   CHECK   scripts/mod/empty.c
> >   CALL    scripts/checksyscalls.sh
> >   CALL    scripts/atomic/check-atomics.sh
> >   CHECK   drivers/iio/imu/adis16400.c
> >   CHECK   drivers/iio/imu/adis16460.c
> >   CHECK   drivers/iio/imu/adis16480.c
> >   CHECK   drivers/iio/imu/adis.c
> >   CHECK   drivers/iio/imu/adis_trigger.c
> >   CHECK   drivers/iio/imu/adis_buffer.c
> > -----------------------------------------------------------------
> >   
> > > Thanks
> > > Alex
> > >   
> > > > Thanks,
> > > > 
> > > > Jonathan
> > > >   
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * __adis_read_reg_32() - read 4 bytes from a 32-bit register
> > > > > > (unlocked version)
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the two registers
> > > > > > + * @val: The value read back from the device
> > > > > > + */
> > > > > > +static inline int __adis_read_reg_32(struct adis *adis, unsigned
> > > > > > int
> > > > > > reg,
> > > > > > +	uint32_t *val)
> > > > > > +{
> > > > > > +	unsigned int tmp;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = __adis_read_reg(adis, reg, &tmp, 4);
> > > > > > +	*val = tmp;
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * adis_write_reg() - write N bytes to register
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the two registers
> > > > > > + * @value: The value to write to device (up to 4 bytes)
> > > > > > + * @size: The size of the @value (in bytes)
> > > > > > + */
> > > > > > +static inline int adis_write_reg(struct adis *adis, unsigned int
> > > > > > reg,
> > > > > > +	unsigned int val, unsigned int size)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	mutex_lock(&adis->state_lock);
> > > > > > +	ret = __adis_write_reg(adis, reg, val, size);
> > > > > > +	mutex_unlock(&adis->state_lock);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * adis_read_reg() - read N bytes from register
> > > > > > + * @adis: The adis device
> > > > > > + * @reg: The address of the lower of the two registers
> > > > > > + * @val: The value read back from the device
> > > > > > + * @size: The size of the @val buffer
> > > > > > + */
> > > > > > +static int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > > > +	unsigned int *val, unsigned int size)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	mutex_lock(&adis->state_lock);
> > > > > > +	ret = __adis_read_reg(adis, reg, val, size);
> > > > > > +	mutex_unlock(&adis->state_lock);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * adis_write_reg_8() - Write single byte to a register
> > > > > >   * @adis: The adis device    

[PATCH 03/10] iio: imu: adis[16480]: group RW into a single lock in adis_enable_irq()

[Alexandru Ardelean]

The adis_enable_irq() does a read & a write. This change keeps a lock for
the duration of both operations vs for each op.

The change is also needed in adis16480, since that has it's own
implementation for adis_enable_irq().

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c      | 17 +++++++++++------
 drivers/iio/imu/adis16480.c |  4 ++--
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 4f3be011c898..dc30f70d36f3 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -250,12 +250,16 @@ int adis_enable_irq(struct adis *adis, bool enable)
 	int ret = 0;
 	uint16_t msc;
 
-	if (adis->data->enable_irq)
-		return adis->data->enable_irq(adis, enable);
+	mutex_lock(&adis->state_lock);
+
+	if (adis->data->enable_irq) {
+		ret = adis->data->enable_irq(adis, enable);
+		goto out_unlock;
+	}
 
-	ret = adis_read_reg_16(adis, adis->data->msc_ctrl_reg, &msc);
+	ret = __adis_read_reg_16(adis, adis->data->msc_ctrl_reg, &msc);
 	if (ret)
-		goto error_ret;
+		goto out_unlock;
 
 	msc |= ADIS_MSC_CTRL_DATA_RDY_POL_HIGH;
 	msc &= ~ADIS_MSC_CTRL_DATA_RDY_DIO2;
@@ -264,9 +268,10 @@ int adis_enable_irq(struct adis *adis, bool enable)
 	else
 		msc &= ~ADIS_MSC_CTRL_DATA_RDY_EN;
 
-	ret = adis_write_reg_16(adis, adis->data->msc_ctrl_reg, msc);
+	ret = __adis_write_reg_16(adis, adis->data->msc_ctrl_reg, msc);
 
-error_ret:
+out_unlock:
+	mutex_unlock(&adis->state_lock);
 	return ret;
 }
 EXPORT_SYMBOL(adis_enable_irq);
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index b99d73887c9f..dc13d8a33612 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -936,14 +936,14 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
 	uint16_t val;
 	int ret;
 
-	ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
+	ret = __adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
 	if (ret < 0)
 		return ret;
 
 	val &= ~ADIS16480_DRDY_EN_MSK;
 	val |= ADIS16480_DRDY_EN(enable);
 
-	return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
+	return __adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
 }
 
 static int adis16480_initial_setup(struct iio_dev *indio_dev)
-- 
2.20.1

Re: [PATCH 03/10] iio: imu: adis[16480]: group RW into a single lock in adis_enable_irq()

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:05 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The adis_enable_irq() does a read & a write. This change keeps a lock for
> the duration of both operations vs for each op.
> 
> The change is also needed in adis16480, since that has it's own
> implementation for adis_enable_irq().
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis.c      | 17 +++++++++++------
>  drivers/iio/imu/adis16480.c |  4 ++--
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 4f3be011c898..dc30f70d36f3 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -250,12 +250,16 @@ int adis_enable_irq(struct adis *adis, bool enable)
>  	int ret = 0;
>  	uint16_t msc;
>  
> -	if (adis->data->enable_irq)
> -		return adis->data->enable_irq(adis, enable);
> +	mutex_lock(&adis->state_lock);
> +
> +	if (adis->data->enable_irq) {
> +		ret = adis->data->enable_irq(adis, enable);
> +		goto out_unlock;
> +	}
>  
> -	ret = adis_read_reg_16(adis, adis->data->msc_ctrl_reg, &msc);
> +	ret = __adis_read_reg_16(adis, adis->data->msc_ctrl_reg, &msc);
>  	if (ret)
> -		goto error_ret;
> +		goto out_unlock;
>  
>  	msc |= ADIS_MSC_CTRL_DATA_RDY_POL_HIGH;
>  	msc &= ~ADIS_MSC_CTRL_DATA_RDY_DIO2;
> @@ -264,9 +268,10 @@ int adis_enable_irq(struct adis *adis, bool enable)
>  	else
>  		msc &= ~ADIS_MSC_CTRL_DATA_RDY_EN;
>  
> -	ret = adis_write_reg_16(adis, adis->data->msc_ctrl_reg, msc);
> +	ret = __adis_write_reg_16(adis, adis->data->msc_ctrl_reg, msc);
>  
> -error_ret:
> +out_unlock:
> +	mutex_unlock(&adis->state_lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL(adis_enable_irq);
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index b99d73887c9f..dc13d8a33612 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -936,14 +936,14 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
>  	uint16_t val;
>  	int ret;
>  
> -	ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
> +	ret = __adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
>  	if (ret < 0)
>  		return ret;
>  
>  	val &= ~ADIS16480_DRDY_EN_MSK;
>  	val |= ADIS16480_DRDY_EN(enable);
>  
> -	return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
> +	return __adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
>  }
>  
>  static int adis16480_initial_setup(struct iio_dev *indio_dev)

[PATCH 04/10] iio: imu: adis: create an unlocked version of adis_check_status()

[Alexandru Ardelean]

This one also gets re-used in certain operations, so it makes sense to
have an unlocked version of this to group it with other
reads/writes/operations to have a single lock for the whole state change.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c       |  8 ++++----
 include/linux/iio/imu/adis.h | 13 ++++++++++++-
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index dc30f70d36f3..e461b9ae22a5 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -277,18 +277,18 @@ int adis_enable_irq(struct adis *adis, bool enable)
 EXPORT_SYMBOL(adis_enable_irq);
 
 /**
- * adis_check_status() - Check the device for error conditions
+ * __adis_check_status() - Check the device for error conditions (unlocked)
  * @adis: The adis device
  *
  * Returns 0 on success, a negative error code otherwise
  */
-int adis_check_status(struct adis *adis)
+int __adis_check_status(struct adis *adis)
 {
 	uint16_t status;
 	int ret;
 	int i;
 
-	ret = adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
+	ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
 	if (ret < 0)
 		return ret;
 
@@ -306,7 +306,7 @@ int adis_check_status(struct adis *adis)
 
 	return -EIO;
 }
-EXPORT_SYMBOL_GPL(adis_check_status);
+EXPORT_SYMBOL_GPL(__adis_check_status);
 
 /**
  * adis_reset() - Reset the device
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 3a028c40e04e..f4ffba0c36b1 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -263,7 +263,18 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg,
 }
 
 int adis_enable_irq(struct adis *adis, bool enable);
-int adis_check_status(struct adis *adis);
+int __adis_check_status(struct adis *adis);
+
+static inline int adis_check_status(struct adis *adis)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	ret = __adis_check_status(adis);
+	mutex_unlock(&adis->state_lock);
+
+	return ret;
+}
 
 int adis_initial_startup(struct adis *adis);
 
-- 
2.20.1

Re: [PATCH 04/10] iio: imu: adis: create an unlocked version of adis_check_status()

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:06 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This one also gets re-used in certain operations, so it makes sense to
> have an unlocked version of this to group it with other
> reads/writes/operations to have a single lock for the whole state change.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

J
> ---
>  drivers/iio/imu/adis.c       |  8 ++++----
>  include/linux/iio/imu/adis.h | 13 ++++++++++++-
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index dc30f70d36f3..e461b9ae22a5 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -277,18 +277,18 @@ int adis_enable_irq(struct adis *adis, bool enable)
>  EXPORT_SYMBOL(adis_enable_irq);
>  
>  /**
> - * adis_check_status() - Check the device for error conditions
> + * __adis_check_status() - Check the device for error conditions (unlocked)
>   * @adis: The adis device
>   *
>   * Returns 0 on success, a negative error code otherwise
>   */
> -int adis_check_status(struct adis *adis)
> +int __adis_check_status(struct adis *adis)
>  {
>  	uint16_t status;
>  	int ret;
>  	int i;
>  
> -	ret = adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
> +	ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -306,7 +306,7 @@ int adis_check_status(struct adis *adis)
>  
>  	return -EIO;
>  }
> -EXPORT_SYMBOL_GPL(adis_check_status);
> +EXPORT_SYMBOL_GPL(__adis_check_status);
>  
>  /**
>   * adis_reset() - Reset the device
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 3a028c40e04e..f4ffba0c36b1 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -263,7 +263,18 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg,
>  }
>  
>  int adis_enable_irq(struct adis *adis, bool enable);
> -int adis_check_status(struct adis *adis);
> +int __adis_check_status(struct adis *adis);
> +
> +static inline int adis_check_status(struct adis *adis)
> +{
> +	int ret;
> +
> +	mutex_lock(&adis->state_lock);
> +	ret = __adis_check_status(adis);
> +	mutex_unlock(&adis->state_lock);
> +
> +	return ret;
> +}
>  
>  int adis_initial_startup(struct adis *adis);
>  

[PATCH 05/10] iio: imu: adis: create an unlocked version of adis_reset()

[Alexandru Ardelean]

The reset routine may also be important to be protected by the state-lock
and grouped with other operations, so create an unlocked version, so that
this can be done.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c       |  8 ++++----
 include/linux/iio/imu/adis.h | 19 ++++++++++++++++++-
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index e461b9ae22a5..b14101bf34b9 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -309,23 +309,23 @@ int __adis_check_status(struct adis *adis)
 EXPORT_SYMBOL_GPL(__adis_check_status);
 
 /**
- * adis_reset() - Reset the device
+ * __adis_reset() - Reset the device (unlocked version)
  * @adis: The adis device
  *
  * Returns 0 on success, a negative error code otherwise
  */
-int adis_reset(struct adis *adis)
+int __adis_reset(struct adis *adis)
 {
 	int ret;
 
-	ret = adis_write_reg_8(adis, adis->data->glob_cmd_reg,
+	ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
 			ADIS_GLOB_CMD_SW_RESET);
 	if (ret)
 		dev_err(&adis->spi->dev, "Failed to reset device: %d\n", ret);
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(adis_reset);
+EXPORT_SYMBOL_GPL(__adis_reset);
 
 static int adis_self_test(struct adis *adis)
 {
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index f4ffba0c36b1..966af241710f 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -73,7 +73,24 @@ struct adis {
 
 int adis_init(struct adis *adis, struct iio_dev *indio_dev,
 	struct spi_device *spi, const struct adis_data *data);
-int adis_reset(struct adis *adis);
+int __adis_reset(struct adis *adis);
+
+/**
+ * adis_reset() - Reset the device
+ * @adis: The adis device
+ *
+ * Returns 0 on success, a negative error code otherwise
+ */
+static inline int adis_reset(struct adis *adis)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	ret = __adis_reset(adis);
+	mutex_unlock(&adis->state_lock);
+
+	return ret;
+}
 
 int __adis_write_reg(struct adis *adis, unsigned int reg,
 	unsigned int val, unsigned int size);
-- 
2.20.1

Re: [PATCH 05/10] iio: imu: adis: create an unlocked version of adis_reset()

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:07 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The reset routine may also be important to be protected by the state-lock
> and grouped with other operations, so create an unlocked version, so that
> this can be done.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied
> ---
>  drivers/iio/imu/adis.c       |  8 ++++----
>  include/linux/iio/imu/adis.h | 19 ++++++++++++++++++-
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index e461b9ae22a5..b14101bf34b9 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -309,23 +309,23 @@ int __adis_check_status(struct adis *adis)
>  EXPORT_SYMBOL_GPL(__adis_check_status);
>  
>  /**
> - * adis_reset() - Reset the device
> + * __adis_reset() - Reset the device (unlocked version)
>   * @adis: The adis device
>   *
>   * Returns 0 on success, a negative error code otherwise
>   */
> -int adis_reset(struct adis *adis)
> +int __adis_reset(struct adis *adis)
>  {
>  	int ret;
>  
> -	ret = adis_write_reg_8(adis, adis->data->glob_cmd_reg,
> +	ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
>  			ADIS_GLOB_CMD_SW_RESET);
>  	if (ret)
>  		dev_err(&adis->spi->dev, "Failed to reset device: %d\n", ret);
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(adis_reset);
> +EXPORT_SYMBOL_GPL(__adis_reset);
>  
>  static int adis_self_test(struct adis *adis)
>  {
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index f4ffba0c36b1..966af241710f 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -73,7 +73,24 @@ struct adis {
>  
>  int adis_init(struct adis *adis, struct iio_dev *indio_dev,
>  	struct spi_device *spi, const struct adis_data *data);
> -int adis_reset(struct adis *adis);
> +int __adis_reset(struct adis *adis);
> +
> +/**
> + * adis_reset() - Reset the device
> + * @adis: The adis device
> + *
> + * Returns 0 on success, a negative error code otherwise
> + */
> +static inline int adis_reset(struct adis *adis)
> +{
> +	int ret;
> +
> +	mutex_lock(&adis->state_lock);
> +	ret = __adis_reset(adis);
> +	mutex_unlock(&adis->state_lock);
> +
> +	return ret;
> +}
>  
>  int __adis_write_reg(struct adis *adis, unsigned int reg,
>  	unsigned int val, unsigned int size);

[PATCH 06/10] iio: imu: adis: protect initial startup routine with state lock

[Alexandru Ardelean]

The initial startup routine is called by some ADIS drivers during probe,
and before registering with IIO. Normally, userspace should not be able to
do any access to the device (as there shouldn't be any available).

This change extends the state lock to the entire initial-startup routine.
Behaviourally nothing should change, but this should make the library
function a bit more robust.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index b14101bf34b9..7468294d1776 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -331,7 +331,7 @@ static int adis_self_test(struct adis *adis)
 {
 	int ret;
 
-	ret = adis_write_reg_16(adis, adis->data->msc_ctrl_reg,
+	ret = __adis_write_reg_16(adis, adis->data->msc_ctrl_reg,
 			adis->data->self_test_mask);
 	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to initiate self test: %d\n",
@@ -341,10 +341,10 @@ static int adis_self_test(struct adis *adis)
 
 	msleep(adis->data->startup_delay);
 
-	ret = adis_check_status(adis);
+	ret = __adis_check_status(adis);
 
 	if (adis->data->self_test_no_autoclear)
-		adis_write_reg_16(adis, adis->data->msc_ctrl_reg, 0x00);
+		__adis_write_reg_16(adis, adis->data->msc_ctrl_reg, 0x00);
 
 	return ret;
 }
@@ -362,19 +362,23 @@ int adis_initial_startup(struct adis *adis)
 {
 	int ret;
 
+	mutex_lock(&adis->state_lock);
+
 	ret = adis_self_test(adis);
 	if (ret) {
 		dev_err(&adis->spi->dev, "Self-test failed, trying reset.\n");
-		adis_reset(adis);
+		__adis_reset(adis);
 		msleep(adis->data->startup_delay);
 		ret = adis_self_test(adis);
 		if (ret) {
 			dev_err(&adis->spi->dev, "Second self-test failed, giving up.\n");
-			return ret;
+			goto out_unlock;
 		}
 	}
 
-	return 0;
+out_unlock:
+	mutex_unlock(&adis->state_lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(adis_initial_startup);
 
-- 
2.20.1

Re: [PATCH 06/10] iio: imu: adis: protect initial startup routine with state lock

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:08 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The initial startup routine is called by some ADIS drivers during probe,
> and before registering with IIO. Normally, userspace should not be able to
> do any access to the device (as there shouldn't be any available).
> 
> This change extends the state lock to the entire initial-startup routine.
> Behaviourally nothing should change, but this should make the library
> function a bit more robust.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

> ---
>  drivers/iio/imu/adis.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index b14101bf34b9..7468294d1776 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -331,7 +331,7 @@ static int adis_self_test(struct adis *adis)
>  {
>  	int ret;
>  
> -	ret = adis_write_reg_16(adis, adis->data->msc_ctrl_reg,
> +	ret = __adis_write_reg_16(adis, adis->data->msc_ctrl_reg,
>  			adis->data->self_test_mask);
>  	if (ret) {
>  		dev_err(&adis->spi->dev, "Failed to initiate self test: %d\n",
> @@ -341,10 +341,10 @@ static int adis_self_test(struct adis *adis)
>  
>  	msleep(adis->data->startup_delay);
>  
> -	ret = adis_check_status(adis);
> +	ret = __adis_check_status(adis);
>  
>  	if (adis->data->self_test_no_autoclear)
> -		adis_write_reg_16(adis, adis->data->msc_ctrl_reg, 0x00);
> +		__adis_write_reg_16(adis, adis->data->msc_ctrl_reg, 0x00);
>  
>  	return ret;
>  }
> @@ -362,19 +362,23 @@ int adis_initial_startup(struct adis *adis)
>  {
>  	int ret;
>  
> +	mutex_lock(&adis->state_lock);
> +
>  	ret = adis_self_test(adis);
>  	if (ret) {
>  		dev_err(&adis->spi->dev, "Self-test failed, trying reset.\n");
> -		adis_reset(adis);
> +		__adis_reset(adis);
>  		msleep(adis->data->startup_delay);
>  		ret = adis_self_test(adis);
>  		if (ret) {
>  			dev_err(&adis->spi->dev, "Second self-test failed, giving up.\n");
> -			return ret;
> +			goto out_unlock;
>  		}
>  	}
>  
> -	return 0;
> +out_unlock:
> +	mutex_unlock(&adis->state_lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(adis_initial_startup);
>  

[PATCH 07/10] iio: imu: adis: group single conversion under a single state lock

[Alexandru Ardelean]

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 7468294d1776..5e28464ea05b 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -404,15 +404,15 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 	unsigned int uval;
 	int ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&adis->state_lock);
 
-	ret = adis_read_reg(adis, chan->address, &uval,
+	ret = __adis_read_reg(adis, chan->address, &uval,
 			chan->scan_type.storagebits / 8);
 	if (ret)
 		goto err_unlock;
 
 	if (uval & error_mask) {
-		ret = adis_check_status(adis);
+		ret = __adis_check_status(adis);
 		if (ret)
 			goto err_unlock;
 	}
@@ -424,7 +424,7 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 
 	ret = IIO_VAL_INT;
 err_unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&adis->state_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(adis_single_conversion);
-- 
2.20.1

Re: [PATCH 07/10] iio: imu: adis: group single conversion under a single state lock

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:09 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.
Thanks,

J
> ---
>  drivers/iio/imu/adis.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 7468294d1776..5e28464ea05b 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -404,15 +404,15 @@ int adis_single_conversion(struct iio_dev *indio_dev,
>  	unsigned int uval;
>  	int ret;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&adis->state_lock);
>  
> -	ret = adis_read_reg(adis, chan->address, &uval,
> +	ret = __adis_read_reg(adis, chan->address, &uval,
>  			chan->scan_type.storagebits / 8);
>  	if (ret)
>  		goto err_unlock;
>  
>  	if (uval & error_mask) {
> -		ret = adis_check_status(adis);
> +		ret = __adis_check_status(adis);
>  		if (ret)
>  			goto err_unlock;
>  	}
> @@ -424,7 +424,7 @@ int adis_single_conversion(struct iio_dev *indio_dev,
>  
>  	ret = IIO_VAL_INT;
>  err_unlock:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&adis->state_lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(adis_single_conversion);

[PATCH 08/10] iio: imu: adis16400: rework locks using ADIS library's state lock

[Alexandru Ardelean]

This change removes the use of indio_dev's mlock in favor using the state
lock from the ADIS library.

The set_freq() & get_freq() hooks are unlocked, so they require specific
locking. That is because in some cases the get_freq() hook is used in
combination with adis16400_set_filter().

In cases where only one read/write is done, the functions that hold the
state lock are used.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16400.c | 51 ++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 0575ff706bd4..e042a2aabf6b 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -162,6 +162,7 @@ struct adis16400_chip_info {
 	unsigned int accel_scale_micro;
 	int temp_scale_nano;
 	int temp_offset;
+	/* set_freq() & get_freq() need to avoid using ADIS lib's state lock */
 	int (*set_freq)(struct adis16400_state *st, unsigned int freq);
 	int (*get_freq)(struct adis16400_state *st);
 };
@@ -326,7 +327,7 @@ static int adis16334_get_freq(struct adis16400_state *st)
 	int ret;
 	uint16_t t;
 
-	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
+	ret = __adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
 	if (ret < 0)
 		return ret;
 
@@ -350,7 +351,7 @@ static int adis16334_set_freq(struct adis16400_state *st, unsigned int freq)
 	t <<= ADIS16334_RATE_DIV_SHIFT;
 	t |= ADIS16334_RATE_INT_CLK;
 
-	return adis_write_reg_16(&st->adis, ADIS16400_SMPL_PRD, t);
+	return __adis_write_reg_16(&st->adis, ADIS16400_SMPL_PRD, t);
 }
 
 static int adis16400_get_freq(struct adis16400_state *st)
@@ -358,7 +359,7 @@ static int adis16400_get_freq(struct adis16400_state *st)
 	int sps, ret;
 	uint16_t t;
 
-	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
+	ret = __adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
 	if (ret < 0)
 		return ret;
 
@@ -390,7 +391,7 @@ static int adis16400_set_freq(struct adis16400_state *st, unsigned int freq)
 	else
 		st->adis.spi->max_speed_hz = ADIS16400_SPI_FAST;
 
-	return adis_write_reg_8(&st->adis, ADIS16400_SMPL_PRD, val);
+	return __adis_write_reg_8(&st->adis, ADIS16400_SMPL_PRD, val);
 }
 
 static const unsigned int adis16400_3db_divisors[] = {
@@ -404,7 +405,7 @@ static const unsigned int adis16400_3db_divisors[] = {
 	[7] = 200, /* Not a valid setting */
 };
 
-static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
+static int __adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
 {
 	struct adis16400_state *st = iio_priv(indio_dev);
 	uint16_t val16;
@@ -415,11 +416,11 @@ static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
 			break;
 	}
 
-	ret = adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
+	ret = __adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
 	if (ret < 0)
 		return ret;
 
-	ret = adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
+	ret = __adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
 					 (val16 & ~0x07) | i);
 	return ret;
 }
@@ -507,32 +508,31 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
 	struct iio_chan_spec const *chan, int val, int val2, long info)
 {
 	struct adis16400_state *st = iio_priv(indio_dev);
+	struct mutex *slock = &st->adis.state_lock;
 	int ret, sps;
 
 	switch (info) {
 	case IIO_CHAN_INFO_CALIBBIAS:
-		mutex_lock(&indio_dev->mlock);
 		ret = adis_write_reg_16(&st->adis,
 				adis16400_addresses[chan->scan_index], val);
-		mutex_unlock(&indio_dev->mlock);
 		return ret;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		/*
 		 * Need to cache values so we can update if the frequency
 		 * changes.
 		 */
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(slock);
 		st->filt_int = val;
 		/* Work out update to current value */
 		sps = st->variant->get_freq(st);
 		if (sps < 0) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(slock);
 			return sps;
 		}
 
-		ret = adis16400_set_filter(indio_dev, sps,
+		ret = __adis16400_set_filter(indio_dev, sps,
 			val * 1000 + val2 / 1000);
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(slock);
 		return ret;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		sps = val * 1000 + val2 / 1000;
@@ -540,9 +540,9 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
 		if (sps <= 0)
 			return -EINVAL;
 
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(slock);
 		ret = st->variant->set_freq(st, sps);
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(slock);
 		return ret;
 	default:
 		return -EINVAL;
@@ -553,6 +553,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 	struct iio_chan_spec const *chan, int *val, int *val2, long info)
 {
 	struct adis16400_state *st = iio_priv(indio_dev);
+	struct mutex *slock = &st->adis.state_lock;
 	int16_t val16;
 	int ret;
 
@@ -596,10 +597,8 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 	case IIO_CHAN_INFO_CALIBBIAS:
-		mutex_lock(&indio_dev->mlock);
 		ret = adis_read_reg_16(&st->adis,
 				adis16400_addresses[chan->scan_index], &val16);
-		mutex_unlock(&indio_dev->mlock);
 		if (ret)
 			return ret;
 		val16 = sign_extend32(val16, 11);
@@ -610,27 +609,27 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		*val = st->variant->temp_offset;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(slock);
 		/* Need both the number of taps and the sampling frequency */
-		ret = adis_read_reg_16(&st->adis,
+		ret = __adis_read_reg_16(&st->adis,
 						ADIS16400_SENS_AVG,
 						&val16);
 		if (ret < 0) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(slock);
 			return ret;
 		}
 		ret = st->variant->get_freq(st);
-		if (ret >= 0) {
-			ret /= adis16400_3db_divisors[val16 & 0x07];
-			*val = ret / 1000;
-			*val2 = (ret % 1000) * 1000;
-		}
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(slock);
 		if (ret < 0)
 			return ret;
+		ret /= adis16400_3db_divisors[val16 & 0x07];
+		*val = ret / 1000;
+		*val2 = (ret % 1000) * 1000;
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(slock);
 		ret = st->variant->get_freq(st);
+		mutex_unlock(slock);
 		if (ret < 0)
 			return ret;
 		*val = ret / 1000;
-- 
2.20.1

Re: [PATCH 08/10] iio: imu: adis16400: rework locks using ADIS library's state lock

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:10 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change removes the use of indio_dev's mlock in favor using the state
> lock from the ADIS library.
> 
> The set_freq() & get_freq() hooks are unlocked, so they require specific
> locking. That is because in some cases the get_freq() hook is used in
> combination with adis16400_set_filter().
> 
> In cases where only one read/write is done, the functions that hold the
> state lock are used.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16400.c | 51 ++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 0575ff706bd4..e042a2aabf6b 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -162,6 +162,7 @@ struct adis16400_chip_info {
>  	unsigned int accel_scale_micro;
>  	int temp_scale_nano;
>  	int temp_offset;
> +	/* set_freq() & get_freq() need to avoid using ADIS lib's state lock */
>  	int (*set_freq)(struct adis16400_state *st, unsigned int freq);
>  	int (*get_freq)(struct adis16400_state *st);
>  };
> @@ -326,7 +327,7 @@ static int adis16334_get_freq(struct adis16400_state *st)
>  	int ret;
>  	uint16_t t;
>  
> -	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> +	ret = __adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -350,7 +351,7 @@ static int adis16334_set_freq(struct adis16400_state *st, unsigned int freq)
>  	t <<= ADIS16334_RATE_DIV_SHIFT;
>  	t |= ADIS16334_RATE_INT_CLK;
>  
> -	return adis_write_reg_16(&st->adis, ADIS16400_SMPL_PRD, t);
> +	return __adis_write_reg_16(&st->adis, ADIS16400_SMPL_PRD, t);
>  }
>  
>  static int adis16400_get_freq(struct adis16400_state *st)
> @@ -358,7 +359,7 @@ static int adis16400_get_freq(struct adis16400_state *st)
>  	int sps, ret;
>  	uint16_t t;
>  
> -	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> +	ret = __adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -390,7 +391,7 @@ static int adis16400_set_freq(struct adis16400_state *st, unsigned int freq)
>  	else
>  		st->adis.spi->max_speed_hz = ADIS16400_SPI_FAST;
>  
> -	return adis_write_reg_8(&st->adis, ADIS16400_SMPL_PRD, val);
> +	return __adis_write_reg_8(&st->adis, ADIS16400_SMPL_PRD, val);
>  }
>  
>  static const unsigned int adis16400_3db_divisors[] = {
> @@ -404,7 +405,7 @@ static const unsigned int adis16400_3db_divisors[] = {
>  	[7] = 200, /* Not a valid setting */
>  };
>  
> -static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
> +static int __adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
>  {
>  	struct adis16400_state *st = iio_priv(indio_dev);
>  	uint16_t val16;
> @@ -415,11 +416,11 @@ static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
>  			break;
>  	}
>  
> -	ret = adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
> +	ret = __adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
> +	ret = __adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
>  					 (val16 & ~0x07) | i);
>  	return ret;
>  }
> @@ -507,32 +508,31 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
>  	struct iio_chan_spec const *chan, int val, int val2, long info)
>  {
>  	struct adis16400_state *st = iio_priv(indio_dev);
> +	struct mutex *slock = &st->adis.state_lock;
>  	int ret, sps;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		mutex_lock(&indio_dev->mlock);
>  		ret = adis_write_reg_16(&st->adis,
>  				adis16400_addresses[chan->scan_index], val);
> -		mutex_unlock(&indio_dev->mlock);
>  		return ret;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		/*
>  		 * Need to cache values so we can update if the frequency
>  		 * changes.
>  		 */
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(slock);
>  		st->filt_int = val;
>  		/* Work out update to current value */
>  		sps = st->variant->get_freq(st);
>  		if (sps < 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(slock);
>  			return sps;
>  		}
>  
> -		ret = adis16400_set_filter(indio_dev, sps,
> +		ret = __adis16400_set_filter(indio_dev, sps,
>  			val * 1000 + val2 / 1000);
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(slock);
>  		return ret;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		sps = val * 1000 + val2 / 1000;
> @@ -540,9 +540,9 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
>  		if (sps <= 0)
>  			return -EINVAL;
>  
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(slock);
>  		ret = st->variant->set_freq(st, sps);
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(slock);
>  		return ret;
>  	default:
>  		return -EINVAL;
> @@ -553,6 +553,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
>  	struct iio_chan_spec const *chan, int *val, int *val2, long info)
>  {
>  	struct adis16400_state *st = iio_priv(indio_dev);
> +	struct mutex *slock = &st->adis.state_lock;
>  	int16_t val16;
>  	int ret;
>  
> @@ -596,10 +597,8 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		mutex_lock(&indio_dev->mlock);
>  		ret = adis_read_reg_16(&st->adis,
>  				adis16400_addresses[chan->scan_index], &val16);
> -		mutex_unlock(&indio_dev->mlock);
>  		if (ret)
>  			return ret;
>  		val16 = sign_extend32(val16, 11);
> @@ -610,27 +609,27 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
>  		*val = st->variant->temp_offset;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(slock);
>  		/* Need both the number of taps and the sampling frequency */
> -		ret = adis_read_reg_16(&st->adis,
> +		ret = __adis_read_reg_16(&st->adis,
>  						ADIS16400_SENS_AVG,
>  						&val16);
>  		if (ret < 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(slock);
>  			return ret;
>  		}
>  		ret = st->variant->get_freq(st);
> -		if (ret >= 0) {
> -			ret /= adis16400_3db_divisors[val16 & 0x07];
> -			*val = ret / 1000;
> -			*val2 = (ret % 1000) * 1000;
> -		}
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(slock);
>  		if (ret < 0)
>  			return ret;
> +		ret /= adis16400_3db_divisors[val16 & 0x07];
> +		*val = ret / 1000;
> +		*val2 = (ret % 1000) * 1000;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(slock);
>  		ret = st->variant->get_freq(st);
> +		mutex_unlock(slock);
>  		if (ret < 0)
>  			return ret;
>  		*val = ret / 1000;

[PATCH 09/10] iio: gyro: adis16136: rework locks using ADIS library's state lock

[Alexandru Ardelean]

This replaces indio_dev's mlock with the state lock/mutex from the ADIS
library.

The __adis16136_get_freq() function has been prefixed to mark it as
unlocked. The adis16136_{set,get}_filter() functions now hold the state
lock for all the ops that they do.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/gyro/adis16136.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index 5bec7ad53d8b..2d2c48f0b996 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -185,12 +185,12 @@ static int adis16136_set_freq(struct adis16136 *adis16136, unsigned int freq)
 	return adis_write_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, t);
 }
 
-static int adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
+static int __adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
 {
 	uint16_t t;
 	int ret;
 
-	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
+	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
 	if (ret < 0)
 		return ret;
 
@@ -224,10 +224,13 @@ static ssize_t adis16136_read_frequency(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct adis16136 *adis16136 = iio_priv(indio_dev);
+	struct mutex *slock = &adis16136->adis.state_lock;
 	unsigned int freq;
 	int ret;
 
-	ret = adis16136_get_freq(adis16136, &freq);
+	mutex_lock(slock);
+	ret = __adis16136_get_freq(adis16136, &freq);
+	mutex_unlock(slock);
 	if (ret < 0)
 		return ret;
 
@@ -252,42 +255,50 @@ static const unsigned adis16136_3db_divisors[] = {
 static int adis16136_set_filter(struct iio_dev *indio_dev, int val)
 {
 	struct adis16136 *adis16136 = iio_priv(indio_dev);
+	struct mutex *slock = &adis16136->adis.state_lock;
 	unsigned int freq;
 	int i, ret;
 
-	ret = adis16136_get_freq(adis16136, &freq);
+	mutex_lock(slock);
+	ret = __adis16136_get_freq(adis16136, &freq);
 	if (ret < 0)
-		return ret;
+		goto out_unlock;
 
 	for (i = ARRAY_SIZE(adis16136_3db_divisors) - 1; i >= 1; i--) {
 		if (freq / adis16136_3db_divisors[i] >= val)
 			break;
 	}
 
-	return adis_write_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, i);
+	ret = __adis_write_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, i);
+out_unlock:
+	mutex_unlock(slock);
+
+	return ret;
 }
 
 static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
 {
 	struct adis16136 *adis16136 = iio_priv(indio_dev);
+	struct mutex *slock = &adis16136->adis.state_lock;
 	unsigned int freq;
 	uint16_t val16;
 	int ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(slock);
 
-	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, &val16);
+	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT,
+				 &val16);
 	if (ret < 0)
 		goto err_unlock;
 
-	ret = adis16136_get_freq(adis16136, &freq);
+	ret = __adis16136_get_freq(adis16136, &freq);
 	if (ret < 0)
 		goto err_unlock;
 
 	*val = freq / adis16136_3db_divisors[val16 & 0x07];
 
 err_unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(slock);
 
 	return ret ? ret : IIO_VAL_INT;
 }
-- 
2.20.1

Re: [PATCH 09/10] iio: gyro: adis16136: rework locks using ADIS library's state lock

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:11 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This replaces indio_dev's mlock with the state lock/mutex from the ADIS
> library.
> 
> The __adis16136_get_freq() function has been prefixed to mark it as
> unlocked. The adis16136_{set,get}_filter() functions now hold the state
> lock for all the ops that they do.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/gyro/adis16136.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
> index 5bec7ad53d8b..2d2c48f0b996 100644
> --- a/drivers/iio/gyro/adis16136.c
> +++ b/drivers/iio/gyro/adis16136.c
> @@ -185,12 +185,12 @@ static int adis16136_set_freq(struct adis16136 *adis16136, unsigned int freq)
>  	return adis_write_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, t);
>  }
>  
> -static int adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
> +static int __adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
>  {
>  	uint16_t t;
>  	int ret;
>  
> -	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
> +	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -224,10 +224,13 @@ static ssize_t adis16136_read_frequency(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct adis16136 *adis16136 = iio_priv(indio_dev);
> +	struct mutex *slock = &adis16136->adis.state_lock;
>  	unsigned int freq;
>  	int ret;
>  
> -	ret = adis16136_get_freq(adis16136, &freq);
> +	mutex_lock(slock);
> +	ret = __adis16136_get_freq(adis16136, &freq);
> +	mutex_unlock(slock);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -252,42 +255,50 @@ static const unsigned adis16136_3db_divisors[] = {
>  static int adis16136_set_filter(struct iio_dev *indio_dev, int val)
>  {
>  	struct adis16136 *adis16136 = iio_priv(indio_dev);
> +	struct mutex *slock = &adis16136->adis.state_lock;
>  	unsigned int freq;
>  	int i, ret;
>  
> -	ret = adis16136_get_freq(adis16136, &freq);
> +	mutex_lock(slock);
> +	ret = __adis16136_get_freq(adis16136, &freq);
>  	if (ret < 0)
> -		return ret;
> +		goto out_unlock;
>  
>  	for (i = ARRAY_SIZE(adis16136_3db_divisors) - 1; i >= 1; i--) {
>  		if (freq / adis16136_3db_divisors[i] >= val)
>  			break;
>  	}
>  
> -	return adis_write_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, i);
> +	ret = __adis_write_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, i);
> +out_unlock:
> +	mutex_unlock(slock);
> +
> +	return ret;
>  }
>  
>  static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
>  {
>  	struct adis16136 *adis16136 = iio_priv(indio_dev);
> +	struct mutex *slock = &adis16136->adis.state_lock;
>  	unsigned int freq;
>  	uint16_t val16;
>  	int ret;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(slock);
>  
> -	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, &val16);
> +	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT,
> +				 &val16);
>  	if (ret < 0)
>  		goto err_unlock;
>  
> -	ret = adis16136_get_freq(adis16136, &freq);
> +	ret = __adis16136_get_freq(adis16136, &freq);
>  	if (ret < 0)
>  		goto err_unlock;
>  
>  	*val = freq / adis16136_3db_divisors[val16 & 0x07];
>  
>  err_unlock:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(slock);
>  
>  	return ret ? ret : IIO_VAL_INT;
>  }

[PATCH 10/10] iio: imu: adis16480: use state lock for filter freq set

[Alexandru Ardelean]

It's the only operation that does 2 operations (a read & a write), so the
unlocked functions can be used under a single state lock.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index dc13d8a33612..01dae50e985b 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -550,6 +550,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 	const struct iio_chan_spec *chan, unsigned int freq)
 {
 	struct adis16480 *st = iio_priv(indio_dev);
+	struct mutex *slock = &st->adis.state_lock;
 	unsigned int enable_mask, offset, reg;
 	unsigned int diff, best_diff;
 	unsigned int i, best_freq;
@@ -560,9 +561,11 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 	offset = ad16480_filter_data[chan->scan_index][1];
 	enable_mask = BIT(offset + 2);
 
-	ret = adis_read_reg_16(&st->adis, reg, &val);
+	mutex_lock(slock);
+
+	ret = __adis_read_reg_16(&st->adis, reg, &val);
 	if (ret < 0)
-		return ret;
+		goto out_unlock;
 
 	if (freq == 0) {
 		val &= ~enable_mask;
@@ -584,7 +587,11 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 		val |= enable_mask;
 	}
 
-	return adis_write_reg_16(&st->adis, reg, val);
+	ret = __adis_write_reg_16(&st->adis, reg, val);
+out_unlock:
+	mutex_unlock(slock);
+
+	return ret;
 }
 
 static int adis16480_read_raw(struct iio_dev *indio_dev,
-- 
2.20.1

Re: [PATCH 10/10] iio: imu: adis16480: use state lock for filter freq set

[Jonathan Cameron]

On Thu, 26 Sep 2019 14:18:12 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> It's the only operation that does 2 operations (a read & a write), so the
> unlocked functions can be used under a single state lock.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index dc13d8a33612..01dae50e985b 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -550,6 +550,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
>  	const struct iio_chan_spec *chan, unsigned int freq)
>  {
>  	struct adis16480 *st = iio_priv(indio_dev);
> +	struct mutex *slock = &st->adis.state_lock;
>  	unsigned int enable_mask, offset, reg;
>  	unsigned int diff, best_diff;
>  	unsigned int i, best_freq;
> @@ -560,9 +561,11 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
>  	offset = ad16480_filter_data[chan->scan_index][1];
>  	enable_mask = BIT(offset + 2);
>  
> -	ret = adis_read_reg_16(&st->adis, reg, &val);
> +	mutex_lock(slock);
> +
> +	ret = __adis_read_reg_16(&st->adis, reg, &val);
>  	if (ret < 0)
> -		return ret;
> +		goto out_unlock;
>  
>  	if (freq == 0) {
>  		val &= ~enable_mask;
> @@ -584,7 +587,11 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
>  		val |= enable_mask;
>  	}
>  
> -	return adis_write_reg_16(&st->adis, reg, val);
> +	ret = __adis_write_reg_16(&st->adis, reg, val);
> +out_unlock:
> +	mutex_unlock(slock);
> +
> +	return ret;
>  }
>  
>  static int adis16480_read_raw(struct iio_dev *indio_dev,