linux-kernel - RE: [PATCH 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] [day] [month] [year] [list]

Message-ID: <DB3PR0402MB39169A9F5E5D2D90A64AAF3DF5700@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Wed, 29 Jul 2020 02:26:33 +0000
From:   Anson Huang <anson.huang@....com>
To:     Guenter Roeck <linux@...ck-us.net>,
        "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for
 wdog operations

Hi, Guenter


> Subject: RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for
> wdog operations
> 
> Hi, Guenter
> 
> 
> > Subject: Re: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the
> > sequence for wdog operations
> >
> > On 7/27/20 11:42 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>
> > > ---
> > >  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..b414ecf 100644
> > > --- a/drivers/watchdog/imx7ulp_wdt.c
> > > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > > @@ -4,6 +4,7 @@
> > >   */
> > >
> > >  #include <linux/clk.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/io.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/module.h>
> > > @@ -48,17 +49,32 @@ struct imx7ulp_wdt_device {
> > >  	struct clk *clk;
> > >  };
> > >
> > > +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
> > > +	int retries = 100;
> > > +
> > > +	do {
> > > +		if (readl_relaxed(base + WDOG_CS) & mask)
> > > +			return;
> > > +		usleep_range(200, 1000);
> > > +	} while (retries--);
> >
> > Sleep with interrupts disabled ? I can not imagine that this works
> > well in a single CPU system. On top of that, it seems quite pointless.
> > Either you don't want to be interrupted or you do, but sleeping with
> > interrupts disabled really doesn't make sense. And does it really take
> > 200-1000 uS for the watchdog subsystem to react, and sometimes up to
> > 200 * 100 = 20 mS ? That seems highly unlikely. If such a delay loop
> > is indeed needed, it should be limited by a time, not by number of
> repetitions.
> >
> > Unless there is evidence that there is a problem that needs to be
> > solved, I am not going to accept this code.
> >
> 
> Oops, this is a mistake of using sleep with interrupt disabled, sorry for that.
> The best option is to use readl_relaxed_poll_timeout_atomic() to poll the
> status bit, however, the i.MX7ULP watchdog is very special that the unlock
> window ONLY open for several cycles, that means the unlock status bit will be
> set and then clear automatically after those cycles, using
> readl_relaxed_poll_timeout_atomic() will fail since there are many timeout
> handle code in it and the unlock window is open and close during this timeout
> handle interval, so it fail to catch the unlock bit.
> 
> The ideal option is using atomic polling without any other timeout check to
> make sure the unlock window is NOT missed, but I think Linux kernel will NOT
> accept a while loop without timeout, and that is why I tried to use
> usleep_ranges(), but obviously I made a mistake of using it with IRQ disabled.
> 
> Do you have any suggestion of how to handle such case? If the hardware ONLY
> unlock the register for a small window, how to poll the status bit with timeout
> handle and also make sure the timeout handle code as quick as possible to
> NOT miss the window?
> 

I did more experiment and found that below readl_poll_timeout_atomic() is actually
working, so I sent a V2 with it, please help review, thank you.


+       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));

Thanks,
Anson