linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Cc: wim@linux-watchdog.org, shawnguo@kernel.org,
	linux-watchdog@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] watchdog: introduce watchdog_dev_suspend/resume
Date: Tue, 15 Jun 2021 07:18:03 -0700	[thread overview]
Message-ID: <20210615141803.GA957871@roeck-us.net> (raw)
In-Reply-To: <20210615123904.2568052-2-grzegorz.jaszczyk@linaro.org>

On Tue, Jun 15, 2021 at 02:39:03PM +0200, Grzegorz Jaszczyk wrote:
> The watchdog drivers often disable wdog clock during suspend and then
> enable it again during resume. Nevertheless the ping worker is still
> running and can issue low-level ping while the wdog clock is disabled
> causing the system hang. To prevent such condition introduce
> watchdog_dev_suspend/resume which can be used by any wdog driver and
> actually cancel ping worker during suspend and restore it back, if
> needed, during resume.
> 

I'll have to look into this further, but I don't think this is the correct
solution. Most likely the watchdog core needs to have its own independent
suspend/resule functions and suspend the high resolution timer on
suspend and restore it on resume. This may require an additional flag
to be set by drivers to indicate that the timer should be stopped on
suspend.

> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> ---
>  drivers/watchdog/watchdog_dev.c | 49 +++++++++++++++++++++++++++++++++
>  include/linux/watchdog.h        |  2 ++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 2946f3a63110..3feca1567281 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1219,6 +1219,55 @@ void __exit watchdog_dev_exit(void)
>  	kthread_destroy_worker(watchdog_kworker);
>  }
>  
> +int watchdog_dev_suspend(struct watchdog_device *wdd)
> +{
> +	struct watchdog_core_data *wd_data = wdd->wd_data;
> +	int ret;
> +
> +	if (!wdd->wd_data)
> +		return -ENODEV;
> +
> +	/* ping for the last time before suspend */
> +	mutex_lock(&wd_data->lock);
> +	if (watchdog_worker_should_ping(wd_data))
> +		ret = __watchdog_ping(wd_data->wdd);
> +	mutex_unlock(&wd_data->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * make sure that watchdog worker will not kick in when the wdog is
> +	 * suspended
> +	 */
> +	hrtimer_cancel(&wd_data->timer);
> +	kthread_cancel_work_sync(&wd_data->work);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(watchdog_dev_suspend);
> +
> +int watchdog_dev_resume(struct watchdog_device *wdd)
> +{
> +	struct watchdog_core_data *wd_data = wdd->wd_data;
> +	int ret;
> +
> +	if (!wdd->wd_data)
> +		return -ENODEV;
> +
> +	/*
> +	 * __watchdog_ping will also retrigger hrtimer and therefore restore the
> +	 * ping worker if needed.
> +	 */
> +	mutex_lock(&wd_data->lock);
> +	if (watchdog_worker_should_ping(wd_data))
> +		ret = __watchdog_ping(wd_data->wdd);
> +	mutex_unlock(&wd_data->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(watchdog_dev_resume);
> +
>  module_param(handle_boot_enabled, bool, 0444);
>  MODULE_PARM_DESC(handle_boot_enabled,
>  	"Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 9b19e6bb68b5..febfde3b1ff6 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -209,6 +209,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
>  				  unsigned int timeout_parm, struct device *dev);
>  extern int watchdog_register_device(struct watchdog_device *);
>  extern void watchdog_unregister_device(struct watchdog_device *);
> +int watchdog_dev_suspend(struct watchdog_device *wdd);
> +int watchdog_dev_resume(struct watchdog_device *wdd);
>  
>  int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);
>  
> -- 
> 2.29.0
> 

  reply	other threads:[~2021-06-15 14:25 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15 12:39 [PATCH 0/2] " Grzegorz Jaszczyk
2021-06-15 12:39 ` [PATCH 1/2] watchdog: " Grzegorz Jaszczyk
2021-06-15 14:18   ` Guenter Roeck [this message]
2021-06-16 13:59     ` Grzegorz Jaszczyk
2021-06-16 17:57       ` Guenter Roeck
2021-06-16 23:21   ` kernel test robot
2021-06-15 12:39 ` [PATCH 2/2] watchdog: imx2_wdg: notify wdog subsystem about wdog suspend/resume Grzegorz Jaszczyk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210615141803.GA957871@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=grzegorz.jaszczyk@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=wim@linux-watchdog.org \
    --subject='Re: [PATCH 1/2] watchdog: introduce watchdog_dev_suspend/resume' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).