| Message ID | 20230420102316.757-1-honda@mechatrax.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag | expand |
On Thu, 2023-04-20 at 19:23 +0900, Masahiro Honda wrote: > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt > line to indicate the completion of a conversion. However, some devices > cannot properly detect the completion of a conversion by an interrupt. > This is for the reason mentioned in the following commit. > > commit e9849777d0e2 ("genirq: Add flag to force mask in > disable_irq[_nosync]()") > > A read operation is performed by an extra interrupt before the completion > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY > flag. > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > --- LGTM: Reviewed-by: Nuno Sá <nuno.sa@analog.com
On Thu, 20 Apr 2023 19:23:16 +0900 Masahiro Honda <honda@mechatrax.com> wrote: > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt > line to indicate the completion of a conversion. However, some devices > cannot properly detect the completion of a conversion by an interrupt. > This is for the reason mentioned in the following commit. > > commit e9849777d0e2 ("genirq: Add flag to force mask in > disable_irq[_nosync]()") > > A read operation is performed by an extra interrupt before the completion > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY > flag. > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > --- > v3: > - Remove the Kconfig option. > v2: https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/ > - Rework commit message. > - Add a new entry in the Kconfig. > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ. > v1: https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/ > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > index d8570f620..215ecbedb 100644 > --- a/drivers/iio/adc/ad_sigma_delta.c > +++ b/drivers/iio/adc/ad_sigma_delta.c > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig) > } > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA); > > +static void ad_sd_free_irq(void *sd) > +{ > + struct ad_sigma_delta *sigma_delta = sd; > + > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > + free_irq(sigma_delta->spi->irq, sigma_delta); > +} Don't fuse the two operations unwinding like this. Just register a callback that only does the irq_clear_status_flags immediately after setting them. Then leave the orginally devm_request_irq call alone. If it fails, the devm cleanup will deal with the irq_clear_status_flag for you. It almost never makes sense for a single devm call to unwind more than one function call in a driver. Otherwise this looks fine to me, Thanks, Jonathan > + > static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev) > { > struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev); > @@ -584,11 +592,18 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de > init_completion(&sigma_delta->completion); > > sigma_delta->irq_dis = true; > - ret = devm_request_irq(dev, sigma_delta->spi->irq, > - ad_sd_data_rdy_trig_poll, > - sigma_delta->info->irq_flags | IRQF_NO_AUTOEN, > - indio_dev->name, > - sigma_delta); > + irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > + ret = request_irq(sigma_delta->spi->irq, > + ad_sd_data_rdy_trig_poll, > + sigma_delta->info->irq_flags | IRQF_NO_AUTOEN, > + indio_dev->name, > + sigma_delta); > + if (ret) { > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > + return ret; > + } > + > + ret = devm_add_action_or_reset(dev, ad_sd_free_irq, sigma_delta); > if (ret) > return ret; >
Hi Jonathan, On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote: > On Thu, 20 Apr 2023 19:23:16 +0900 > Masahiro Honda <honda@mechatrax.com> wrote: > > > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt > > line to indicate the completion of a conversion. However, some devices > > cannot properly detect the completion of a conversion by an interrupt. > > This is for the reason mentioned in the following commit. > > > > commit e9849777d0e2 ("genirq: Add flag to force mask in > > disable_irq[_nosync]()") > > > > A read operation is performed by an extra interrupt before the completion > > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY > > flag. > > > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > > --- > > v3: > > - Remove the Kconfig option. > > v2: > > https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/ > > - Rework commit message. > > - Add a new entry in the Kconfig. > > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the > > IRQ. > > v1: > > https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/ > > > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++----- > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c > > b/drivers/iio/adc/ad_sigma_delta.c > > index d8570f620..215ecbedb 100644 > > --- a/drivers/iio/adc/ad_sigma_delta.c > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, > > struct iio_trigger *trig) > > } > > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA); > > > > +static void ad_sd_free_irq(void *sd) > > +{ > > + struct ad_sigma_delta *sigma_delta = sd; > > + > > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > > + free_irq(sigma_delta->spi->irq, sigma_delta); > > +} > > Don't fuse the two operations unwinding like this. Just register a callback > that only > does the irq_clear_status_flags immediately after setting them. Then leave I was the one to propose fusing them together because I thought that we could have issues by clearing the flag after calling free_irq(). After looking again at the IRQ code, I can see that it is not up to free_irq() to free the allocated irq_descs (that might only happen when unmapping the virq) which means we should be fine doing the normal way. That said, looking at the only users that care to clear this flag, it looks like they do it before calling free_irq(). Hence, I'm not sure if there's anything subtle going on. In fact, looking at this line: https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909 I'm not so sure we actually need to clear the flag as for these devices, we should only have one consumer/action per IRQ. Anyways, probably for correctness we should still explicitly clear it? - Nuno Sá
Hi Jonathan, On Sun, Apr 23, 2023 at 7:59 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 20 Apr 2023 19:23:16 +0900 > Masahiro Honda <honda@mechatrax.com> wrote: > > > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt > > line to indicate the completion of a conversion. However, some devices > > cannot properly detect the completion of a conversion by an interrupt. > > This is for the reason mentioned in the following commit. > > > > commit e9849777d0e2 ("genirq: Add flag to force mask in > > disable_irq[_nosync]()") > > > > A read operation is performed by an extra interrupt before the completion > > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY > > flag. > > > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > > --- > > v3: > > - Remove the Kconfig option. > > v2: https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/ > > - Rework commit message. > > - Add a new entry in the Kconfig. > > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ. > > v1: https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/ > > > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++----- > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > > index d8570f620..215ecbedb 100644 > > --- a/drivers/iio/adc/ad_sigma_delta.c > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig) > > } > > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA); > > > > +static void ad_sd_free_irq(void *sd) > > +{ > > + struct ad_sigma_delta *sigma_delta = sd; > > + > > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > > + free_irq(sigma_delta->spi->irq, sigma_delta); > > +} > > Don't fuse the two operations unwinding like this. Just register a callback that only > does the irq_clear_status_flags immediately after setting them. Then leave > the orginally devm_request_irq call alone. If it fails, the devm cleanup will > deal with the irq_clear_status_flag for you. > > It almost never makes sense for a single devm call to unwind more than one function call in > a driver. > > Otherwise this looks fine to me, > > Thanks, > > Jonathan > I understand. I'll fix it. Thanks,
Hi Jonathan, On Wed, Apr 26, 2023 at 9:02 PM Masahiro Honda <honda@mechatrax.com> wrote: > > I understand. I'll fix it. > Sorry, I made a mistake. I would like to wait for a conclusion. Regards,
On Mon, 24 Apr 2023 11:09:34 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > Hi Jonathan, > > On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote: > > On Thu, 20 Apr 2023 19:23:16 +0900 > > Masahiro Honda <honda@mechatrax.com> wrote: > > > > > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt > > > line to indicate the completion of a conversion. However, some devices > > > cannot properly detect the completion of a conversion by an interrupt. > > > This is for the reason mentioned in the following commit. > > > > > > commit e9849777d0e2 ("genirq: Add flag to force mask in > > > disable_irq[_nosync]()") > > > > > > A read operation is performed by an extra interrupt before the completion > > > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY > > > flag. > > > > > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > > > --- > > > v3: > > > - Remove the Kconfig option. > > > v2: > > > https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/ > > > - Rework commit message. > > > - Add a new entry in the Kconfig. > > > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the > > > IRQ. > > > v1: > > > https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/ > > > > > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++----- > > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c > > > b/drivers/iio/adc/ad_sigma_delta.c > > > index d8570f620..215ecbedb 100644 > > > --- a/drivers/iio/adc/ad_sigma_delta.c > > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, > > > struct iio_trigger *trig) > > > } > > > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA); > > > > > > +static void ad_sd_free_irq(void *sd) > > > +{ > > > + struct ad_sigma_delta *sigma_delta = sd; > > > + > > > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > > > + free_irq(sigma_delta->spi->irq, sigma_delta); > > > +} > > > > Don't fuse the two operations unwinding like this. Just register a callback > > that only > > does the irq_clear_status_flags immediately after setting them. Then leave > > I was the one to propose fusing them together because I thought that we could > have issues by clearing the flag after calling free_irq(). After looking again > at the IRQ code, I can see that it is not up to free_irq() to free the allocated > irq_descs (that might only happen when unmapping the virq) which means we should > be fine doing the normal way. Ah. I'd missed the ordering. If that had been valid (and I think you are correct that it is not required) then a comment to make that clear would be necessary. Usual case of: When doing something non obvious with ordering, say why. > > That said, looking at the only users that care to clear this flag, it looks like > they do it before calling free_irq(). Hence, I'm not sure if there's anything > subtle going on. In fact, looking at this line: > > https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909 > > I'm not so sure we actually need to clear the flag as for these devices, we > should only have one consumer/action per IRQ. Anyways, probably for correctness > we should still explicitly clear it? Good question... Looks to me like a driver shouldn't be clearing this flag itself, but it's probably harmless in most cases. I'd drop the clear of the status flag, perhaps adding a comment that the irq core does it for us. Jonathan > > - Nuno Sá >
Hi all, On Mon, May 1, 2023 at 2:27 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 24 Apr 2023 11:09:34 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > Hi Jonathan, > > > > On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote: > > > On Thu, 20 Apr 2023 19:23:16 +0900 > > > Masahiro Honda <honda@mechatrax.com> wrote: > > > > > > > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt > > > > line to indicate the completion of a conversion. However, some devices > > > > cannot properly detect the completion of a conversion by an interrupt. > > > > This is for the reason mentioned in the following commit. > > > > > > > > commit e9849777d0e2 ("genirq: Add flag to force mask in > > > > disable_irq[_nosync]()") > > > > > > > > A read operation is performed by an extra interrupt before the completion > > > > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY > > > > flag. > > > > > > > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > > > > --- > > > > v3: > > > > - Remove the Kconfig option. > > > > v2: > > > > https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/ > > > > - Rework commit message. > > > > - Add a new entry in the Kconfig. > > > > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the > > > > IRQ. > > > > v1: > > > > https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/ > > > > > > > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++----- > > > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c > > > > b/drivers/iio/adc/ad_sigma_delta.c > > > > index d8570f620..215ecbedb 100644 > > > > --- a/drivers/iio/adc/ad_sigma_delta.c > > > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > > > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, > > > > struct iio_trigger *trig) > > > > } > > > > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA); > > > > > > > > +static void ad_sd_free_irq(void *sd) > > > > +{ > > > > + struct ad_sigma_delta *sigma_delta = sd; > > > > + > > > > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); > > > > + free_irq(sigma_delta->spi->irq, sigma_delta); > > > > +} > > > > > > Don't fuse the two operations unwinding like this. Just register a callback > > > that only > > > does the irq_clear_status_flags immediately after setting them. Then leave > > > > I was the one to propose fusing them together because I thought that we could > > have issues by clearing the flag after calling free_irq(). After looking again > > at the IRQ code, I can see that it is not up to free_irq() to free the allocated > > irq_descs (that might only happen when unmapping the virq) which means we should > > be fine doing the normal way. > > Ah. I'd missed the ordering. If that had been valid (and I think you are correct > that it is not required) then a comment to make that clear would be necessary. > > Usual case of: When doing something non obvious with ordering, say why. > > > > > That said, looking at the only users that care to clear this flag, it looks like > > they do it before calling free_irq(). Hence, I'm not sure if there's anything > > subtle going on. In fact, looking at this line: > > > > https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909 > > > > I'm not so sure we actually need to clear the flag as for these devices, we > > should only have one consumer/action per IRQ. Anyways, probably for correctness > > we should still explicitly clear it? > > Good question... Looks to me like a driver shouldn't be clearing this flag > itself, but it's probably harmless in most cases. > > I'd drop the clear of the status flag, perhaps adding a comment that > the irq core does it for us. > > Jonathan > > > > > > - Nuno Sá > > > I'll remove the callback and just call irq_set_status_flags() with a comment. Thanks, Masahiro
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index d8570f620..215ecbedb 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig) } EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA); +static void ad_sd_free_irq(void *sd) +{ + struct ad_sigma_delta *sigma_delta = sd; + + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); + free_irq(sigma_delta->spi->irq, sigma_delta); +} + static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev) { struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev); @@ -584,11 +592,18 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de init_completion(&sigma_delta->completion); sigma_delta->irq_dis = true; - ret = devm_request_irq(dev, sigma_delta->spi->irq, - ad_sd_data_rdy_trig_poll, - sigma_delta->info->irq_flags | IRQF_NO_AUTOEN, - indio_dev->name, - sigma_delta); + irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); + ret = request_irq(sigma_delta->spi->irq, + ad_sd_data_rdy_trig_poll, + sigma_delta->info->irq_flags | IRQF_NO_AUTOEN, + indio_dev->name, + sigma_delta); + if (ret) { + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); + return ret; + } + + ret = devm_add_action_or_reset(dev, ad_sd_free_irq, sigma_delta); if (ret) return ret;
The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt line to indicate the completion of a conversion. However, some devices cannot properly detect the completion of a conversion by an interrupt. This is for the reason mentioned in the following commit. commit e9849777d0e2 ("genirq: Add flag to force mask in disable_irq[_nosync]()") A read operation is performed by an extra interrupt before the completion of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY flag. Signed-off-by: Masahiro Honda <honda@mechatrax.com> --- v3: - Remove the Kconfig option. v2: https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/ - Rework commit message. - Add a new entry in the Kconfig. - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ. v1: https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/ drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)