Re: [PATCH 1/2] watchdog: da9063: Ping watchdog only during allowed time window

[Guenter Roeck <linux@roeck-us.net>]

On 08/06/2015 09:36 AM, Philipp Zabel wrote:
> There is a cooldown period after watchdog timer setup, and also after each
> watchdog ping. If a ping is issued too early, the watchdog triggers the
> watchdog error condition and causes a system reset.
>
> The 256 ms period length is a best guess based on the same value being used
> for the DA9053 predecessor PMIC and experiments that have shown 200 ms are
> not enough to avoid the error.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Hi Philipp,

instead of using this quite complex approach, would it be possible to use the
same approach as the da9062 driver ?

I understand the downside, which is that the caller may end up sleeping,
but this should not happen too often. Also, I plan to move the enforcement
of a minimal time between heartbeats into the infrastructure, so this would
not least forever.

Thanks,
Guneter

> ---
>   drivers/watchdog/da9063_wdt.c | 88 ++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 79 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index e2fe2eb..b2e9201 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -35,11 +35,15 @@ static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 };
>   #define DA9063_WDT_MIN_TIMEOUT		wdt_timeout[DA9063_TWDSCALE_MIN]
>   #define DA9063_WDT_MAX_TIMEOUT		wdt_timeout[DA9063_TWDSCALE_MAX]
>   #define DA9063_WDG_TIMEOUT		wdt_timeout[3]
> +#define DA9063_TWDMIN			256
>
>   struct da9063_watchdog {
>   	struct da9063 *da9063;
>   	struct watchdog_device wdtdev;
>   	struct notifier_block restart_handler;
> +	struct delayed_work ping_work;
> +	struct mutex mutex;
> +	bool defer_ping;
>   };
>
>   static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
> @@ -54,10 +58,23 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
>   	return DA9063_TWDSCALE_MAX;
>   }
>
> -static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
> +static int _da9063_wdt_set_timeout(struct da9063_watchdog *wdt,
> +				   unsigned int regval)
>   {
> -	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> -				  DA9063_TWDSCALE_MASK, regval);
> +	struct da9063 *da9063 = wdt->da9063;
> +	int ret;
> +
> +	mutex_lock(&wdt->mutex);
> +
> +	ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> +				 DA9063_TWDSCALE_MASK, regval);
> +
> +	wdt->defer_ping = false;
> +	schedule_delayed_work(&wdt->ping_work, msecs_to_jiffies(DA9063_TWDMIN));
> +
> +	mutex_unlock(&wdt->mutex);
> +
> +	return ret;
>   }
>
>   static int da9063_wdt_start(struct watchdog_device *wdd)
> @@ -67,12 +84,16 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
>   	int ret;
>
>   	selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout);
> -	ret = _da9063_wdt_set_timeout(wdt->da9063, selector);
> -	if (ret)
> +	ret = _da9063_wdt_set_timeout(wdt, selector);
> +	if (ret) {
>   		dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n",
>   			ret);
> +		return ret;
> +	}
>
> -	return ret;
> +	wdd->timeout = wdt_timeout[selector];
> +
> +	return 0;
>   }
>
>   static int da9063_wdt_stop(struct watchdog_device *wdd)
> @@ -80,6 +101,8 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
>   	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
>   	int ret;
>
> +	cancel_delayed_work_sync(&wdt->ping_work);
> +
>   	ret = regmap_update_bits(wdt->da9063->regmap, DA9063_REG_CONTROL_D,
>   				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
>   	if (ret)
> @@ -89,9 +112,8 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
>   	return ret;
>   }
>
> -static int da9063_wdt_ping(struct watchdog_device *wdd)
> +static int da9063_wdt_issue_ping(struct da9063_watchdog *wdt)
>   {
> -	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
>   	int ret;
>
>   	ret = regmap_write(wdt->da9063->regmap, DA9063_REG_CONTROL_F,
> @@ -100,9 +122,45 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
>   		dev_alert(wdt->da9063->dev, "Failed to ping the watchdog (err = %d)\n",
>   			  ret);
>
> +	wdt->defer_ping = false;
> +	schedule_delayed_work(&wdt->ping_work, msecs_to_jiffies(DA9063_TWDMIN));
> +
>   	return ret;
>   }
>
> +static int da9063_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	int ret = 0;
> +
> +	mutex_lock(&wdt->mutex);
> +
> +	if (delayed_work_pending(&wdt->ping_work)) {
> +		wdt->defer_ping = true;
> +
> +		goto out;
> +	}
> +
> +	da9063_wdt_issue_ping(wdt);
> +out:
> +	mutex_unlock(&wdt->mutex);
> +
> +	return ret;
> +}
> +
> +static void da9063_wdt_ping_work(struct work_struct *work)
> +{
> +	struct da9063_watchdog *wdt = container_of(to_delayed_work(work),
> +					struct da9063_watchdog, ping_work);
> +
> +	mutex_lock(&wdt->mutex);
> +
> +	if (wdt->defer_ping)
> +		da9063_wdt_issue_ping(wdt);
> +
> +	mutex_unlock(&wdt->mutex);
> +}
> +
>   static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>   				  unsigned int timeout)
>   {
> @@ -111,7 +169,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>   	int ret;
>
>   	selector = da9063_wdt_timeout_to_sel(timeout);
> -	ret = _da9063_wdt_set_timeout(wdt->da9063, selector);
> +	ret = _da9063_wdt_set_timeout(wdt, selector);
>   	if (ret)
>   		dev_err(wdt->da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
>   			ret);
> @@ -178,6 +236,9 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>
>   	wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
>
> +	INIT_DELAYED_WORK(&wdt->ping_work, da9063_wdt_ping_work);
> +	mutex_init(&wdt->mutex);
> +
>   	watchdog_set_drvdata(&wdt->wdtdev, wdt);
>   	dev_set_drvdata(&pdev->dev, wdt);
>
> @@ -202,13 +263,22 @@ static int da9063_wdt_remove(struct platform_device *pdev)
>   	unregister_restart_handler(&wdt->restart_handler);
>
>   	watchdog_unregister_device(&wdt->wdtdev);
> +	cancel_delayed_work_sync(&wdt->ping_work);
>
>   	return 0;
>   }
>
> +static void da9063_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct da9063_watchdog *wdt = dev_get_drvdata(&pdev->dev);
> +
> +	cancel_delayed_work_sync(&wdt->ping_work);
> +}
> +
>   static struct platform_driver da9063_wdt_driver = {
>   	.probe = da9063_wdt_probe,
>   	.remove = da9063_wdt_remove,
> +	.shutdown = da9063_wdt_shutdown,
>   	.driver = {
>   		.name = DA9063_DRVNAME_WATCHDOG,
>   	},
>