2021-09-28 16:57:35

by Thomas Weißschuh

Subject: [PATCH] watchdog: iTCO_wdt: Full reinitialize on resume

The Thinkpad T460s always needs driver-side suspend-resume handling.
If the watchdog is not stopped before suspend then the system will hang
on resume.
If the interval is not set before starting the watchdog then the machine
will instantly be reset after resume.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=198019

Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/watchdog/iTCO_wdt.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 643c6c2d0b72..2297a0a1e5fc 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -47,6 +47,7 @@
/* Includes */
#include <linux/acpi.h> /* For ACPI support */
#include <linux/bits.h> /* For BIT() */
+#include <linux/dmi.h> /* For DMI matching */
#include <linux/module.h> /* For module specific items */
#include <linux/moduleparam.h> /* For new moduleparam's */
#include <linux/types.h> /* For standard types (like size_t) */
@@ -605,9 +606,20 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
*/

#ifdef CONFIG_ACPI
+static const struct dmi_system_id iTCO_wdt_force_suspend[] = {
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad T460s"),
+ },
+ },
+ { },
+};
+
static inline bool need_suspend(void)
{
- return acpi_target_system_state() == ACPI_STATE_S0;
+ return acpi_target_system_state() == ACPI_STATE_S0 ||
+ dmi_check_system(iTCO_wdt_force_suspend);
}
#else
static inline bool need_suspend(void) { return true; }
@@ -631,8 +643,10 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
{
struct iTCO_wdt_private *p = dev_get_drvdata(dev);

- if (p->suspended)
+ if (p->suspended) {
+ iTCO_wdt_set_timeout(&p->wddev, p->wddev.timeout);
iTCO_wdt_start(&p->wddev);
+ }

return 0;
}

base-commit: 41e73feb1024929e75eaf2f7cd93f35a3feb331b
--
2.33.0

2021-10-02 13:25:23

by Guenter Roeck

Subject: Re: [PATCH] watchdog: iTCO_wdt: Full reinitialize on resume

On Tue, Sep 28, 2021 at 06:53:43PM +0200, Thomas Wei?schuh wrote:
> The Thinkpad T460s always needs driver-side suspend-resume handling.
> If the watchdog is not stopped before suspend then the system will hang
> on resume.
> If the interval is not set before starting the watchdog then the machine
> will instantly be reset after resume.
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=198019

The Fixes: tag references a SHA, not a bugzilla bug.
>
> Signed-off-by: Thomas Wei?schuh <[email protected]>
> ---
> drivers/watchdog/iTCO_wdt.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 643c6c2d0b72..2297a0a1e5fc 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -47,6 +47,7 @@
> /* Includes */
> #include <linux/acpi.h> /* For ACPI support */
> #include <linux/bits.h> /* For BIT() */
> +#include <linux/dmi.h> /* For DMI matching */
> #include <linux/module.h> /* For module specific items */
> #include <linux/moduleparam.h> /* For new moduleparam's */
> #include <linux/types.h> /* For standard types (like size_t) */
> @@ -605,9 +606,20 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> */
>
> #ifdef CONFIG_ACPI
> +static const struct dmi_system_id iTCO_wdt_force_suspend[] = {
> + {
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad T460s"),
> + },
> + },
> + { },
> +};
> +
> static inline bool need_suspend(void)
> {
> - return acpi_target_system_state() == ACPI_STATE_S0;
> + return acpi_target_system_state() == ACPI_STATE_S0 ||
> + dmi_check_system(iTCO_wdt_force_suspend);
> }
> #else
> static inline bool need_suspend(void) { return true; }
> @@ -631,8 +643,10 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
> {
> struct iTCO_wdt_private *p = dev_get_drvdata(dev);
>
> - if (p->suspended)
> + if (p->suspended) {
> + iTCO_wdt_set_timeout(&p->wddev, p->wddev.timeout);
> iTCO_wdt_start(&p->wddev);
> + }
>
> return 0;
> }
>
> base-commit: 41e73feb1024929e75eaf2f7cd93f35a3feb331b
> --
> 2.33.0
>

2021-10-02 14:02:38

by Thomas Weißschuh

Subject: Re: [PATCH] watchdog: iTCO_wdt: Full reinitialize on resume

On 2021-10-02T06:23-0700, Guenter Roeck wrote:
> On Tue, Sep 28, 2021 at 06:53:43PM +0200, Thomas Weißschuh wrote:
> > The Thinkpad T460s always needs driver-side suspend-resume handling.
> > If the watchdog is not stopped before suspend then the system will hang
> > on resume.
> > If the interval is not set before starting the watchdog then the machine
> > will instantly be reset after resume.
> >
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=198019
>
> The Fixes: tag references a SHA, not a bugzilla bug.

Thanks, I'll change that for v2.

> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> > drivers/watchdog/iTCO_wdt.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index 643c6c2d0b72..2297a0a1e5fc 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > @@ -47,6 +47,7 @@
> > /* Includes */
> > #include <linux/acpi.h> /* For ACPI support */
> > #include <linux/bits.h> /* For BIT() */
> > +#include <linux/dmi.h> /* For DMI matching */
> > #include <linux/module.h> /* For module specific items */
> > #include <linux/moduleparam.h> /* For new moduleparam's */
> > #include <linux/types.h> /* For standard types (like size_t) */
> > @@ -605,9 +606,20 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> > */
> >
> > #ifdef CONFIG_ACPI
> > +static const struct dmi_system_id iTCO_wdt_force_suspend[] = {
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > + DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad T460s"),
> > + },
> > + },
> > + { },
> > +};
> > +
> > static inline bool need_suspend(void)
> > {
> > - return acpi_target_system_state() == ACPI_STATE_S0;
> > + return acpi_target_system_state() == ACPI_STATE_S0 ||
> > + dmi_check_system(iTCO_wdt_force_suspend);
> > }
> > #else
> > static inline bool need_suspend(void) { return true; }
> > @@ -631,8 +643,10 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
> > {
> > struct iTCO_wdt_private *p = dev_get_drvdata(dev);
> >
> > - if (p->suspended)
> > + if (p->suspended) {
> > + iTCO_wdt_set_timeout(&p->wddev, p->wddev.timeout);
> > iTCO_wdt_start(&p->wddev);
> > + }
> >
> > return 0;
> > }
> >
> > base-commit: 41e73feb1024929e75eaf2f7cd93f35a3feb331b
> > --
> > 2.33.0

Is the current way with the dmi table the correct way to do it?

I'm also CC-ing Mark Person from Lenovo who may be able to take a look and
ask their firmware team to maybe fix this on their side.

Thomas