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?
>> * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
