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@xxxxxxxxx>
> >> ---
> >> 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.
> Limiting the interrupt rate could be an alternative solution.
>
> What do you think about something like this:
>
A sleep in an interrupt handler to "prevent" an interrupt storm
is never acceptable.
Guenter
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index ce8ebe60fcdc..74886b8066ab 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -79,6 +79,7 @@
> * concern all supported chipsets, unless mentioned otherwise.
> */
>
> +#include <linux/delay.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/slab.h>
> @@ -201,6 +202,9 @@ enum chips { lm90, adm1032, lm99, lm86, max6657,
> max6659, adt7461, max6680,
> #define MAX6696_STATUS2_R2OT2 (1 << 6) /* remote2 emergency limit
> tripped */
> #define MAX6696_STATUS2_LOT2 (1 << 7) /* local emergency limit tripped */
>
> +/* Prevent instant interrupt re-triggering */
> +#define LM90_IRQ_DELAY (15 * MSEC_PER_SEC)
> +
> /*
> * Driver data (common to all clients)
> */
> @@ -1756,10 +1760,12 @@ static irqreturn_t lm90_irq_thread(int irq, void
> *dev_id)
> struct i2c_client *client = dev_id;
> u16 status;
>
> - if (lm90_is_tripped(client, &status))
> - return IRQ_HANDLED;
> - else
> + if (!lm90_is_tripped(client, &status))
> return IRQ_NONE;
> +
> + msleep(LM90_IRQ_DELAY);
> +
> + return IRQ_HANDLED;
> }
>
> static void lm90_remove_pec(void *dev)
