linux-kernel - Re: [PATCH 1/1] watchdog: sp805: ping fails to abort wdt reset

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: <569DE192.5060406@roeck-us.net>
Date:	Mon, 18 Jan 2016 23:11:14 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Viresh Kumar <viresh.kumar@...aro.org>,
	Sandeep Tripathy <tripathy@...adcom.com>
Cc:	wim@...ana.be, linux-watchdog@...r.kernel.org,
	linux-kernel@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com
Subject: Re: [PATCH 1/1] watchdog: sp805: ping fails to abort wdt reset

On 01/18/2016 10:53 PM, Viresh Kumar wrote:
> On 19-01-16, 11:34, Sandeep Tripathy wrote:
>>   sp805 wdt asserts interrupt for the first expiry and
>
> Why an extra space before sp805 ?
>
>> reloads the counter. If wdt interrupt is set and count
>> reaches zero then wdt reset event is generated.To get
>
> Add space after full-stop.
>
>> wdt reset at t timeout the driver loads wdt counter
>
> Keep t and t/2 in single quotes, its hardly readable otherwise.
>
>> with t/2. A ping before t second *should* prevent wdt
>> reset. Currently if ping is done after t/2 sec then
>> wdt interrupt condition gets set. On the next countdown
>> of loadval wdt reset event occurs eventhough wdt was
>> reloaded before the set timeout t.
>>
>>   This patch clears the interrupt condition on ping.
>
> Why extra spaces before 'This'?
>
>>
>> Signed-off-by: Sandeep Tripathy <tripathy@...adcom.com>
>> ---
>>   drivers/watchdog/sp805_wdt.c |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 01d8162..e7a715e 100644
>> --- a/drivers/watchdog/sp805_wdt.c
>> +++ b/drivers/watchdog/sp805_wdt.c
>> @@ -139,12 +139,11 @@ static int wdt_config(struct watchdog_device *wdd, bool ping)
>>
>>   	writel_relaxed(UNLOCK, wdt->base + WDTLOCK);
>>   	writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);
>> +	writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);
>>
>> -	if (!ping) {
>> -		writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);
>> +	if (!ping)
>>   		writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base +
>>   				WDTCONTROL);
>> -	}
>>
>>   	writel_relaxed(LOCK, wdt->base + WDTLOCK);
>
> If I read the TRM properly, then you don't need to load the WDTLOAD
> register on ping and so your change can be written as:
>
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 01d816251302..42e1ec86532d 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -138,10 +138,10 @@ static int wdt_config(struct watchdog_device *wdd, bool ping)
>          spin_lock(&wdt->lock);
>
>          writel_relaxed(UNLOCK, wdt->base + WDTLOCK);
> -       writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);
> +       writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);
>
>          if (!ping) {
> -               writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);
> +               writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);
>                  writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base +
>                                  WDTCONTROL);
>          }
>

That would require additional changes. Reason is that the load value is not immediately
written after a timeout change, but only with the next ping (which happens immediately
afterwards). With your proposed change, the load value would never be updated.

Besides, that would be a separate change, and would require a separate patch.

Guenter