From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757058AbcFHOIP (ORCPT ); Wed, 8 Jun 2016 10:08:15 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:37647 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754550AbcFHOIN (ORCPT ); Wed, 8 Jun 2016 10:08:13 -0400 Subject: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework To: Wolfram Sang References: <1465321127-19522-1-git-send-email-vladimir_zapolskiy@mentor.com> <1465321127-19522-5-git-send-email-vladimir_zapolskiy@mentor.com> <20160608065445.GB1521@katana> CC: Wim Van Sebroeck , Guenter Roeck , Robin Gong , , From: Vladimir Zapolskiy Message-ID: <575826C9.6010808@mentor.com> Date: Wed, 8 Jun 2016 17:08:09 +0300 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Icedove/38.1.0 MIME-Version: 1.0 In-Reply-To: <20160608065445.GB1521@katana> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [137.202.0.76] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Wolfram, On 08.06.2016 09:54, Wolfram Sang wrote: > On Tue, Jun 07, 2016 at 08:38:45PM +0300, Vladimir Zapolskiy wrote: >> 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 some watchdog devices. >> >> A user selects a default watchdog pretimeout governor during >> compilation stage. >> >> Watchdogs with WDIOF_PRETIMEOUT capability now have two device >> attributes in sysfs: pretimeout to display currently set pretimeout >> value and pretimeout_governor attribute to display the selected >> watchdog pretimeout governor. >> >> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in >> sysfs, and such watchdog devices do not require the framework. >> >> Signed-off-by: Vladimir Zapolskiy >> --- >> Changes from v2 to v3: >> * essentially simplified the implementation due to removal of runtime >> dynamic selection of watchdog pretimeout governors by a user, this >> feature is supposed to be added later on > > Hmm, your call, but I'm not sure this will make the reviewing process > easier... Looks like the series hits a relatively vague contradiction described in Documentation//development-process/5.Posting : 1) logically independent change should be formatted as a separate patch 2) the changes need to be considered in their final form While in v1/v2 I've selected point 2) and even tried to defend it in conversation with Guenter, for v3 I primarily choose point 1), because all other features can be added on top. Guenter, do you have any judgment? >> * removed support of sleepable watchdog pretimeout governors > > This does. Same as above. >> * moved sysfs device attributes to watchdog_dev.c, this required to >> add exported watchdog_pretimeout_governor_name() interface > > Why this move? Before, all the pretimeout stuff was nicely encapsulated > in its own file which could be compiled out. Now things are mixing. What > was wrong with the approach I took?` Simplification of the "struct device" life time management? A lot of time and efforts were spent to centralize it, while you know that I took both approaches, I tend to keep it exclusively in watchdog_dev.c , probably Guenter can express his point of view. >> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd) >> } >> } >> >> + ret = watchdog_register_pretimeout(wdd); >> + if (ret) { >> + watchdog_dev_unregister(wdd); >> + ida_simple_remove(&watchdog_ida, wdd->id); >> + return ret; >> + } >> + > > What is the advantage of adding it here instead of adding it in > watchdog_dev.c? I mean the files to control govenors are tied to the > watchdog_device anyhow, so I'd think it's cleaner to move all that > action to watchdog_dev instead of having this stray one in the core. > This makes sense, I will move it to watchdog_dev.c. Thank you for review. -- Best wishes, Vladimir