[PATCH v2 0/8] Remove adis_initial_startup usage

[PATCH v2 0/8] Remove adis_initial_startup usage

[Ramona Bolboaca]

Remove 'adis_initial_startup()' usage due to the fact that it leads to a 
deadlock.
The same mutex is acquired twice, without releasing it, once inside
'adis_initial_startup()' and once inside 'adis_enable_irq()'.
Instead of 'adis_initial_startup()', use '__adis_initial_startup()'.

Ramona Bolboaca (8):
  iio: accel: adis16201: Fix deadlock in probe
  iio: accel: adis16209: Fix deadlock in probe
  iio: gyro: adis16136: Fix deadlock in probe
  iio: gyro: adis16260: Fix deadlock in probe
  iio: imu: adis16400: Fix deadlock in probe
  staging: iio: accel: adis16203: Fix deadlock in probe
  staging: iio: accel: adis16240: Fix deadlock in probe
  iio: imu: adis: Remove adis_initial_startup function

 drivers/iio/accel/adis16201.c         |  2 +-
 drivers/iio/accel/adis16209.c         |  2 +-
 drivers/iio/gyro/adis16136.c          |  2 +-
 drivers/iio/gyro/adis16260.c          |  2 +-
 drivers/iio/imu/adis16400.c           |  2 +-
 drivers/staging/iio/accel/adis16203.c |  2 +-
 drivers/staging/iio/accel/adis16240.c |  2 +-
 include/linux/iio/imu/adis.h          | 12 ------------
 8 files changed, 7 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH v2 1/8] iio: accel: adis16201: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/iio/accel/adis16201.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
index dfb8e2e5bdf5..d054721859b3 100644
--- a/drivers/iio/accel/adis16201.c
+++ b/drivers/iio/accel/adis16201.c
@@ -281,7 +281,7 @@ static int adis16201_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = adis_initial_startup(st);
+	ret = __adis_initial_startup(st);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 2/8] iio: accel: adis16209: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/iio/accel/adis16209.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adis16209.c b/drivers/iio/accel/adis16209.c
index 5a9c6e2296f1..0035e4f4db63 100644
--- a/drivers/iio/accel/adis16209.c
+++ b/drivers/iio/accel/adis16209.c
@@ -291,7 +291,7 @@ static int adis16209_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = adis_initial_startup(st);
+	ret = __adis_initial_startup(st);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 3/8] iio: gyro: adis16136: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/iio/gyro/adis16136.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index 71295709f2b9..c95cf41be34b 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -429,7 +429,7 @@ static int adis16136_initial_setup(struct iio_dev *indio_dev)
 	uint16_t prod_id;
 	int ret;
 
-	ret = adis_initial_startup(&adis16136->adis);
+	ret = __adis_initial_startup(&adis16136->adis);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 4/8] iio: gyro: adis16260: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/iio/gyro/adis16260.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index eaf57bd339ed..112d635b7dfd 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -395,7 +395,7 @@ static int adis16260_probe(struct spi_device *spi)
 		return ret;
 
 	/* Get the device into a sane initial state */
-	ret = adis_initial_startup(&adis16260->adis);
+	ret = __adis_initial_startup(&adis16260->adis);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 5/8] iio: imu: adis16400: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/iio/imu/adis16400.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 17bb0c40a149..c02fc35dceb4 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -445,7 +445,7 @@ static int adis16400_initial_setup(struct iio_dev *indio_dev)
 	st->adis.spi->mode = SPI_MODE_3;
 	spi_setup(st->adis.spi);
 
-	ret = adis_initial_startup(&st->adis);
+	ret = __adis_initial_startup(&st->adis);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 6/8] staging: iio: accel: adis16203: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/staging/iio/accel/adis16203.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
index 62d5397ff1f9..c0e4c9266b5f 100644
--- a/drivers/staging/iio/accel/adis16203.c
+++ b/drivers/staging/iio/accel/adis16203.c
@@ -285,7 +285,7 @@ static int adis16203_probe(struct spi_device *spi)
 		return ret;
 
 	/* Get the device into a sane initial state */
-	ret = adis_initial_startup(st);
+	ret = __adis_initial_startup(st);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 7/8] staging: iio: accel: adis16240: Fix deadlock in probe

[Ramona Bolboaca]

Use '__adis_initial_startup()' instead of 'adis_initial_startup()' to
avoid deadlock.
When using 'adis_initial_startup()' 'mutex_lock()' is called twice,
without releasing it (once inside 'adis_initial_startup()' and
once inside 'adis_enable_irq()').

Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'")
Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - changed commit in 'Fixes' tag to the correct commit
 - added commas and brackets for functions in commit message
 drivers/staging/iio/accel/adis16240.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index bca857eef92e..337492785f04 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -414,7 +414,7 @@ static int adis16240_probe(struct spi_device *spi)
 		return ret;
 
 	/* Get the device into a sane initial state */
-	ret = adis_initial_startup(st);
+	ret = __adis_initial_startup(st);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 8/8] iio: imu: adis: Remove adis_initial_startup function

[Ramona Bolboaca]

Remove adis_initial_startup function since it is not used
anymore.

Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
 include/linux/iio/imu/adis.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 515ca09764fe..fa12a100b6b2 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -417,18 +417,6 @@ static inline int adis_check_status(struct adis *adis)
 	return ret;
 }
 
-/* locked version of __adis_initial_startup() */
-static inline int adis_initial_startup(struct adis *adis)
-{
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_initial_startup(adis);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
-}
-
 static inline void adis_dev_lock(struct adis *adis)
 {
 	mutex_lock(&adis->state_lock);
-- 
2.25.1

RE: [PATCH v2 0/8] Remove adis_initial_startup usage

[Sa, Nuno]

> From: Ramona Bolboaca <ramona.bolboaca@analog.com>
> Sent: Thursday, November 3, 2022 9:09 AM
> To: jic23@kernel.org; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Bolboaca, Ramona <Ramona.Bolboaca@analog.com>
> Subject: [PATCH v2 0/8] Remove adis_initial_startup usage
> 
> 
> Remove 'adis_initial_startup()' usage due to the fact that it leads to a
> deadlock.
> The same mutex is acquired twice, without releasing it, once inside
> 'adis_initial_startup()' and once inside 'adis_enable_irq()'.
> Instead of 'adis_initial_startup()', use '__adis_initial_startup()'.
> 
> Ramona Bolboaca (8):
>   iio: accel: adis16201: Fix deadlock in probe
>   iio: accel: adis16209: Fix deadlock in probe
>   iio: gyro: adis16136: Fix deadlock in probe
>   iio: gyro: adis16260: Fix deadlock in probe
>   iio: imu: adis16400: Fix deadlock in probe
>   staging: iio: accel: adis16203: Fix deadlock in probe
>   staging: iio: accel: adis16240: Fix deadlock in probe
>   iio: imu: adis: Remove adis_initial_startup function
> 
>  drivers/iio/accel/adis16201.c         |  2 +-
>  drivers/iio/accel/adis16209.c         |  2 +-
>  drivers/iio/gyro/adis16136.c          |  2 +-
>  drivers/iio/gyro/adis16260.c          |  2 +-
>  drivers/iio/imu/adis16400.c           |  2 +-
>  drivers/staging/iio/accel/adis16203.c |  2 +-
>  drivers/staging/iio/accel/adis16240.c |  2 +-
>  include/linux/iio/imu/adis.h          | 12 ------------
>  8 files changed, 7 insertions(+), 19 deletions(-)
> 

You could have placed your v2 changelog in the cover letter.
Moreover it's the same for all patches... Anyways: 

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

- Nuno Sá

Re: [PATCH v2 0/8] Remove adis_initial_startup usage

[Jonathan Cameron]

On Thu, 3 Nov 2022 12:35:31 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Ramona Bolboaca <ramona.bolboaca@analog.com>
> > Sent: Thursday, November 3, 2022 9:09 AM
> > To: jic23@kernel.org; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Cc: Bolboaca, Ramona <Ramona.Bolboaca@analog.com>
> > Subject: [PATCH v2 0/8] Remove adis_initial_startup usage
> > 
> > 
> > Remove 'adis_initial_startup()' usage due to the fact that it leads to a
> > deadlock.
> > The same mutex is acquired twice, without releasing it, once inside
> > 'adis_initial_startup()' and once inside 'adis_enable_irq()'.
> > Instead of 'adis_initial_startup()', use '__adis_initial_startup()'.
> > 
> > Ramona Bolboaca (8):
> >   iio: accel: adis16201: Fix deadlock in probe
> >   iio: accel: adis16209: Fix deadlock in probe
> >   iio: gyro: adis16136: Fix deadlock in probe
> >   iio: gyro: adis16260: Fix deadlock in probe
> >   iio: imu: adis16400: Fix deadlock in probe
> >   staging: iio: accel: adis16203: Fix deadlock in probe
> >   staging: iio: accel: adis16240: Fix deadlock in probe
> >   iio: imu: adis: Remove adis_initial_startup function
> > 
> >  drivers/iio/accel/adis16201.c         |  2 +-
> >  drivers/iio/accel/adis16209.c         |  2 +-
> >  drivers/iio/gyro/adis16136.c          |  2 +-
> >  drivers/iio/gyro/adis16260.c          |  2 +-
> >  drivers/iio/imu/adis16400.c           |  2 +-
> >  drivers/staging/iio/accel/adis16203.c |  2 +-
> >  drivers/staging/iio/accel/adis16240.c |  2 +-
> >  include/linux/iio/imu/adis.h          | 12 ------------
> >  8 files changed, 7 insertions(+), 19 deletions(-)
> >   
> 
> You could have placed your v2 changelog in the cover letter.
> Moreover it's the same for all patches... Anyways: 
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>

This feels a little backwards.  Normally we'd expect the
outer function to take the lock and the inner call to not
do so.  Now it's fine to not take the lock here at all because
the outer function call is in probe anyway, before we reach
the point where there should be an concurrency.

I wonder if we should instead do this by having
an unlocked __adis_enable_irq() that is always called
by __adis_initial_startup().  That would be the fix that
then needs backporting.

Switching the calls from adis_initial_startup() to
__adis_initial_startup() would then just be a trivial
optimization to not take locks before they should ever matter.

This all hinges on my assumption that the lock isn't useful.
Am I right on that?

Jonathan


> 
> - Nuno Sá

Re: [PATCH v2 0/8] Remove adis_initial_startup usage

[Nuno Sá]

On Sat, 2022-11-05 at 15:06 +0000, Jonathan Cameron wrote:
> On Thu, 3 Nov 2022 12:35:31 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: Ramona Bolboaca <ramona.bolboaca@analog.com>
> > > Sent: Thursday, November 3, 2022 9:09 AM
> > > To: jic23@kernel.org; linux-iio@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Cc: Bolboaca, Ramona <Ramona.Bolboaca@analog.com>
> > > Subject: [PATCH v2 0/8] Remove adis_initial_startup usage
> > > 
> > > 
> > > Remove 'adis_initial_startup()' usage due to the fact that it
> > > leads to a
> > > deadlock.
> > > The same mutex is acquired twice, without releasing it, once
> > > inside
> > > 'adis_initial_startup()' and once inside 'adis_enable_irq()'.
> > > Instead of 'adis_initial_startup()', use
> > > '__adis_initial_startup()'.
> > > 
> > > Ramona Bolboaca (8):
> > >   iio: accel: adis16201: Fix deadlock in probe
> > >   iio: accel: adis16209: Fix deadlock in probe
> > >   iio: gyro: adis16136: Fix deadlock in probe
> > >   iio: gyro: adis16260: Fix deadlock in probe
> > >   iio: imu: adis16400: Fix deadlock in probe
> > >   staging: iio: accel: adis16203: Fix deadlock in probe
> > >   staging: iio: accel: adis16240: Fix deadlock in probe
> > >   iio: imu: adis: Remove adis_initial_startup function
> > > 
> > >  drivers/iio/accel/adis16201.c         |  2 +-
> > >  drivers/iio/accel/adis16209.c         |  2 +-
> > >  drivers/iio/gyro/adis16136.c          |  2 +-
> > >  drivers/iio/gyro/adis16260.c          |  2 +-
> > >  drivers/iio/imu/adis16400.c           |  2 +-
> > >  drivers/staging/iio/accel/adis16203.c |  2 +-
> > >  drivers/staging/iio/accel/adis16240.c |  2 +-
> > >  include/linux/iio/imu/adis.h          | 12 ------------
> > >  8 files changed, 7 insertions(+), 19 deletions(-)
> > >   
> > 
> > You could have placed your v2 changelog in the cover letter.
> > Moreover it's the same for all patches... Anyways: 
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> This feels a little backwards.  Normally we'd expect the
> outer function to take the lock and the inner call to not
> do so.  Now it's fine to not take the lock here at all because
> the outer function call is in probe anyway, before we reach
> the point where there should be an concurrency.
> 
> I wonder if we should instead do this by having
> an unlocked __adis_enable_irq() that is always called
> by __adis_initial_startup().  That would be the fix that
> then needs backporting.
> 

I did mentioned the same thing in the first version of the series but
did not really pushed for it. Now that you mention, I agree it feels
weird (and wrong from a design perspective) to have the lock,
"silently", taken inside a function starting with double underscore
(which should mean unlocked call).

> Switching the calls from adis_initial_startup() to
> __adis_initial_startup() would then just be a trivial
> optimization to not take locks before they should ever matter.
> 
> This all hinges on my assumption that the lock isn't useful.
> Am I right on that?
> 

I think so as all the calls happen during probe before registering the
userspace interface.

- Nuno Sá