linux-kernel - Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB 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] [day] [month] [year] [list]

Date:	Fri, 27 May 2016 17:24:55 +0200
From:	Neil Armstrong <narmstrong@...libre.com>
To:	Guenter Roeck <linux@...ck-us.net>,
	Wim Van Sebroeck <wim@...ana.be>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org, linux-watchdog@...r.kernel.org
Subject: Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

On 05/27/2016 03:48 PM, Guenter Roeck wrote:
> On 05/27/2016 01:25 AM, Neil Armstrong wrote:
> [ ... ]
>>>> +    data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>>>> +    data->wdt_dev.min_hw_heartbeat_ms = 1;
>>>
>>> Does the device require a minimum time between heartbeats ?
>>> Just asking, because you violate it yourself below.
>>>
>>> If you want to set the minimum timeout, that would be min_timeout.
>>
>> Ok, these values are not very clear actually.
>>
> Hmmm .. yes, reading the description again, it doesn't really describe well
> what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
> core, and should be used if a watchdog hardware can not tolerate short intervals
> between heartbeats. min_timeout is the minimum timeout value configurable from
> user space.
OK, I'll switch to min_timeout = 1.

> 

[ ... ]

>>>
>>> This is unusual. If the watchdog can be already running, it might make sense
>>> to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
>>> can send heartbeats until user space opens the device.
>>
>> Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.
>>
> 
> Not only that - if the watchdog _is_ already running at boot time, you should
> really set WDOG_HW_RUNNING to let the watchdog core know. You status function
> would come handy there.
> 
>     if (meson_gxbb_wdt_status(data))    // note the changed parameter
>         set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status);
Yes, it can be handy.

I will push this feature in a next version, I'll stick to a simpler behavior and check
if it would be running before the kernel starts.

Thanks,
Neil

> 
> Thanks,
> Guenter
>