At 2017-10-10 23:05:14, "Guenter Roeck" <linux@xxxxxxxxxxxx> 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@xxxxxxx>
>> > > ---
>> > > 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
