From: Guenter Roeck <linux@roeck-us.net>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: Jean Delvare <jdelvare@suse.com>,
linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org
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: <de7682c2-ae34-c594-d237-330ea33cbc78@gmail.com>
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 <digetx@gmail.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 [1] says that reading the status register
> >> doesn't reset interrupt until temperature is returned back into normal
> >> state, which is what I'm witnessing.
> >>
> >> [1] 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[179]: /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[179]: /dev/kmsg buffer overrun, ...
> >>
> >> It's interesting that the very first version of the nct1008-support
> >> patch used edge-triggered interrupt flags [2].
> >>
> >> [2] 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 [PATCH v1] hwmon: (lm90) Use edge-triggered interrupt 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 \
--to=linux@roeck-us.net \
--cc=digetx@gmail.com \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).