Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

From: Timur Tabi <timur@codeaurora.org>
To: Al Stone <al.stone@linaro.org>,
	Guenter Roeck <linux@roeck-us.net>, Fu Wei <fu.wei@linaro.org>
Cc: Pratyush Anand <panand@redhat.com>,
	devicetree@vger.kernel.org, linux-watchdog@vger.kernel.org,
	Arnd Bergmann <arnd@arndb.de>,
	linux-doc@vger.kernel.org, Jon Masters <jcm@redhat.com>,
	Linaro ACPI Mailman List <linaro-acpi@lists.linaro.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	lkml <linux-kernel@vger.kernel.org>,
	Will Deacon <will.deacon@arm.com>,
	Wim Van Sebroeck <wim@iguana.be>,
	Rob Herring <robherring2@gmail.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Wei Fu <tekkamanninja@gmail.com>,
	Jonathan Corbet <corbet@lwn.net>, Dave Young <dyoung@redhat.com>,
	Vipul Gandhi <vgandhi@codeaurora.org>
Subject: Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver
Date: Thu, 12 Nov 2015 18:23:52 -0600	[thread overview]
Message-ID: <56452D98.80704@codeaurora.org> (raw)
In-Reply-To: <56452970.4070209@linaro.org>

On 11/12/2015 06:06 PM, Al Stone wrote:
> If it is a NAK, that's fine, but I also want to be sure I understand what the
> objections are.  Based on my understanding of the discussion so far over the
> multiple versions, I think the primary objection is that the use of pretimeout
> makes this driver too complex, and indeed complex enough that there is some
> concern that it could destabilize a running system.  Do I have that right?

I don't have a problem with the concept of pre-timeout per se.  My 
primary objection is this code:

 > +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
 > +{
 > +       struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
 > +       struct watchdog_device *wdd = &gwdt->wdd;
 > +
 > +       /* We don't use pretimeout, trigger WS1 now */
 > +       if (!wdd->pretimeout)
 > +               sbsa_gwdt_set_wcv(wdd, 0);

This driver depends on an interrupt handler in order to properly program 
the hardware.  Unlike some other devices, the SBSA watchdog does not 
need assistance to reset on a timeout -- it is a "fire and forget" 
device.  What happens if there is a hard lockup, and interrupts no 
longer work?

The reason why Fu does this is because he wants to support a pre-timeout 
value that's independent of the timeout value.  The SBSA watchdog is 
normally programmed where real timeout equals twice the pre-timeout.  I 
would prefer that the driver adhere to this limitation.  That would 
eliminate the need to pre-program the hardware in the interrupt handler.

> And finally, a simpler, single stage timeout watchdog driver would be a
> reasonable thing to accept, yes?  I can see where that would make sense.

I would be okay with merging such a driver, and then enhancing it later 
to add pre-timeout support.

> The issue for me in that case is that the SBSA requires a two stage timeout,
> so a single stage driver has no real value for me.

There are plenty of existing watchdog devices that have a two-stage 
timeout but the driver treats it as a single stage.  The PowerPC 
watchdog driver is like that.  The hardware is programmed for the second 
stage to cause a hardware reset, and the interrupt handler is typically 
a no-op or just a printk().

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.