On Thu, Feb 04, 2016 at 07:28:02PM -0600, Kyle Roeschley wrote: [..] > diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c > index 13f5c10..1fce4d0 100644 > --- a/drivers/watchdog/ni9x3x_wdt.c > +++ b/drivers/watchdog/ni9x3x_wdt.c > @@ -15,6 +15,7 @@ > #include <linux/acpi.h> > #include <linux/interrupt.h> > #include <linux/module.h> > +#include <linux/sysfs.h> > #include <linux/watchdog.h> > > #define NIWD_CONTROL 0x01 > @@ -28,6 +29,7 @@ > #define NIWD_IO_SIZE 0x08 > > #define NIWD_CONTROL_MODE 0x80 > +#define NIWD_CONTROL_PROC_INTERRUPT 0x40 > #define NIWD_CONTROL_PROC_RESET 0x20 > #define NIWD_CONTROL_PET 0x10 > #define NIWD_CONTROL_RUNNING 0x08 > @@ -48,6 +50,7 @@ struct ni9x3x_wdt { > struct acpi_device *acpi_device; > u16 io_base; > u16 io_size; > + u32 irq; Interrupts have type 'int'. > spinlock_t lock; > struct watchdog_device wdog; > }; > @@ -101,6 +104,34 @@ static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd) > return (unsigned int)((counter * (u64)NIWD_PERIOD_NS) / 1000000000); > } > > +static irqreturn_t ni9x3x_wdt_irq_handler(int irq, void *data) > +{ > + struct ni9x3x_wdt *wdt = data; > + irqreturn_t ret = IRQ_NONE; > + u8 control; > + unsigned long flags; > + > + spin_lock_irqsave(&wdt->lock, flags); Interrupts are disabled already, so the _irqsave()/_irqrestore() is unnecessary. > + > + control = inb(wdt->io_base + NIWD_CONTROL); > + > + if (!(NIWD_CONTROL_ALARM & control)) { > + dev_err(&wdt->acpi_device->dev, > + "Spurious watchdog interrupt, 0x%02X\n", control); No need to print anything here, just return IRQ_NONE. The irq core will print a much better (and rate-limited) error when/if this occurs. > + goto out_unlock; > + } > + > + /* Acknowledge the interrupt. */ > + outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL); > + > + ret = IRQ_HANDLED; > + > +out_unlock: > + spin_unlock_irqrestore(&wdt->lock, flags); > + > + return ret; > +} This is...strange. You're allowing a user to enable an interrupt reset action, but...there is no way that a user can actually _do_ anything when that action occurs. I'm reminded of one of these: https://www.youtube.com/watch?v=aqAUmgE3WyM (The only reason I can think of where this would be a legitimate thing to do was if we were relying on the watchdog interrupt to bring the CPU out of a low power state...but AFAIK that's not something we do). Josh
Attachment:
signature.asc
Description: PGP signature