Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932438AbbKMAX7 (ORCPT ); Thu, 12 Nov 2015 19:23:59 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:55190 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753626AbbKMAX4 (ORCPT ); Thu, 12 Nov 2015 19:23:56 -0500 Subject: Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver To: Al Stone , Guenter Roeck , Fu Wei References: <1445961999-9506-1-git-send-email-fu.wei@linaro.org> <1445961999-9506-6-git-send-email-fu.wei@linaro.org> <563AE588.1080009@roeck-us.net> <563B5DF9.6080102@codeaurora.org> <563B62F7.3050307@codeaurora.org> <563B6A4B.7090400@codeaurora.org> <563B869F.2010004@roeck-us.net> <56452970.4070209@linaro.org> Cc: Pratyush Anand , devicetree@vger.kernel.org, linux-watchdog@vger.kernel.org, Arnd Bergmann , linux-doc@vger.kernel.org, Jon Masters , Linaro ACPI Mailman List , "Rafael J. Wysocki" , lkml , Will Deacon , Wim Van Sebroeck , Rob Herring , Catalin Marinas , Wei Fu , Jonathan Corbet , Dave Young , Vipul Gandhi From: Timur Tabi Message-ID: <56452D98.80704@codeaurora.org> Date: Thu, 12 Nov 2015 18:23:52 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56452970.4070209@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2618 Lines: 55 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/