Re: [PATCH v1] hwmon: (lm90) Use edge-triggered interrupt

[Dmitry Osipenko]

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

Limiting the interrupt rate could be an alternative solution.

What do you think about something like this:

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)