diff mbox series

watchdog: iTCO_wdt: Full reinitialize on resume

Message ID 20210928165343.23401-1-linux@weissschuh.net (mailing list archive)
State Changes Requested
Headers show
Series watchdog: iTCO_wdt: Full reinitialize on resume | expand

Commit Message

Thomas Weißschuh Sept. 28, 2021, 4:53 p.m. UTC 
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 <linux@weissschuh.net>
---
 drivers/watchdog/iTCO_wdt.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)


base-commit: 41e73feb1024929e75eaf2f7cd93f35a3feb331b

Comments

Guenter Roeck Oct. 2, 2021, 1:23 p.m. UTC | #1 
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 <linux@weissschuh.net>
> ---
>  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
>
Thomas Weißschuh Oct. 2, 2021, 1:43 p.m. UTC | #2 
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 <linux@weissschuh.net>
> > ---
> >  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
diff mbox series

Patch

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;
 }