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