From: Guenter Roeck <email@example.com> To: Dmitry Osipenko <firstname.lastname@example.org> Cc: Jean Delvare <email@example.com>, firstname.lastname@example.org, email@example.com Subject: Re: [PATCH v1] hwmon: (lm90) Use edge-triggered interrupt Date: Thu, 17 Jun 2021 07:13:00 -0700 [thread overview] Message-ID: <20210617141300.GA1366442@roeck-us.net> (raw) In-Reply-To: <firstname.lastname@example.org> On Thu, Jun 17, 2021 at 04:48:08PM +0300, Dmitry Osipenko wrote: > 17.06.2021 16:12, Guenter Roeck пишет: > > On Thu, Jun 17, 2021 at 10:11:19AM +0300, Dmitry Osipenko wrote: > >> 17.06.2021 03:12, Guenter Roeck пишет: > >>> On Wed, Jun 16, 2021 at 10:07:08PM +0300, Dmitry Osipenko wrote: > >>>> The LM90 driver uses level-based interrupt triggering. The interrupt > >>>> handler prints a warning message about the breached temperature and > >>>> quits. There is no way to stop interrupt from re-triggering since it's > >>>> level-based, thus thousands of warning messages are printed per second > >>>> once interrupt is triggered. Use edge-triggered interrupt in order to > >>>> fix this trouble. > >>>> > >>>> Fixes: 109b1283fb532 ("hwmon: (lm90) Add support to handle IRQ") > >>>> Signed-off-by: Dmitry Osipenko <email@example.com> > >>>> --- > >>>> drivers/hwmon/lm90.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > >>>> index ebbfd5f352c0..ce8ebe60fcdc 100644 > >>>> --- a/drivers/hwmon/lm90.c > >>>> +++ b/drivers/hwmon/lm90.c > >>>> @@ -1908,7 +1908,7 @@ static int lm90_probe(struct i2c_client *client) > >>>> dev_dbg(dev, "IRQ: %d\n", client->irq); > >>>> err = devm_request_threaded_irq(dev, client->irq, > >>>> NULL, lm90_irq_thread, > >>>> - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > >>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >>>> "lm90", client); > >>> > >>> We can't do that. Problem is that many of the devices supported by this driver > >>> behave differently when it comes to interrupts. Specifically, the interrupt > >>> handler is supposed to reset the interrupt condition (ie reading the status > >>> register should reset it). If that is the not the case for a specific chip, > >>> we'll have to update the code to address the problem for that specific chip. > >>> The above code would probably just generate a single interrupt while never > >>> resetting the interrupt condition, which is obviously not what we want to > >>> happen. > >> > >> The nct1008/72 datasheet  says that reading the status register > >> doesn't reset interrupt until temperature is returned back into normal > >> state, which is what I'm witnessing. > >> > >>  https://www.onsemi.com/pdf/datasheet/nct1008-d.pdf > >> > >> Page 10 "Status Register": > >> > >> "Reading the status register clears the five flags, Bit 6 to Bit 2, > >> provided the error conditions causing the flags to beset have gone > >> away. A flag bit can be reset only if the corresponding > >> value register contains an in-limit measurement or if the > >> sensor is good." > >> > >> So the interrupt handler doesn't actually stop interrupt from > >> reoccurring and the whole KMSG is instantly spammed with: > >> > >> ... > >> [ 217.484034] lm90 0-004c: temp2 out of range, please check! > >> [ 217.484569] lm90 0-004c: temp2 out of range, please check! > >> [ 217.485006] systemd-journald: /dev/kmsg buffer overrun, some > >> messages lost. > >> [ 217.485109] lm90 0-004c: temp2 out of range, please check! > >> [ 217.485699] lm90 0-004c: temp2 out of range, please check! > >> [ 217.486235] lm90 0-004c: temp2 out of range, please check! > >> [ 217.486776] lm90 0-004c: temp2 out of range, please check! > >> [ 217.486874] systemd-journald: /dev/kmsg buffer overrun, ... > >> > >> It's interesting that the very first version of the nct1008-support > >> patch used edge-triggered interrupt flags . > >> > >>  http://lkml.iu.edu/hypermail/linux/kernel/1104.1/01669.html > >> > > A lot of this depends on the chip and its wiring, as well as on chip > > configuration. Even for a specific chip there may be configuration > > dependencies. The interrupt configuration in situations like this > > should really be determined by devicetree configuration, and not > > be hardcoded. Is this a devicetree based system ? If so, there should > > be an entry for this chip pointing to the interrupt, and that entry > > should include a trigger mask. That mask should be set to edge > > triggered. > > This is a device-tree based system, in particular it's NVIDIA Tegra30 > Nexus 7. The interrupt support was originally added to the lm90 driver > by Wei Ni who works at NVIDIA and did it for the Tegra boards. The Tegra > device-trees are specifying the trigger mask and apparently they all are > cargo-culted and wrong because they use IRQ_TYPE_LEVEL_HIGH, while it Be fair, no one is perfect. > should be IRQ_TYPE_EDGE_FALLING. It should probably be both IRQ_TYPE_EDGE_FALLING and IRQ_TYPE_EDGE_RISING, and the interrupt handler should call hwmon_notify_event() instead of clogging the kernel log, but that should be done in a separate patch. Anyway, the tegra30 dts files in the upstream kernel either use IRQ_TYPE_LEVEL_LOW or no interrupts for nct1008. The Nexus 7 dts file in the upstream kernel has no interrupt configured (and coincidentally it was you who added that entry). Where do you see IRQ_TYPE_LEVEL_HIGH ? > > The IRQF flag in devm_request_threaded_irq() overrides the trigger mask > specified in a device-tree. IIUC, the interrupt is used only by OF-based > devices, hence I think we could simply remove the IRQF flag from the > code and fix the device-trees. Does it sound good to you? Yes, that is a better approach. Thanks, Guenter
next prev parent reply other threads:[~2021-06-17 14:13 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-16 19:07 Dmitry Osipenko 2021-06-17 0:12 ` Guenter Roeck 2021-06-17 7:11 ` Dmitry Osipenko 2021-06-17 13:12 ` Guenter Roeck 2021-06-17 13:48 ` Dmitry Osipenko 2021-06-17 14:13 ` Guenter Roeck [this message] 2021-06-17 14:46 ` Dmitry Osipenko 2021-06-17 15:12 ` Guenter Roeck 2021-06-17 15:27 ` Dmitry Osipenko 2021-06-17 21:42 ` Guenter Roeck 2021-06-18 8:55 ` Dmitry Osipenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210617141300.GA1366442@roeck-us.net \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v1] hwmon: (lm90) Use edge-triggered interrupt' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).