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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
> From: Ramona Bolboaca <[email protected]>
> Sent: Thursday, November 3, 2022 9:09 AM
> To: [email protected]; [email protected]; linux-
> [email protected]
> Cc: Bolboaca, Ramona <[email protected]>
> 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? <[email protected]>
- Nuno S?
On Thu, 3 Nov 2022 12:35:31 +0000
"Sa, Nuno" <[email protected]> wrote:
> > From: Ramona Bolboaca <[email protected]>
> > Sent: Thursday, November 3, 2022 9:09 AM
> > To: [email protected]; [email protected]; linux-
> > [email protected]
> > Cc: Bolboaca, Ramona <[email protected]>
> > 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á <[email protected]>
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á
On Sat, 2022-11-05 at 15:06 +0000, Jonathan Cameron wrote:
> On Thu, 3 Nov 2022 12:35:31 +0000
> "Sa, Nuno" <[email protected]> wrote:
>
> > > From: Ramona Bolboaca <[email protected]>
> > > Sent: Thursday, November 3, 2022 9:09 AM
> > > To: [email protected]; [email protected]; linux-
> > > [email protected]
> > > Cc: Bolboaca, Ramona <[email protected]>
> > > 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á <[email protected]>
>
> 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á