| Message ID | 20200326150203.371673-2-s.riedmueller@phytec.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over | expand |
On 26 March 2020 15:02, Stefan Riedmueller wrote: > If the watchdog is already running during probe use its pre-configured > timeout instead of a default timeout to make sure the watchdog is pinged > in time until userspace takes over. At least for this driver I don't think there's an issue here with regards to not pinging in time. Calling 'da9063_wdt_update_timeout()', as it currently does in the probe() when the watchdog is already active, actually disables the watchdog before then setting a new timeout value, so by that method we're avoiding a timeout and starting a new timer period. To my mind the timeout value should come from DT if possible, which I would assume for the most part would match whatever is defined in the bootloader as well, unless I'm mistaken. If that's not available though then I would maybe agree on falling back to a value that was already programmed in the bootloader rather than the driver default which should be the last resort. > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > --- > drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > index 3d65e92a4e3f..34d0c4f03814 100644 > --- a/drivers/watchdog/da9063_wdt.c > +++ b/drivers/watchdog/da9063_wdt.c > @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned > int secs) > } > > /* > - * Return 0 if watchdog is disabled, else non zero. > + * Read the currently active timeout. > + * Zero means the watchdog is disabled. > */ > -static unsigned int da9063_wdt_is_running(struct da9063 *da9063) > +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063) > { > unsigned int val; > > regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val); > > - return val & DA9063_TWDSCALE_MASK; > + return wdt_timeout[val & DA9063_TWDSCALE_MASK]; > } > > static int da9063_wdt_disable_timer(struct da9063 *da9063) > @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device > *pdev) > struct device *dev = &pdev->dev; > struct da9063 *da9063; > struct watchdog_device *wdd; > + int timeout; > > if (!dev->parent) > return -EINVAL; > @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device > *pdev) > watchdog_set_restart_priority(wdd, 128); > watchdog_set_drvdata(wdd, da9063); > > - /* Set default timeout, maybe override it with DT value, scale it */ > - wdd->timeout = DA9063_WDG_TIMEOUT; > - watchdog_init_timeout(wdd, 0, dev); > - da9063_wdt_set_timeout(wdd, wdd->timeout); > - > - /* Change the timeout to the default value if the watchdog is running */ > - if (da9063_wdt_is_running(da9063)) { > - da9063_wdt_update_timeout(da9063, wdd->timeout); > + /* > + * Use pre-configured timeout if watchdog is already running. > + * Otherwise set default timeout, maybe override it with DT value, > + * scale it > + */ > + timeout = da9063_wdt_read_timeout(da9063); > + if (timeout) { > + wdd->timeout = timeout; > set_bit(WDOG_HW_RUNNING, &wdd->status); > + dev_info(da9063->dev, "watchdog is running (%u s)", timeout); > + } else { > + wdd->timeout = DA9063_WDG_TIMEOUT; > + watchdog_init_timeout(wdd, 0, dev); > + da9063_wdt_set_timeout(wdd, wdd->timeout); > } > > return devm_watchdog_register_device(dev, wdd); > -- > 2.23.0
On 3/30/20 9:38 AM, Adam Thomson wrote: > On 26 March 2020 15:02, Stefan Riedmueller wrote: > >> If the watchdog is already running during probe use its pre-configured >> timeout instead of a default timeout to make sure the watchdog is pinged >> in time until userspace takes over. > > At least for this driver I don't think there's an issue here with regards to > not pinging in time. Calling 'da9063_wdt_update_timeout()', as it currently > does in the probe() when the watchdog is already active, actually disables the > watchdog before then setting a new timeout value, so by that method we're > avoiding a timeout and starting a new timer period. > > To my mind the timeout value should come from DT if possible, which I would > assume for the most part would match whatever is defined in the bootloader as > well, unless I'm mistaken. If that's not available though then I would maybe > agree on falling back to a value that was already programmed in the bootloader > rather than the driver default which should be the last resort. > Agreed. Guenter >> >> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> >> --- >> drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++----------- >> 1 file changed, 18 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c >> index 3d65e92a4e3f..34d0c4f03814 100644 >> --- a/drivers/watchdog/da9063_wdt.c >> +++ b/drivers/watchdog/da9063_wdt.c >> @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned >> int secs) >> } >> >> /* >> - * Return 0 if watchdog is disabled, else non zero. >> + * Read the currently active timeout. >> + * Zero means the watchdog is disabled. >> */ >> -static unsigned int da9063_wdt_is_running(struct da9063 *da9063) >> +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063) >> { >> unsigned int val; >> >> regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val); >> >> - return val & DA9063_TWDSCALE_MASK; >> + return wdt_timeout[val & DA9063_TWDSCALE_MASK]; >> } >> >> static int da9063_wdt_disable_timer(struct da9063 *da9063) >> @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device >> *pdev) >> struct device *dev = &pdev->dev; >> struct da9063 *da9063; >> struct watchdog_device *wdd; >> + int timeout; >> >> if (!dev->parent) >> return -EINVAL; >> @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device >> *pdev) >> watchdog_set_restart_priority(wdd, 128); >> watchdog_set_drvdata(wdd, da9063); >> >> - /* Set default timeout, maybe override it with DT value, scale it */ >> - wdd->timeout = DA9063_WDG_TIMEOUT; >> - watchdog_init_timeout(wdd, 0, dev); >> - da9063_wdt_set_timeout(wdd, wdd->timeout); >> - >> - /* Change the timeout to the default value if the watchdog is running */ >> - if (da9063_wdt_is_running(da9063)) { >> - da9063_wdt_update_timeout(da9063, wdd->timeout); >> + /* >> + * Use pre-configured timeout if watchdog is already running. >> + * Otherwise set default timeout, maybe override it with DT value, >> + * scale it >> + */ >> + timeout = da9063_wdt_read_timeout(da9063); >> + if (timeout) { >> + wdd->timeout = timeout; >> set_bit(WDOG_HW_RUNNING, &wdd->status); >> + dev_info(da9063->dev, "watchdog is running (%u s)", timeout); >> + } else { >> + wdd->timeout = DA9063_WDG_TIMEOUT; >> + watchdog_init_timeout(wdd, 0, dev); >> + da9063_wdt_set_timeout(wdd, wdd->timeout); >> } >> >> return devm_watchdog_register_device(dev, wdd); >> -- >> 2.23.0 >
Hi Guenter, Adam, Guenter Roeck <linux@roeck-us.net> wrote on Tue, 31. Mar 20 09:08: > On 3/30/20 9:38 AM, Adam Thomson wrote: > > On 26 March 2020 15:02, Stefan Riedmueller wrote: > > > >> If the watchdog is already running during probe use its pre-configured > >> timeout instead of a default timeout to make sure the watchdog is pinged > >> in time until userspace takes over. > > > > At least for this driver I don't think there's an issue here with regards to > > not pinging in time. Calling 'da9063_wdt_update_timeout()', as it currently > > does in the probe() when the watchdog is already active, actually disables the > > watchdog before then setting a new timeout value, so by that method we're > > avoiding a timeout and starting a new timer period. > > > > To my mind the timeout value should come from DT if possible, which I would > > assume for the most part would match whatever is defined in the bootloader as > > well, unless I'm mistaken. If that's not available though then I would maybe > > agree on falling back to a value that was already programmed in the bootloader > > rather than the driver default which should be the last resort. > > > Agreed. Thanks for both your feedback. I'll drop the pre-configured timeout part and stick with the default timeout and do the same procedure (init_timeout + update_timeout) for the da9062. Thanks Stefan > > Guenter > > >> > >> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > >> --- > >> drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++----------- > >> 1 file changed, 18 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > >> index 3d65e92a4e3f..34d0c4f03814 100644 > >> --- a/drivers/watchdog/da9063_wdt.c > >> +++ b/drivers/watchdog/da9063_wdt.c > >> @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned > >> int secs) > >> } > >> > >> /* > >> - * Return 0 if watchdog is disabled, else non zero. > >> + * Read the currently active timeout. > >> + * Zero means the watchdog is disabled. > >> */ > >> -static unsigned int da9063_wdt_is_running(struct da9063 *da9063) > >> +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063) > >> { > >> unsigned int val; > >> > >> regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val); > >> > >> - return val & DA9063_TWDSCALE_MASK; > >> + return wdt_timeout[val & DA9063_TWDSCALE_MASK]; > >> } > >> > >> static int da9063_wdt_disable_timer(struct da9063 *da9063) > >> @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device > >> *pdev) > >> struct device *dev = &pdev->dev; > >> struct da9063 *da9063; > >> struct watchdog_device *wdd; > >> + int timeout; > >> > >> if (!dev->parent) > >> return -EINVAL; > >> @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device > >> *pdev) > >> watchdog_set_restart_priority(wdd, 128); > >> watchdog_set_drvdata(wdd, da9063); > >> > >> - /* Set default timeout, maybe override it with DT value, scale it */ > >> - wdd->timeout = DA9063_WDG_TIMEOUT; > >> - watchdog_init_timeout(wdd, 0, dev); > >> - da9063_wdt_set_timeout(wdd, wdd->timeout); > >> - > >> - /* Change the timeout to the default value if the watchdog is running */ > >> - if (da9063_wdt_is_running(da9063)) { > >> - da9063_wdt_update_timeout(da9063, wdd->timeout); > >> + /* > >> + * Use pre-configured timeout if watchdog is already running. > >> + * Otherwise set default timeout, maybe override it with DT value, > >> + * scale it > >> + */ > >> + timeout = da9063_wdt_read_timeout(da9063); > >> + if (timeout) { > >> + wdd->timeout = timeout; > >> set_bit(WDOG_HW_RUNNING, &wdd->status); > >> + dev_info(da9063->dev, "watchdog is running (%u s)", timeout); > >> + } else { > >> + wdd->timeout = DA9063_WDG_TIMEOUT; > >> + watchdog_init_timeout(wdd, 0, dev); > >> + da9063_wdt_set_timeout(wdd, wdd->timeout); > >> } > >> > >> return devm_watchdog_register_device(dev, wdd); > >> -- > >> 2.23.0 > > >
diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c index 3d65e92a4e3f..34d0c4f03814 100644 --- a/drivers/watchdog/da9063_wdt.c +++ b/drivers/watchdog/da9063_wdt.c @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs) } /* - * Return 0 if watchdog is disabled, else non zero. + * Read the currently active timeout. + * Zero means the watchdog is disabled. */ -static unsigned int da9063_wdt_is_running(struct da9063 *da9063) +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063) { unsigned int val; regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val); - return val & DA9063_TWDSCALE_MASK; + return wdt_timeout[val & DA9063_TWDSCALE_MASK]; } static int da9063_wdt_disable_timer(struct da9063 *da9063) @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct da9063 *da9063; struct watchdog_device *wdd; + int timeout; if (!dev->parent) return -EINVAL; @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device *pdev) watchdog_set_restart_priority(wdd, 128); watchdog_set_drvdata(wdd, da9063); - /* Set default timeout, maybe override it with DT value, scale it */ - wdd->timeout = DA9063_WDG_TIMEOUT; - watchdog_init_timeout(wdd, 0, dev); - da9063_wdt_set_timeout(wdd, wdd->timeout); - - /* Change the timeout to the default value if the watchdog is running */ - if (da9063_wdt_is_running(da9063)) { - da9063_wdt_update_timeout(da9063, wdd->timeout); + /* + * Use pre-configured timeout if watchdog is already running. + * Otherwise set default timeout, maybe override it with DT value, + * scale it + */ + timeout = da9063_wdt_read_timeout(da9063); + if (timeout) { + wdd->timeout = timeout; set_bit(WDOG_HW_RUNNING, &wdd->status); + dev_info(da9063->dev, "watchdog is running (%u s)", timeout); + } else { + wdd->timeout = DA9063_WDG_TIMEOUT; + watchdog_init_timeout(wdd, 0, dev); + da9063_wdt_set_timeout(wdd, wdd->timeout); } return devm_watchdog_register_device(dev, wdd);
If the watchdog is already running during probe use its pre-configured timeout instead of a default timeout to make sure the watchdog is pinged in time until userspace takes over. Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> --- drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)