lkml.org 
[lkml]   [2017]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog
From
Date
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

\
 
 \ /
  Last update: 2017-10-12 16:13    [W:0.050 / U:1.732 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site