From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751453AbdJLN5Q (ORCPT ); Thu, 12 Oct 2017 09:57:16 -0400 Received: from m13-125.163.com ([220.181.13.125]:11260 "EHLO m13-125.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751138AbdJLN5O (ORCPT ); Thu, 12 Oct 2017 09:57:14 -0400 X-Greylist: delayed 927 seconds by postgrey-1.27 at vger.kernel.org; Thu, 12 Oct 2017 09:57:11 EDT X-Originating-IP: [121.229.139.157] Date: Thu, 12 Oct 2017 21:40:33 +0800 (CST) From: =?GBK?B?wfXOyA==?= <18502523564@163.com> To: "Guenter Roeck" Cc: "Alexandre Belloni" , a.zummo@towertech.it, linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org Subject: Re:Re: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog X-Priority: 3 X-Mailer: Coremail Webmail Server Version SP_ntes V3.5 build 20160729(86883.8884) Copyright (c) 2002-2017 www.mailtech.cn 163com In-Reply-To: <20171010150514.GB1879@roeck-us.net> References: <1507641172-6099-1-git-send-email-18502523564@163.com> <8b80788f-5906-38a5-4b40-0a78f0ab55d9@roeck-us.net> <20171010135134.2zepquujhfwbhxoq@piout.net> <20171010150514.GB1879@roeck-us.net> Content-Type: text/plain; charset=GBK MIME-Version: 1.0 Message-ID: <33912070.da58.15f10d0b307.Coremail.18502523564@163.com> X-Coremail-Locale: zh_CN X-CM-TRANSID: fcGowAAHD+vRcN9ZBko9AA--.43315W X-CM-SenderInfo: jprykiysvsjkiwu6il2tof0z/1tbiVh5v9VWBaO7UEgABsH X-Coremail-Antispam: 1U5529EdanIXcx71UUUUU7vcSsGvfC2KfnxnUU== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v9CDvK5F011690 At 2017-10-10 23:05:14, "Guenter Roeck" wrote: >On Tue, Oct 10, 2017 at 03:51:34PM +0200, Alexandre Belloni wrote: >> On 10/10/2017 at 06:41:15 -0700, Guenter Roeck wrote: >> > On 10/10/2017 06:12 AM, winton.liu wrote: >> > > When enable CONFIG_RTC_DRV_DS1374_WDT use as watchdog, >> > > in suspend mode, watchdog is still working but no daemon >> > > patting the watchdog. The system will reboot if timeout. >> > > >> > > Add support suspend/resume for watchdog. >> > > suspend: disable the watchdog >> > > resume: disable existing watchdog, reload watchdog timer, enable watchdog >> > > >> > > Signed-off-by: winton.liu <18502523564@163.com> >> > > --- >> > > drivers/rtc/rtc-ds1374.c | 31 +++++++++++++++++++++++++++++++ >> > > 1 file changed, 31 insertions(+) >> > > >> > > diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c >> > > index 38a2e9e..642e31d 100644 >> > > --- a/drivers/rtc/rtc-ds1374.c >> > > +++ b/drivers/rtc/rtc-ds1374.c >> > > @@ -437,6 +437,29 @@ static void ds1374_wdt_ping(void) >> > > pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret); >> > > } >> > > +static void ds1374_wdt_resume(void) >> > > +{ >> > > + int ret = -ENOIOCTLCMD; >> > >> > Useless initialization (yes, I can see that this is widely done in the driver, >> > but that doesn't make it better). >> > Yes, I think this is useless. The original ds1374_wdt_disable has the same issue. >> > > + int cr; >> > > + >> > > + cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR); >> > > + >> > > + /* Disable any existing watchdog/alarm before setting the new one */ >> > > + cr &= ~DS1374_REG_CR_WACE; >> > > + >> > > + i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr); >> > > + >> > > + /* Reload watchdog timer */ >> > > + ds1374_wdt_ping(); >> > > + >> > > + /* Enable watchdog timer */ >> > > + cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM; >> > > + cr &= ~DS1374_REG_CR_AIE; >> > > + >> > > + ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr); >> > > + >> > Extra empty line. Also, returns void, so what is the point of assigning >> > the result to ret ? >> > >> > > +} >> > >> > Unless I am missing something, this unconditionally starts the watchdog >> > at resume time. So if it was not running before, it will be started anyway, >> > and the system will reboot since there will be no ping. >> > This driver starts watchdog by default. In probe watchdog starts: #ifdef CONFIG_RTC_DRV_DS1374_WDT save_client = client; ret = misc_register(&ds1374_miscdev); if (ret) return ret; ret = register_reboot_notifier(&ds1374_wdt_notifier); if (ret) { misc_deregister(&ds1374_miscdev); return ret; } ds1374_wdt_settimeout(131072); //Here starts watchdog #endif And userspace watchdogd will ping. >> >> Also, I'm still not convinced this is the right thing to do. I have seen >> many systems were it was desirable to let the watchdog run while the >> system is suspended. It ensures it will either wake up or reboot. If you >> don't want that, why not disabling the watchdog from userspace before >> going to suspend? >> > >Usually watchdog drivers supporting suspend/resume do handle it this way. >Maybe that depends on the HW. Expecting user space to do it makes it >even more racy than it already is, since there is no watchdog protection >after it has been disabled, so I am not sure if that is really better. >Does anyone happen to know if/how systemd and watchdogd are handling >this situation ? > >> > I assume it is guaranteed that the chip doesn't forget the previously >> > configured timeout on resume. >> > >> > Overall the driver would really benefit from a conversion to the watchdog >> > subsystem. >> > >> >> That is the point of https://www.spinics.net/lists/linux-watchdog/msg12095.html > >Ah, yes, and I even provided feedback. Hope I didn't miss an updated >version of that patch. Either case, seems to me we should wait for that >patch to make it in before accepting any further changes to the driver. > Is this multi function patch has any update ? If not, could my patch be acceptable before moving to watchdog subsystem.(I could update a new patch for your suggestion)? Because current kernel using ds1374 for watchdog. If the device need suspend, there will be a reboot issue. >Guenter