Hi Wim, On Thu, Jan 21, 2016 at 1:36 PM, Wim Coekaerts <wim.coekaerts@xxxxxxxxxx> wrote: > 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 Ok, I misunderstood. time_remaining is useless then. Thanks, -- Julian Calaby Email: julian.calaby@xxxxxxxxx Profile: http://www.google.com/profiles/julian.calaby/ -- 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
