On Wed, Apr 22, 2015 at 02:11:35PM +0300, Timo Kokkonen wrote:
> There is a great deal of diversity in the watchdog hardware found on
> different devices. Differen hardware have different contstraints on
> them, many of the constraints that are excessively difficult for the
> user space to satisfy.
>
> One such constraint is the length of the timeout value, which in many
> cases can be just a few seconds. Drivers are creating ad hoc solutions
> with timers and workqueues to extend the timeout in order to give user
> space more time between updates. Looking at the drivers it is clear
> that this has resulted to a lot of duplicate code.
>
> Add an extension to the watchdog kernel API that allows the driver to
> describe tis HW constraints to the watchdog code. A kernel worker in
> the core is then used to extend the watchdog timeout on behalf of the
> user space. This allows the drivers to be simplified as core takes
> over the timer extending.
>
> Tested-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> Signed-off-by: Timo Kokkonen <timo.kokkonen@xxxxxxxxxx>
I started twice to review this series, and each time there is a new version
before I can finish the review. Guess I'll wait until it settles down a bit
before trying again :-(. Just a quick comment below.
> ---
> drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
> drivers/watchdog/watchdog_dev.c | 75 ++++++++++++++++++++++++++---
> include/linux/watchdog.h | 23 +++++++++
> 3 files changed, 189 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..fd12489 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> /**
> + * watchdog_init_parms() - initialize generic watchdog parameters
> + * @wdd: Watchdog device to operate
> + * @dev: Device that stores the device tree properties
> + *
> + * Initialize the generic timeout parameters. The driver needs to set
> + * hw_features bitmask from @wdd prior calling this function in order
> + * to ensure the core knows how to handle the HW.
> + *
> + * A zero is returned on success and -EINVAL for failure.
> + */
> +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
> +{
> + int ret = 0;
> +
> + ret = watchdog_init_timeout(wdd, wdd->timeout, dev);
You are changing the semantics of watchdog_init_timeout here;
for all practical purposes it no longer accepts the timeout passed
as parameter, but expects the timeout to be configured in wdd->timeout
instead. Please don't do that.
Guenter
--
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
