On 1/20/16 6:23 PM, Julian Calaby wrote:
Hi Wim,
On Thu, Jan 21, 2016 at 12:35 PM, Wim Coekaerts
<wim.coekaerts@xxxxxxxxxx> wrote:
On 1/20/16 3:45 PM, Guenter Roeck wrote:
On Wed, Jan 20, 2016 at 03:19:46PM -0800, Wim Coekaerts wrote:
On 01/20/2016 02:43 PM, Julian Calaby wrote:
Hi Wim Coekaerts,
On Thu, Jan 21, 2016 at 7:30 AM, <wim.coekaerts@xxxxxxxxxx> wrote:
From: Wim Coekaerts <wim.coekaerts@xxxxxxxxxx>
This adds a simple watchdog timer for the SPARC sunv4 architecture.
Export the sun4v_mach_set_watchdog() hv call, and add the target.
The driver is implemented using the new watchdog api framework.
Signed-off-by: Wim Coekaerts <wim.coekaerts@xxxxxxxxxx>
This all looks _much_ better.
thanks! :)
There's nothing glaringly wrong with the code like the last version,
so I've only got a couple of general questions:
1. Is the platform device and driver necessary? Can't you just
register the watchdog device directly from sun4v_wdt_init_module()?
2. If the platform device is unnecessary, do you still need a struct
watchdog_device in struct sun4v_wdt? I.e. does the watchdog core stop
watchdogs when you call watchdog_unregister_device()? (Or could you
refactor to not require it?)
I like to be able to implement support for suspend/resume for ldoms
as we add more support for that in the future, and support for migrating
domains and such. So having a platform driver is useful to here as a
framework.
3. Is it possible to get the data for sun4v_wdt_get_timeleft() by
calling some other sun4v_mach_*() function?
No there is only one hv call for watchdog and that's this one.
time_remaining is the time that was left until the timer was going
to expire when making the call so it's not useful for the future time.
And you can't just make a call to get time_remaining except to reset
the timer or disable it along with it. quantum physics timer :)
I'll comment on the rest of the driver separately. However, since
get_timeleft()
was brought up - the idea here is to report the time left as reported from
the
hardware, not the time left calculated by software. If we want to
calculate
the time left until expiry in software, it should be done in the watchdog
core.
It should not be done in drivers.
I guess you could add a soft_get_timeleft() so that it's clear that if a
driver doesn't
implement the call, you get a best effort response.
Happy to remove the op from the driver and maybe we can implement it
separately in core in a separate patch if that's of interest.
However, this makes me ponder what could be done with the "time_remaining"
behavior on sun4v. It could be useful for a piece of software to use that
value
to see the drift. "I pinged 30 seconds ago, per my assumption of time but
the
timer says it was 50 seconds ago, something is wrong".
What if I would report back time_remaining like that, but of course now
that it's
pinged again it reset the timer. Not sure whether you can see any use of
such
behavior. It wouldn't be get_timeleft() but more get_timewasleft() ;) or
it could be a positive return from _ping... just noodling
How about _start() and _ping() set wdt->expires like this:
err = sun4v_mach_set_watchdog(wdd->timeout * 1000, &time_remaining);
wdt->expires = ktime_to_timespec(ktime_get()).tv_sec + time_remaining / 1000;
Then _timeleft() could be something like:
return wdt->expires - ktime_to_timespec(ktime_get()).tv_sec;
Thanks,
This is how the return works :
let's say wdd->timeout = 60
I call it at time [0s] - so my timer expires in 60s (at time [60s])
at time [20s], I ping, it will end up with :
set_watchdog(60, &time_remaining)
timer expires is back to 60s from time [20s] so would expires at time [80s]
however, at time [20s] time_remaining will return 40 (time remaining on
timer when I made the call).
as this returns the time remaining from the previous timer.
40 is clearly wrong for the use of timeleft or expires because the timer
is reset back to 60s countdown
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
