Re: [PATCH 3/3] watchdog: qcom: register a restart notifier

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

On 09/18/2014 03:27 PM, Josh Cartwright wrote:
> The WDT's BITE_TIME warm-reset behavior can be leveraged as a last
> resort mechanism for triggering chip reset.  Usually, other restart
> methods (such as PS_HOLD) are preferrable for issuing a more complete
> reset of the chip.  As such, keep the priority of the watchdog notifier
> low.
>
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>   drivers/watchdog/qcom-wdt.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index e9409f5..710ab43 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -15,6 +15,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/watchdog.h>
>
>   #define WDT_RST		0x0
> @@ -24,6 +25,7 @@
>   struct qcom_wdt {
>   	struct watchdog_device	wdd;
>   	unsigned long		freq;
> +	struct notifier_block	restart_nb;
>   	void __iomem		*base;
>   };
>
> @@ -82,6 +84,24 @@ static const struct watchdog_info qcom_wdt_info = {
>   	.identity	= KBUILD_MODNAME,
>   };
>
> +static int qcom_wdt_restart(struct notifier_block *nb, unsigned long action,
> +			    void *data)
> +{
> +	struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb);
> +
> +	/*
> +	 * Trigger watchdog bite:
> +	 *    Setup BITE_TIME to be very low, and enable WDT.
> +	 */
> +	mutex_lock(&wdt->wdd.lock);

At this time you don't need to worry about locks.

Actually, this might be dangerous if the lock happens to be taken,
as it won't be released (there is no other code running anymore
when this function is called).

> +	writel_relaxed(0, wdt->base + WDT_EN);
> +	writel_relaxed(1, wdt->base + WDT_RST);
> +	writel_relaxed(0x31F3, wdt->base + WDT_BITE_TIME);

What is the magic here, ie what does 0x31F3 stand for ?

> +	writel_relaxed(1, wdt->base + WDT_EN);
> +	mutex_unlock(&wdt->wdd.lock);
> +	return NOTIFY_DONE;
> +}
> +
>   static int qcom_watchdog_probe(struct platform_device *pdev)
>   {
>   	struct qcom_wdt *wdt;
> @@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>
> +	/*
> +	 * WDT restart notifier has priority 0 (use as a last resort)
> +	 */
> +	wdt->restart_nb.notifier_call = qcom_wdt_restart;
> +	ret = register_restart_handler(&wdt->restart_nb);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to setup restart handler\n");
> +		watchdog_unregister_device(&wdt->wdd);
> +		return ret;

Sure you want to return an error here ? The watchdog itself is still working,
and this is supposed to be a restart method of last resort. Causing the driver
to fail loading because it can not register its restart handler seems to be
a bit aggressive.

Thanks,
Guenter

> +	}
> +
>   	return 0;
>   }
>
>