linux-kernel - Re: [PATCH 1/5] wdt: sunxi: Move restart code to the watchdog driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <535BD13B.103@roeck-us.net>
Date:	Sat, 26 Apr 2014 08:31:07 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Arnd Bergmann <arnd@...db.de>, wim@...ana.be,
	dbaryshkov@...il.com, dwmw2@...radead.org
CC:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-watchdog@...r.kernel.org, linux-sunxi@...glegroups.com
Subject: Re: [PATCH 1/5] wdt: sunxi: Move restart code to the watchdog driver

On 04/23/2014 08:04 AM, Maxime Ripard wrote:
> Most of the watchdog code is duplicated between the machine restart code and
> the watchdog driver. Add the restart hook to the watchdog driver, to be able to
> remove it from the machine code eventually.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> ---
>   drivers/watchdog/sunxi_wdt.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index cd00a7836cdc..0644c45d2b60 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -14,6 +14,7 @@
>    */
>
>   #include <linux/clk.h>
> +#include <linux/delay.h>
>   #include <linux/err.h>
>   #include <linux/init.h>
>   #include <linux/io.h>
> @@ -22,9 +23,12 @@
>   #include <linux/moduleparam.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
>
> +#include <asm/system_misc.h>
> +
>   #define WDT_MAX_TIMEOUT         16
>   #define WDT_MIN_TIMEOUT         1
>   #define WDT_MODE_TIMEOUT(n)     ((n) << 3)
> @@ -70,6 +74,30 @@ static const int wdt_timeout_map[] = {
>   	[16] = 0b1011, /* 16s */
>   };
>
> +static struct sunxi_wdt_dev *sunxi_restart_ctx;
> +
> +static void sun4i_wdt_restart(enum reboot_mode mode, const char *cmd)
> +{
> +	if (!sunxi_restart_ctx)
> +		return;
> +
The only condition where this can happen if is there is a race between the calling
code and the remove function below. If that race really exists, it could as well
happen after this check, so the check does not really provide any value and can
be removed.

Note that I find the variable name a bit misleading. It isn't a context,
it is the poitner to sunxi_wdt_dev. What is really needed is wdt_base,
not this pointer. Would it make more sense to provide that pointer
directly instead, like you did in the code for the A31 ?

Guenter

> +	/* Enable timer and set reset bit in the watchdog */
> +	writel(WDT_MODE_EN | WDT_MODE_RST_EN,
> +	       sunxi_restart_ctx->wdt_base + WDT_MODE);
> +
> +	/*
> +	 * Restart the watchdog. The default (and lowest) interval
> +	 * value for the watchdog is 0.5s.
> +	 */
> +	writel(WDT_CTRL_RELOAD, sunxi_restart_ctx->wdt_base + WDT_CTRL);
> +
> +	while (1) {
> +		mdelay(5);
> +		writel(WDT_MODE_EN | WDT_MODE_RST_EN,
> +		       sunxi_restart_ctx->wdt_base + WDT_MODE);
> +	}
> +}
> +
>   static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
>   {
>   	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> @@ -181,6 +209,9 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>   	if (unlikely(err))
>   		return err;
>
> +	sunxi_restart_ctx = sunxi_wdt;
> +	arm_pm_restart = sun4i_wdt_restart;
> +
>   	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
>   			sunxi_wdt->wdt_dev.timeout, nowayout);
>
> @@ -191,6 +222,8 @@ static int sunxi_wdt_remove(struct platform_device *pdev)
>   {
>   	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
>
> +	arm_pm_restart = NULL;
> +
>   	watchdog_unregister_device(&sunxi_wdt->wdt_dev);
>   	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, NULL);
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/