From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:42880 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751028AbcAUBgJ (ORCPT ); Wed, 20 Jan 2016 20:36:09 -0500 Subject: Re: [PATCH] watchdog: add sun4v_wdt device support To: Guenter Roeck References: <1453321844-27615-1-git-send-email-wim.coekaerts@oracle.com> <56A01612.9040901@oracle.com> <20160120234554.GA21708@roeck-us.net> Cc: Julian Calaby , wim@iguana.be, linux-watchdog@vger.kernel.org, sparclinux From: Wim Coekaerts Message-ID: <56A035D7.5030406@oracle.com> Date: Wed, 20 Jan 2016 17:35:19 -0800 MIME-Version: 1.0 In-Reply-To: <20160120234554.GA21708@roeck-us.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org 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, wrote: >>>> From: Wim Coekaerts >>>> >>>> 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 >>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wim Coekaerts Date: Thu, 21 Jan 2016 01:35:19 +0000 Subject: Re: [PATCH] watchdog: add sun4v_wdt device support Message-Id: <56A035D7.5030406@oracle.com> List-Id: References: <1453321844-27615-1-git-send-email-wim.coekaerts@oracle.com> <56A01612.9040901@oracle.com> <20160120234554.GA21708@roeck-us.net> In-Reply-To: <20160120234554.GA21708@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guenter Roeck Cc: Julian Calaby , wim@iguana.be, linux-watchdog@vger.kernel.org, sparclinux 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, wrote: >>>> From: Wim Coekaerts >>>> >>>> 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 >>> 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