Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752967AbbDBQqJ (ORCPT ); Thu, 2 Apr 2015 12:46:09 -0400 Received: from mail-qg0-f51.google.com ([209.85.192.51]:35684 "EHLO mail-qg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752889AbbDBQqF (ORCPT ); Thu, 2 Apr 2015 12:46:05 -0400 MIME-Version: 1.0 In-Reply-To: <551C96AA.2060906@roeck-us.net> References: <1427910196-28568-1-git-send-email-abrestic@chromium.org> <20150401222218.GB13077@jhogan-linux.le.imgtec.org> <551C96AA.2060906@roeck-us.net> Date: Thu, 2 Apr 2015 09:46:04 -0700 X-Google-Sender-Auth: bEDgoxBP_Um2vgBhTB_tkV_ZF-s Message-ID: Subject: Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree From: Andrew Bresticker To: Guenter Roeck Cc: James Hogan , Wim Van Sebroeck , "linux-watchdog@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Ezequiel Garcia Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3444 Lines: 87 On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck wrote: > On 04/01/2015 03:22 PM, James Hogan wrote: >> >> Hi Andrew, >> >> On Wed, Apr 01, 2015 at 10:43:14AM -0700, Andrew Bresticker wrote: >>> >>> Since the heartbeat is statically initialized to its default value, >>> watchdog_init_timeout() will never look in the device-tree for a >>> timeout-sec value. Instead of statically initializing heartbeat, >>> fall back to the default timeout value if watchdog_init_timeout() >>> fails. >> >> >> Whoops. Sorry about that. I wasn't aware that a timeout-sec value was >> expected. It isn't mentioned in the DT binding documentation for this >> device :-(. >> >>> >>> Signed-off-by: Andrew Bresticker >>> Cc: Ezequiel Garcia >>> Cc: James Hogan >>> --- >>> New for v2. >>> --- >>> drivers/watchdog/imgpdc_wdt.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/watchdog/imgpdc_wdt.c >>> b/drivers/watchdog/imgpdc_wdt.c >>> index 0deaa4f..89b2abc 100644 >>> --- a/drivers/watchdog/imgpdc_wdt.c >>> +++ b/drivers/watchdog/imgpdc_wdt.c >>> @@ -42,7 +42,7 @@ >>> #define PDC_WDT_MIN_TIMEOUT 1 >>> #define PDC_WDT_DEF_TIMEOUT 64 >>> >>> -static int heartbeat = PDC_WDT_DEF_TIMEOUT; >>> +static int heartbeat; >>> module_param(heartbeat, int, 0); >>> MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds " >>> "(default=" __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")"); >>> @@ -195,9 +195,9 @@ static int pdc_wdt_probe(struct platform_device >>> *pdev) >>> >>> ret = watchdog_init_timeout(&pdc_wdt->wdt_dev, heartbeat, >>> &pdev->dev); >>> if (ret < 0) { >>> - pdc_wdt->wdt_dev.timeout = pdc_wdt->wdt_dev.max_timeout; >>> + pdc_wdt->wdt_dev.timeout = PDC_WDT_DEF_TIMEOUT; >> >> >> The watchdog_init_timeout kerneldoc comment suggests that the old value >> should be the default timeout, i.e. that timeout should be set to >> PDC_WDT_DEF_TIMEOUT before calling watchdog_init_timeout, rather than >> whenever ret < 0. >> >> Indeed, if heartbeat is set to an invalid non-zero value, >> watchdog_init_timeout will still try and set timeout from DT, but also >> still returns -EINVAL regardless of whether that succeeds, and this >> would incorrectly override the timeout from DT with the hardcoded >> default. >> >>> dev_warn(&pdev->dev, >>> - "Initial timeout out of range! setting max >>> timeout\n"); >>> + "Initial timeout out of range! setting default >>> timeout\n"); >> >> >> It feels wrong for a presumably safe & normal situation (i.e. no default >> in DT, which arguably shouldn't contain policy anyway) to show a >> warning, but it can also show due to an invalid module parameter (or >> invalid DT property) which is most definitely justified. >> > > Agreed. I would suggest to leave that part alone and set the default prior > to calling watchdog_init_timeout(). Yes, but I think James' concern here was that we'd now get a dev_warn() in the normal case where no timeout is specified via module parameter or DT. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/