From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from [85.17.2.162] ([85.17.2.162]:39696 "EHLO spo001.leaseweb.nl" rhost-flags-FAIL-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753888AbcJGHoN (ORCPT ); Fri, 7 Oct 2016 03:44:13 -0400 Date: Fri, 7 Oct 2016 09:24:57 +0200 From: Wim Van Sebroeck To: Vladimir Zapolskiy Cc: Guenter Roeck , Wolfram Sang , linux-watchdog@vger.kernel.org Subject: Re: [PATCH v5 00/10] watchdog: add watchdog pretimeout framework Message-ID: <20161007072457.GA11426@spo001.leaseweb.nl> References: <1472644370-16982-1-git-send-email-vladimir_zapolskiy@mentor.com> <20160928084921.GA15636@spo001.leaseweb.nl> <1c167827-8d73-f25f-61a4-38598e0fa8ec@mentor.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org Hi Vladimir, > On 09/28/2016 02:34 PM, Vladimir Zapolskiy wrote: > >Hello Wim, > > > >On 09/28/2016 11:49 AM, Wim Van Sebroeck wrote: > >>Hi All, > >> > >>>The change adds a simple watchdog pretimeout framework infrastructure, > >>>its purpose is to allow users to select a desired handling of watchdog > >>>pretimeout events, which may be generated by a watchdog driver. > >>> > >>>The idea of adding this kind of a framework appeared after reviewing > >>>several attempts to add hardcoded pretimeout event handling to some > >>>watchdog driver and after a discussion with Guenter, see > >>>https://lkml.org/lkml/2015/11/4/346 > >>> > >>>Watchdogs with WDIOF_PRETIMEOUT capability now may have three device > >>>attributes in sysfs: read only pretimeout value attribute, read/write > >>>pretimeout_governor attribute, read only pretimeout_available_governors > >>>attribute. > >>> > >>>To throw a pretimeout event for further processing a watchdog driver > >>>should call exported watchdog_notify_pretimeout(wdd) interface. > >>> > >>>In addition to the framework two simple watchdog pretimeout governors > >>>are added for review: panic and noop. > >> > >>I still have 2 questions about this series: > >>1) Patch 3: > >>+/* Use the following functions to report watchdog pretimeout event */ > >>+#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV) > >>+void watchdog_notify_pretimeout(struct watchdog_device *wdd); > >>+#else > >>+static inline void watchdog_notify_pretimeout(struct watchdog_device > >>*wdd) > >>+{ > >>+ panic("watchdog pretimeout event\n"); > >>+} > >>+#endif > >>+ > >> > >>Why a panic here? Why not just a printk like in the noop pretimeout > >>governor? > >>We now also don't do a panic, which means that changing this to a panic > >>means that we change the default behaviour... > >> > >>2) Why did we choose the panic pretimeout governer as the default and not > >>the noop governor? > >>This is basically the same question/remark as my previous question. noop > >>is more the old behaviour, panic is a change in regards to the > >>old/current behaviour... > >> > > > >regarding a selection of panic as a default action I had a discussion with > >Guenter about it right in the beginnig of the development (v1 of the series > >introduced printk by default, v2 and all later series select panic), please > >follow the link to it (ctrl-f panic): > > > > http://www.spinics.net/lists/linux-watchdog/msg07955.html > > > >I generally don't have a strong preference, because it is configurable both > >during build time and runtime, and I'm flexible to accept any selection. > > > > I noticed that the complete series is not on your watchdog/master branch, > for clarity do you request to do any changes to merge it into v4.9? I'll make the changes this weekend. I want to have the noop as default. So rest should go in this weekend so that we can still get it in for 4.9. Kind regards, Wim.