From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Subject: Re: [PATCH v5 00/10] watchdog: add watchdog pretimeout framework To: Wim Van Sebroeck References: <1472644370-16982-1-git-send-email-vladimir_zapolskiy@mentor.com> <20160928084921.GA15636@spo001.leaseweb.nl> CC: Guenter Roeck , Wolfram Sang , From: Vladimir Zapolskiy Message-ID: <1c167827-8d73-f25f-61a4-38598e0fa8ec@mentor.com> Date: Wed, 28 Sep 2016 14:34:16 +0300 MIME-Version: 1.0 In-Reply-To: <20160928084921.GA15636@spo001.leaseweb.nl> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit List-ID: 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. -- With best wishes, Vladimir