Re:Re: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog

[刘稳 <18502523564@163.com>]

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.
>> > > +	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