| Message ID | 20230306044737.862-1-honda@mechatrax.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag | expand |
On Mon, 2023-03-06 at 13:47 +0900, Masahiro Honda wrote: > ADC using ad7793.ko, such as AD7794, may read incorrect data. > Extra interrupt is pending if the data on DOUT contains a falling > edge. > Therefore, wait_for_completion_timeout returns immediately. > This patch fixes the issue by setting IRQ_DISABLE_UNLAZY flag. > > Signed-off-by: Masahiro Honda <honda@mechatrax.com> > --- > drivers/iio/adc/ad_sigma_delta.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iio/adc/ad_sigma_delta.c > b/drivers/iio/adc/ad_sigma_delta.c > index d8570f620..364051809 100644 > --- a/drivers/iio/adc/ad_sigma_delta.c > +++ b/drivers/iio/adc/ad_sigma_delta.c > @@ -584,6 +584,7 @@ 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; > + irq_set_status_flags(sigma_delta->spi->irq, > IRQ_DISABLE_UNLAZY); Hmmm this looks to be a very likely issue for any device having to brute force IRQ disabling by disabling the line (disable_irq()). That said, I think the commit message can (needs) to be improved. The message feels a bit confusing to me. Also, having a reference to commit e9849777d0e2 ("genirq: Add flag to force mask in disable_irq[_nosync]()") would be nice to give some background on why this can be an issue. Another thing that came to my mind is if the data is totally garbage/wrong or is it just some outstanding sample? Some research on this also seems to point that we should (need?) call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ. I also wonder if we should keep this out of the library and have a per device config? Sure, this won't make anything wrong but it will hurt performance. OTOH, even though no one else ever reported this before, it looks like this can be an issue for all of the supported sigma delta ADCs. - Nuno Sá
On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > Hmmm this looks to be a very likely issue for any device having to > brute force IRQ disabling by disabling the line (disable_irq()). That > said, I think the commit message can (needs) to be improved. The > message feels a bit confusing to me. Also, having a reference to > > commit e9849777d0e2 ("genirq: Add flag to force mask in > disable_irq[_nosync]()") > > would be nice to give some background on why this can be an issue. > > Another thing that came to my mind is if the data is totally > garbage/wrong or is it just some outstanding sample? > > > Some research on this also seems to point that we should (need?) call > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ. > Thank you for your reply. I'll improve it. > I also wonder if we should keep this out of the library and have a per > device config? Sure, this won't make anything wrong but it will hurt > performance. OTOH, even though no one else ever reported this before, > it looks like this can be an issue for all of the supported sigma delta > ADCs. > I'll also investigate it.
Hi, Nuno > On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > Another thing that came to my mind is if the data is totally > > garbage/wrong or is it just some outstanding sample? I'm sorry for not answering your question. The data is the same as the previous conversion even if the input voltage is changed. At this time, a logic analyzer shows that the read operation is performed immediately after the conversion is performed. The read operation returns the previous conversion result because it is performed before the completion of the conversion. > > Some research on this also seems to point that we should (need?) call > > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ. I have understood that I need to call irq_clear_status_flags. However, I cannot find a code to free the IRQ in ad_sigma_delta.c and other Sigma-Delta ADC driver source files. So, I would like to implement only irq_set_status_flags.
On Mon, 2023-03-27 at 18:01 +0900, Masahiro Honda wrote: Hi Masahiro, Thanks for looking in more detail into this... > Hi, Nuno > > > On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <noname.nuno@gmail.com> > > wrote: > > > Another thing that came to my mind is if the data is totally > > > garbage/wrong or is it just some outstanding sample? > > I'm sorry for not answering your question. The data is the same as > the previous conversion even if the input voltage is changed. At this > time, a logic analyzer shows that the read operation is performed > immediately after the conversion is performed. The read operation > returns the previous conversion result because it is performed before > the completion of the conversion. > So, my suspicion was right... We are getting stalled data (which is obviously not good). AFAIU, when disabling the IRQ, we don't immediately mask the IRQ and we only do it in the next time an interrupt (sample) comes which means (I think) we'll process (right away) that outstanding interrupt next time we enable the IRQ. > > > Some research on this also seems to point that we should (need?) > > > call > > > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the > > > IRQ. > > I have understood that I need to call irq_clear_status_flags. > However, > I cannot find a code to free the IRQ in ad_sigma_delta.c and other > Sigma-Delta ADC driver source files. So, I would like to implement > only irq_set_status_flags. Well, that's because we are using devm_request_irq() which is a device managed API. So, I can see two options in here... 1) You do not use devm_request_irq() and use request_irq() + devm_add_action_or_reset() and in your release() function you would call irq_clear_status_flags() + free_irq(). 2) You add a devm_add_action_or_reset() after devm_request_irq() and your release() function would only clear the flag. But in here we would likely also have to be careful in the case where devm_request_irq() fails. So option 2) seems a bit more "ugly". I would likely go to option 1) but maybe Jonathan or others have better ideas. - Nuno Sá
On Mon, Mar 27, 2023 at 9:26 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > So, my suspicion was right... We are getting stalled data (which is > obviously not good). AFAIU, when disabling the IRQ, we don't > immediately mask the IRQ and we only do it in the next time an > interrupt (sample) comes which means (I think) we'll process (right > away) that outstanding interrupt next time we enable the IRQ. > Thank you. I understand. > > > > Some research on this also seems to point that we should (need?) > > > > call > > > > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the > > > > IRQ. > > > > I have understood that I need to call irq_clear_status_flags. > > However, > > I cannot find a code to free the IRQ in ad_sigma_delta.c and other > > Sigma-Delta ADC driver source files. So, I would like to implement > > only irq_set_status_flags. > > Well, that's because we are using devm_request_irq() which is a device > managed API. So, I can see two options in here... > > 1) You do not use devm_request_irq() and use request_irq() + > devm_add_action_or_reset() and in your release() function you would > call irq_clear_status_flags() + free_irq(). > > 2) You add a devm_add_action_or_reset() after devm_request_irq() and > your release() function would only clear the flag. But in here we would > likely also have to be careful in the case where devm_request_irq() > fails. So option 2) seems a bit more "ugly". > > I would likely go to option 1) but maybe Jonathan or others have better > ideas. > Thank you very much for letting me know about the API and ideas. I'll try to implement 1).
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index d8570f620..364051809 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -584,6 +584,7 @@ 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; + irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY); ret = devm_request_irq(dev, sigma_delta->spi->irq, ad_sd_data_rdy_trig_poll, sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
ADC using ad7793.ko, such as AD7794, may read incorrect data. Extra interrupt is pending if the data on DOUT contains a falling edge. Therefore, wait_for_completion_timeout returns immediately. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY flag. Signed-off-by: Masahiro Honda <honda@mechatrax.com> --- drivers/iio/adc/ad_sigma_delta.c | 1 + 1 file changed, 1 insertion(+)