Messages in this thread | | | Subject | Re: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog | From | Guenter Roeck <> | Date | Thu, 12 Oct 2017 07:12:24 -0700 |
| |
On 10/12/2017 06:40 AM, 刘稳 wrote: > > > > > > > > > > > At 2017-10-10 23:05:14, "Guenter Roeck" <linux@roeck-us.net> 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.
That doesn't make it better and is not an argument.
>>>>> + 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
Meaning the system will reboot unless the watchdog device is opened ? Weird, and conceptually wrong.
What if the watchdog device is stopped explicitly by the watchdog demon ?
> #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. > Only if CONFIG_RTC_DRV_DS1374_WDT is enabled, and then it appears the driver has other problems such as unconditionally enabling the watchdog. Given that, I am not in favor of temporary changes.
Guenter
| |