Re: [PATCH] watchdog: add sun4v_wdt device support

[Wim Coekaerts <wim.coekaerts@oracle.com>]

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@oracle.com> 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@oracle.com> wrote:
>>>>>> From: Wim Coekaerts <wim.coekaerts@oracle.com>
>>>>>>
>>>>>> 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@oracle.com>
>>>>> 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