* [PATCH] iio: gyro: adis16260: remove indio_dev mlock
@ 2019-09-19 11:57 Alexandru Ardelean
2019-10-05 14:38 ` Jonathan Cameron
2019-10-08 8:22 ` [PATCH v2] iio: gyro: adis16260: replace mlock with ADIS lib's state_lock Alexandru Ardelean
0 siblings, 2 replies; 6+ messages in thread
From: Alexandru Ardelean @ 2019-09-19 11:57 UTC (permalink / raw)
To: linux-iio; +Cc: Alexandru Ardelean
The internal lock that is by the ADIS library should be sufficient to keep
state consistent. There is no need for an extra lock.
This patch removes it.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/gyro/adis16260.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index 207a0ce13439..0fa93d02062a 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
addr = adis16260_addresses[chan->scan_index][1];
return adis_write_reg_16(adis, addr, val);
case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&indio_dev->mlock);
if (spi_get_device_id(adis->spi)->driver_data)
t = 256 / val;
else
@@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
- mutex_unlock(&indio_dev->mlock);
return ret;
}
return -EINVAL;
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: adis16260: remove indio_dev mlock
2019-09-19 11:57 [PATCH] iio: gyro: adis16260: remove indio_dev mlock Alexandru Ardelean
@ 2019-10-05 14:38 ` Jonathan Cameron
2019-10-05 14:49 ` Jonathan Cameron
2019-10-08 8:22 ` [PATCH v2] iio: gyro: adis16260: replace mlock with ADIS lib's state_lock Alexandru Ardelean
1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2019-10-05 14:38 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio
On Thu, 19 Sep 2019 14:57:16 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> The internal lock that is by the ADIS library should be sufficient to keep
> state consistent. There is no need for an extra lock.
I'm not sure that's true. In theory we could get two different attempts
to set the sampling frequency running concurrently. That could lead to
a race between the point where we set the spi frequency for future
messages and the point where we set the devices sampling frequency.
Bang it stops working. So, whilst it is arguably paranoid I think
you do need a lock here, but it should be something that is driver
local rather than mlock.
thanks,
Jonathan
>
> This patch removes it.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/gyro/adis16260.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
> index 207a0ce13439..0fa93d02062a 100644
> --- a/drivers/iio/gyro/adis16260.c
> +++ b/drivers/iio/gyro/adis16260.c
> @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
> addr = adis16260_addresses[chan->scan_index][1];
> return adis_write_reg_16(adis, addr, val);
> case IIO_CHAN_INFO_SAMP_FREQ:
> - mutex_lock(&indio_dev->mlock);
> if (spi_get_device_id(adis->spi)->driver_data)
> t = 256 / val;
> else
> @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
> adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
> ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
>
> - mutex_unlock(&indio_dev->mlock);
> return ret;
> }
> return -EINVAL;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: adis16260: remove indio_dev mlock
2019-10-05 14:38 ` Jonathan Cameron
@ 2019-10-05 14:49 ` Jonathan Cameron
2019-10-07 7:56 ` Ardelean, Alexandru
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2019-10-05 14:49 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio
On Sat, 5 Oct 2019 15:38:45 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On Thu, 19 Sep 2019 14:57:16 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > The internal lock that is by the ADIS library should be sufficient to keep
> > state consistent. There is no need for an extra lock.
>
> I'm not sure that's true. In theory we could get two different attempts
> to set the sampling frequency running concurrently. That could lead to
> a race between the point where we set the spi frequency for future
> messages and the point where we set the devices sampling frequency.
>
> Bang it stops working. So, whilst it is arguably paranoid I think
> you do need a lock here, but it should be something that is driver
> local rather than mlock.
Thinking a bit more on this do we have a potential issue where we
race with a read on the spi bus as we are changing this frequency?
They might all be 'safe' but I haven't thought it through properly yet.
>
> thanks,
>
> Jonathan
>
>
> >
> > This patch removes it.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/iio/gyro/adis16260.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
> > index 207a0ce13439..0fa93d02062a 100644
> > --- a/drivers/iio/gyro/adis16260.c
> > +++ b/drivers/iio/gyro/adis16260.c
> > @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
> > addr = adis16260_addresses[chan->scan_index][1];
> > return adis_write_reg_16(adis, addr, val);
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > - mutex_lock(&indio_dev->mlock);
> > if (spi_get_device_id(adis->spi)->driver_data)
> > t = 256 / val;
> > else
> > @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
> > adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
> > ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
> >
> > - mutex_unlock(&indio_dev->mlock);
> > return ret;
> > }
> > return -EINVAL;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: adis16260: remove indio_dev mlock
2019-10-05 14:49 ` Jonathan Cameron
@ 2019-10-07 7:56 ` Ardelean, Alexandru
0 siblings, 0 replies; 6+ messages in thread
From: Ardelean, Alexandru @ 2019-10-07 7:56 UTC (permalink / raw)
To: jic23; +Cc: linux-iio
On Sat, 2019-10-05 at 15:49 +0100, Jonathan Cameron wrote:
> [External]
>
> On Sat, 5 Oct 2019 15:38:45 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Thu, 19 Sep 2019 14:57:16 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > The internal lock that is by the ADIS library should be sufficient to
> > > keep
> > > state consistent. There is no need for an extra lock.
> >
> > I'm not sure that's true. In theory we could get two different
> > attempts
> > to set the sampling frequency running concurrently. That could lead to
> > a race between the point where we set the spi frequency for future
> > messages and the point where we set the devices sampling frequency.
> >
> > Bang it stops working. So, whilst it is arguably paranoid I think
> > you do need a lock here, but it should be something that is driver
> > local rather than mlock.
>
> Thinking a bit more on this do we have a potential issue where we
> race with a read on the spi bus as we are changing this frequency?
> They might all be 'safe' but I haven't thought it through properly yet.
>
To be honest, I don't know.
I am seeing what you're saying about the `max_speed_hz` change and that
could be an issue. That detail has slipped me.
I guess, I could rework this patch to use the ADIS library's lock.
thanks
Alex
> > thanks,
> >
> > Jonathan
> >
> >
> > > This patch removes it.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > > drivers/iio/gyro/adis16260.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/gyro/adis16260.c
> > > b/drivers/iio/gyro/adis16260.c
> > > index 207a0ce13439..0fa93d02062a 100644
> > > --- a/drivers/iio/gyro/adis16260.c
> > > +++ b/drivers/iio/gyro/adis16260.c
> > > @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev
> > > *indio_dev,
> > > addr = adis16260_addresses[chan->scan_index][1];
> > > return adis_write_reg_16(adis, addr, val);
> > > case IIO_CHAN_INFO_SAMP_FREQ:
> > > - mutex_lock(&indio_dev->mlock);
> > > if (spi_get_device_id(adis->spi)->driver_data)
> > > t = 256 / val;
> > > else
> > > @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev
> > > *indio_dev,
> > > adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
> > > ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
> > >
> > > - mutex_unlock(&indio_dev->mlock);
> > > return ret;
> > > }
> > > return -EINVAL;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] iio: gyro: adis16260: replace mlock with ADIS lib's state_lock
2019-09-19 11:57 [PATCH] iio: gyro: adis16260: remove indio_dev mlock Alexandru Ardelean
2019-10-05 14:38 ` Jonathan Cameron
@ 2019-10-08 8:22 ` Alexandru Ardelean
2019-10-12 13:43 ` Jonathan Cameron
1 sibling, 1 reply; 6+ messages in thread
From: Alexandru Ardelean @ 2019-10-08 8:22 UTC (permalink / raw)
To: linux-iio; +Cc: jic23, Alexandru Ardelean
This change uses the ADIS library's state_lock to protect the state of the
`max_speed_hz` change that is done during the set of the sampling
frequency.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
Changelog v1 -> v2:
* use ADIS library's state_lock instead indio_dev's mlock ; vs just
removing it (as V1 did)
drivers/iio/gyro/adis16260.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index 207a0ce13439..726a0aa321a8 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -293,7 +293,7 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
addr = adis16260_addresses[chan->scan_index][1];
return adis_write_reg_16(adis, addr, val);
case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&adis->state_lock);
if (spi_get_device_id(adis->spi)->driver_data)
t = 256 / val;
else
@@ -308,9 +308,9 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
adis->spi->max_speed_hz = ADIS16260_SPI_SLOW;
else
adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
- ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
+ ret = __adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&adis->state_lock);
return ret;
}
return -EINVAL;
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: gyro: adis16260: replace mlock with ADIS lib's state_lock
2019-10-08 8:22 ` [PATCH v2] iio: gyro: adis16260: replace mlock with ADIS lib's state_lock Alexandru Ardelean
@ 2019-10-12 13:43 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2019-10-12 13:43 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio
On Tue, 8 Oct 2019 11:22:25 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> This change uses the ADIS library's state_lock to protect the state of the
> `max_speed_hz` change that is done during the set of the sampling
> frequency.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Looks good.
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.
Thanks,
Jonathan
p.s. personal preference is don't put a new patch version as a reply
to an earlier one. Makes it a lot more likely I'll fail to notice
it!
> ---
>
> Changelog v1 -> v2:
> * use ADIS library's state_lock instead indio_dev's mlock ; vs just
> removing it (as V1 did)
>
> drivers/iio/gyro/adis16260.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
> index 207a0ce13439..726a0aa321a8 100644
> --- a/drivers/iio/gyro/adis16260.c
> +++ b/drivers/iio/gyro/adis16260.c
> @@ -293,7 +293,7 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
> addr = adis16260_addresses[chan->scan_index][1];
> return adis_write_reg_16(adis, addr, val);
> case IIO_CHAN_INFO_SAMP_FREQ:
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&adis->state_lock);
> if (spi_get_device_id(adis->spi)->driver_data)
> t = 256 / val;
> else
> @@ -308,9 +308,9 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
> adis->spi->max_speed_hz = ADIS16260_SPI_SLOW;
> else
> adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
> - ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
> + ret = __adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&adis->state_lock);
> return ret;
> }
> return -EINVAL;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-12 13:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 11:57 [PATCH] iio: gyro: adis16260: remove indio_dev mlock Alexandru Ardelean
2019-10-05 14:38 ` Jonathan Cameron
2019-10-05 14:49 ` Jonathan Cameron
2019-10-07 7:56 ` Ardelean, Alexandru
2019-10-08 8:22 ` [PATCH v2] iio: gyro: adis16260: replace mlock with ADIS lib's state_lock Alexandru Ardelean
2019-10-12 13:43 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).