linux-kernel - Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations

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]

Date:   Wed, 29 Jul 2020 08:15:35 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Anson Huang <Anson.Huang@....com>, wim@...ux-watchdog.org,
        shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
        festevam@...il.com, linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     Linux-imx@....com
Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence
 for wdog operations

On 7/28/20 7:20 PM, Anson Huang wrote:
> According to reference manual, the i.MX7ULP WDOG's operations should
> follow below sequence:
> 
> 1. disable global interrupts;
> 2. unlock the wdog and wait unlock bit set;
> 3. reconfigure the wdog and wait for reconfiguration bit set;
> 4. enabel global interrupts.
> 
> Strictly follow the recommended sequence can make it more robust.
> 
> Signed-off-by: Anson Huang <Anson.Huang@....com>
> ---
> Changes since V1:
> 	- use readl_poll_timeout_atomic() instead of usleep_ranges() since IRQ is disabled.
> ---
>  drivers/watchdog/imx7ulp_wdt.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index 7993c8c..7d2b12e 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -36,6 +37,7 @@
>  #define DEFAULT_TIMEOUT	60
>  #define MAX_TIMEOUT	128
>  #define WDOG_CLOCK_RATE	1000
> +#define WDOG_WAIT_TIMEOUT	10000
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0000);
> @@ -48,17 +50,31 @@ struct imx7ulp_wdt_device {
>  	struct clk *clk;
>  };
>  
> +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask)
> +{
> +	u32 val = readl(base + WDOG_CS);
> +
> +	if (!(val & mask))
> +		WARN_ON(readl_poll_timeout_atomic(base + WDOG_CS, val,
> +						  val & mask, 0,
> +						  WDOG_WAIT_TIMEOUT));
> +}
> +
>  static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
>  {
>  	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
>  
>  	u32 val = readl(wdt->base + WDOG_CS);
>  
> +	local_irq_disable();
>  	writel(UNLOCK, wdt->base + WDOG_CNT);
> +	imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK);
>  	if (enable)
>  		writel(val | WDOG_CS_EN, wdt->base + WDOG_CS);
>  	else
>  		writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
> +	imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
> +	local_irq_enable();
>  }
>  
>  static bool imx7ulp_wdt_is_enabled(void __iomem *base)
> @@ -72,7 +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
>  {
>  	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
>  
> +	local_irq_disable();
> +	writel(UNLOCK, wdt->base + WDOG_CNT);
> +	imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK);
>  	writel(REFRESH, wdt->base + WDOG_CNT);
> +	imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);

Per reference manual (section 59.5.4), the waits are not required here,
and neither is the unlock. For practical purposes, disabling interrupts
is useless as well since the refresh write operation is just a single
register write.

Guenter