Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

On Wed, Jun 08, 2016 at 05:08:09PM +0300, Vladimir Zapolskiy wrote:
> 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 <vladimir_zapolskiy@xxxxxxxxxx>
> >> ---
> >> 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?
> 
Kind of mixed feelings. I prefer a sequence of simple patches,
but most important is that patches are, at the end, complete.
I got the impression that your patch series wasn't complete.
That a series is incomplete is easier to hide if patches are
split into multiple parts.

Simple patches are easy to review, complex patches take more time.
But that doesn't help if the simple patches are incomplete, because
it means that the reviewer has to spend a lot of time trying to find
the missing pieces. Finding one missing piece suggests that there can
and will be more, meaning even more time needs to be spent. Since time
is always short at hand, a single missing piece may result in the entire
series to be put on the back-burner.

It can be difficult to see the forest because of all the trees,
and it can be difficult to see the individual trees in a forest.
It is all about trade-offs; nothing is black and white.

> >> * 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.
> 

Yes, I am very much concerned that I'll have to go back and look into
all the lifetime issues again which we just managed to resolve.
Not looking forward to it :-(.

Guenter