| Message ID | 20221122082757.449452-2-ramona.bolboaca@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add '__adis_enable_irq()' | expand |
On Tue, 22 Nov 2022 10:27:49 +0200 Ramona Bolboaca <ramona.bolboaca@analog.com> wrote: > Add '__adis_enable_irq()' implementation which is the unlocked > version of 'adis_enable_irq()'. > Call '__adis_enable_irq()' instead of 'adis_enable_irq()' from > '__adis_intial_startup()' to keep the expected unlocked functionality. > > This fix is needed to remove a deadlock for all devices which are > using 'adis_initial_startup()'. The deadlock occurs because the > same mutex is acquired twice, without releasing it. > The mutex is acquired once inside 'adis_initial_startup()', before > calling '__adis_initial_startup()', and once inside > 'adis_enable_irq()', which is called by '__adis_initial_startup()'. > The deadlock is removed by calling '__adis_enable_irq()', instead of > 'adis_enable_irq()' from within '__adis_initial_startup()'. > > Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'") > Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com> > Reviewed-by: Nuno Sá <nuno.sa@analog.com> Build issue in here that I've fixed up. > --- > drivers/iio/imu/adis.c | 28 ++++++++++------------------ > include/linux/iio/imu/adis.h | 13 ++++++++++++- > 2 files changed, 22 insertions(+), 19 deletions(-) > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > index f7fcfd04f659..bc40240b29e2 100644 > --- a/drivers/iio/imu/adis.c > +++ b/drivers/iio/imu/adis.c > @@ -270,23 +270,19 @@ EXPORT_SYMBOL_NS(adis_debugfs_reg_access, IIO_ADISLIB); > #endif > > /** > - * adis_enable_irq() - Enable or disable data ready IRQ > + * __adis_enable_irq() - Enable or disable data ready IRQ (unlocked) > * @adis: The adis device > * @enable: Whether to enable the IRQ > * > * Returns 0 on success, negative error code otherwise > */ > -int adis_enable_irq(struct adis *adis, bool enable) > +int __adis_enable_irq(struct adis *adis, bool enable) > { > - int ret = 0; > + int ret; > u16 msc; > > - mutex_lock(&adis->state_lock); > - > - if (adis->data->enable_irq) { > - ret = adis->data->enable_irq(adis, enable); > - goto out_unlock; > - } > + if (adis->data->enable_irq) > + return adis->data->enable_irq(adis, enable); > > if (adis->data->unmasked_drdy) { > if (enable) > @@ -294,12 +290,12 @@ int adis_enable_irq(struct adis *adis, bool enable) > else > disable_irq(adis->spi->irq); > > - goto out_unlock; > + return 0; > } > > ret = __adis_read_reg_16(adis, adis->data->msc_ctrl_reg, &msc); > if (ret) > - goto out_unlock; > + return ret; > > msc |= ADIS_MSC_CTRL_DATA_RDY_POL_HIGH; > msc &= ~ADIS_MSC_CTRL_DATA_RDY_DIO2; > @@ -308,13 +304,9 @@ 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); > - > -out_unlock: > - mutex_unlock(&adis->state_lock); > - return ret; > + return __adis_write_reg_16(adis, adis->data->msc_ctrl_reg, msc); > } > -EXPORT_SYMBOL_NS(adis_enable_irq, IIO_ADISLIB); > +EXPORT_SYMBOL_NS(__adis_enable_irq, IIO_ADISLIB); > > /** > * __adis_check_status() - Check the device for error conditions (unlocked) > @@ -445,7 +437,7 @@ int __adis_initial_startup(struct adis *adis) > * with 'IRQF_NO_AUTOEN' anyways. > */ > if (!adis->data->unmasked_drdy) > - adis_enable_irq(adis, false); > + __adis_enable_irq(adis, false); > > if (!adis->data->prod_id_reg) > return 0; > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > index 515ca09764fe..d789ecf8d0c8 100644 > --- a/include/linux/iio/imu/adis.h > +++ b/include/linux/iio/imu/adis.h > @@ -402,9 +402,20 @@ static inline int adis_update_bits_base(struct adis *adis, unsigned int reg, > __adis_update_bits_base(adis, reg, mask, val, sizeof(val)); \ > }) > > -int adis_enable_irq(struct adis *adis, bool enable); > +static inline int adis_enable_irq(struct adis *adis, bool enable) > +{ > + int ret; > + > + mutex_lock(&adis->state_lock); > + ret = __adis_enable_irq(adis, enable); > + mutex_unlock(&adis->state_lock); > + > + return ret; > +} > + > int __adis_check_status(struct adis *adis); > int __adis_initial_startup(struct adis *adis); > +int __adis_enable_irq(struct adis *adis, bool enable); Move the above down here seeing as __adis_enable_irq() isn't defined until this point (and that gave a bunch of build issues). > > static inline int adis_check_status(struct adis *adis) > {
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c index f7fcfd04f659..bc40240b29e2 100644 --- a/drivers/iio/imu/adis.c +++ b/drivers/iio/imu/adis.c @@ -270,23 +270,19 @@ EXPORT_SYMBOL_NS(adis_debugfs_reg_access, IIO_ADISLIB); #endif /** - * adis_enable_irq() - Enable or disable data ready IRQ + * __adis_enable_irq() - Enable or disable data ready IRQ (unlocked) * @adis: The adis device * @enable: Whether to enable the IRQ * * Returns 0 on success, negative error code otherwise */ -int adis_enable_irq(struct adis *adis, bool enable) +int __adis_enable_irq(struct adis *adis, bool enable) { - int ret = 0; + int ret; u16 msc; - mutex_lock(&adis->state_lock); - - if (adis->data->enable_irq) { - ret = adis->data->enable_irq(adis, enable); - goto out_unlock; - } + if (adis->data->enable_irq) + return adis->data->enable_irq(adis, enable); if (adis->data->unmasked_drdy) { if (enable) @@ -294,12 +290,12 @@ int adis_enable_irq(struct adis *adis, bool enable) else disable_irq(adis->spi->irq); - goto out_unlock; + return 0; } ret = __adis_read_reg_16(adis, adis->data->msc_ctrl_reg, &msc); if (ret) - goto out_unlock; + return ret; msc |= ADIS_MSC_CTRL_DATA_RDY_POL_HIGH; msc &= ~ADIS_MSC_CTRL_DATA_RDY_DIO2; @@ -308,13 +304,9 @@ 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); - -out_unlock: - mutex_unlock(&adis->state_lock); - return ret; + return __adis_write_reg_16(adis, adis->data->msc_ctrl_reg, msc); } -EXPORT_SYMBOL_NS(adis_enable_irq, IIO_ADISLIB); +EXPORT_SYMBOL_NS(__adis_enable_irq, IIO_ADISLIB); /** * __adis_check_status() - Check the device for error conditions (unlocked) @@ -445,7 +437,7 @@ int __adis_initial_startup(struct adis *adis) * with 'IRQF_NO_AUTOEN' anyways. */ if (!adis->data->unmasked_drdy) - adis_enable_irq(adis, false); + __adis_enable_irq(adis, false); if (!adis->data->prod_id_reg) return 0; diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h index 515ca09764fe..d789ecf8d0c8 100644 --- a/include/linux/iio/imu/adis.h +++ b/include/linux/iio/imu/adis.h @@ -402,9 +402,20 @@ static inline int adis_update_bits_base(struct adis *adis, unsigned int reg, __adis_update_bits_base(adis, reg, mask, val, sizeof(val)); \ }) -int adis_enable_irq(struct adis *adis, bool enable); +static inline int adis_enable_irq(struct adis *adis, bool enable) +{ + int ret; + + mutex_lock(&adis->state_lock); + ret = __adis_enable_irq(adis, enable); + mutex_unlock(&adis->state_lock); + + return ret; +} + int __adis_check_status(struct adis *adis); int __adis_initial_startup(struct adis *adis); +int __adis_enable_irq(struct adis *adis, bool enable); static inline int adis_check_status(struct adis *adis) {
Add '__adis_enable_irq()' implementation which is the unlocked version of 'adis_enable_irq()'. Call '__adis_enable_irq()' instead of 'adis_enable_irq()' from '__adis_intial_startup()' to keep the expected unlocked functionality. This fix is needed to remove a deadlock for all devices which are using 'adis_initial_startup()'. The deadlock occurs because the same mutex is acquired twice, without releasing it. The mutex is acquired once inside 'adis_initial_startup()', before calling '__adis_initial_startup()', and once inside 'adis_enable_irq()', which is called by '__adis_initial_startup()'. The deadlock is removed by calling '__adis_enable_irq()', instead of 'adis_enable_irq()' from within '__adis_initial_startup()'. Fixes: b600bd7eb3335 ("iio: adis: do not disabe IRQs in 'adis_init()'") Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com> Reviewed-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/imu/adis.c | 28 ++++++++++------------------ include/linux/iio/imu/adis.h | 13 ++++++++++++- 2 files changed, 22 insertions(+), 19 deletions(-)