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> <1c167827-8d73-f25f-61a4-38598e0fa8ec@mentor.com> CC: Guenter Roeck , Wolfram Sang , From: Vladimir Zapolskiy Message-ID: Date: Tue, 4 Oct 2016 16:29:20 +0300 MIME-Version: 1.0 In-Reply-To: <1c167827-8d73-f25f-61a4-38598e0fa8ec@mentor.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit List-ID: Hello Wim, 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? -- With best wishes, Vladimir