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

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

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@mentor.com>
> ---
> 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...

> * removed support of sleepable watchdog pretimeout governors

This does.

> * 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?`

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]