linux-kernel - Re:Re: [PATCH V3] watchdog: mtk: support pre-timeout when the bark irq is available

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: <AAgApgCdDjCicbWFGpPmeaor.3.1618479605424.Hmail.wangqing@vivo.com>
Date:   Thu, 15 Apr 2021 17:40:05 +0800 (GMT+08:00)
From:   王擎 <wangqing@...o.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re:Re: [PATCH V3] watchdog: mtk: support pre-timeout when the bark irq is available


>On 4/14/21 4:48 AM, Wang Qing wrote:
>> Use the bark interrupt as the pretimeout notifier if available.
>> 
>> By default, the pretimeout notification shall occur one second earlier
>> than the timeout.
>> 
>> V2:
>> - panic() by default if WATCHDOG_PRETIMEOUT_GOV is not enabled.
>> 
>> V3:
>> - Modify the pretimeout behavior, manually reset after the pretimeout
>> - is processed and wait until timeout.
>> 
>> Signed-off-by: Wang Qing <wangqing@...o.com>
>> ---
>>  drivers/watchdog/mtk_wdt.c | 62 ++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 57 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>> index 97ca993..7bef1e3
>> --- a/drivers/watchdog/mtk_wdt.c
>> +++ b/drivers/watchdog/mtk_wdt.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/reset-controller.h>
>>  #include <linux/types.h>
>>  #include <linux/watchdog.h>
>> +#include <linux/interrupt.h>
>>  
>>  #define WDT_MAX_TIMEOUT		31
>>  #define WDT_MIN_TIMEOUT		1
>> @@ -234,18 +235,46 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
>>  	void __iomem *wdt_base = mtk_wdt->wdt_base;
>>  	int ret;
>>  
>> -	ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
>> +	ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout - wdt_dev->pretimeout);
>>  	if (ret < 0)
>>  		return ret;
>>  
>>  	reg = ioread32(wdt_base + WDT_MODE);
>> -	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>> +	reg &= ~WDT_MODE_IRQ_EN;
>> +	if (wdt_dev->pretimeout)
>> +		reg |= WDT_MODE_IRQ_EN;
>> +	else
>> +		reg &= ~WDT_MODE_IRQ_EN;
>>  	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
>>  	iowrite32(reg, wdt_base + WDT_MODE);
>>  
>>  	return 0;
>>  }
>>  
>> +static int mtk_wdt_set_pretimeout(struct watchdog_device *wdd,
>> +				   unsigned int timeout)
>> +{
>> +	wdd->pretimeout = timeout;
>> +	return mtk_wdt_start(wdd);
>
>The watchdog is not necessarily active here.
>
>> +}
>> +
>> +static irqreturn_t mtk_wdt_isr(int irq, void *arg)
>> +{
>> +	struct watchdog_device *wdd = arg;
>> +	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdd);
>> +	void __iomem *wdt_base = mtk_wdt->wdt_base;
>> +
>> +	watchdog_notify_pretimeout(wdd);
>> +	/*
>> +	 * Guaranteed to be reset when the timeout
>> +	 * expires under any situations
>> +	 */
>> +	mdelay(1000*wdd->pretimeout);
>
>That is not how this is supposed to work. The idea with a pretimeout is that the
>real watchdog reset will happen under all circumstances, and that executing
>the pretimeout (and changing some hardware registers) is not a prerequisite
>for the real timeout to happen. After all, the system could be stuck hard, with
>interrupts disabled.
>
>On top of that, just sleeping here while waiting for the real timeout and
>then resetting the system isn't the idea either. On a single core system this
>will just hang. On a multi-core system, who knows if userspace managed to ping
>the watchdog in the meantime.
>
>Unless there is a means to trigger the watchdog twice, without intervention,
>the first time generating an interrupt and the second time resetting the system,
>there is no way for this to work. I don't see how this chip really supports
>pretimeout. It seems that it supports either a hard reset or generating an
>interrupt on watchdog timeout, and there is only a single timeout.
>
>If you have a use case for generating an interrupt and resetting the system via
>software (ie panic) _instead_ of having it generate a hard reset, please feel
>free to submit a patch along that line, together with a description of its use
>case.
>
>Thanks,
>Guenter
>

Yes, as mentioned before, the behavior of WDT_MODE_IRQ_EN is use irq instead of
reset, so we must use WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN if like you said
"the first time generating an interrupt and the second time resetting the system" . 

The Dual mode is mentioned in the MTK datasheet:
In this mode, the watchdog will be AUTO-RESTART after interrupt is triggered. 
AP need to clear WDT_STA after receiving interrupt from TOPRGU, or system reset
will be triggered after watchdog timer expires.
Instructions for use：
Set wdt_en = 1'b1.
Set dual_mode = 1'b1.
Set wdt_irq = 1'b1.

We can use Dual mode to achieve pretimeout behavior, only in this way can we
get more information during pretimeout processing, instead of directly resetting.

Thanks,
Qing