x86/suspend: disable watchdog before calling console_start_sync()
diff mbox series

Message ID 1580160556-25621-1-git-send-email-igor.druzhinin@citrix.com
State Superseded
Headers show
Series
  • x86/suspend: disable watchdog before calling console_start_sync()
Related show

Commit Message

Igor Druzhinin Jan. 27, 2020, 9:29 p.m. UTC
... and enable it after exiting S-state. Otherwise accumulated
output in serial buffer might easily trigger the watchdog if it's
still enabled after entering sync transmission mode.

The issue observed on machines which, unfortunately, generate non-0
output in CPU offline callbacks.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 xen/arch/x86/acpi/power.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Roger Pau Monne Jan. 28, 2020, 10:45 a.m. UTC | #1
On Mon, Jan 27, 2020 at 09:29:16PM +0000, Igor Druzhinin wrote:
> ... and enable it after exiting S-state. Otherwise accumulated
> output in serial buffer might easily trigger the watchdog if it's
> still enabled after entering sync transmission mode.

Can't you just process the watchdog in serial_start_sync instead of
disabling it?

Thanks, Roger.
Igor Druzhinin Jan. 28, 2020, 10:55 a.m. UTC | #2
On 28/01/2020 10:45, Roger Pau Monné wrote:
> On Mon, Jan 27, 2020 at 09:29:16PM +0000, Igor Druzhinin wrote:
>> ... and enable it after exiting S-state. Otherwise accumulated
>> output in serial buffer might easily trigger the watchdog if it's
>> still enabled after entering sync transmission mode.
> 
> Can't you just process the watchdog in serial_start_sync instead of
> disabling it?

I think it would be layering violation. Plus in every other place we're
enabling sync mode we're also disabling watchdog as well.

I also think it's beneficial to disable watchdog before entering firmware.

Igor
Roger Pau Monne Jan. 28, 2020, 11:32 a.m. UTC | #3
On Tue, Jan 28, 2020 at 10:55:00AM +0000, Igor Druzhinin wrote:
> On 28/01/2020 10:45, Roger Pau Monné wrote:
> > On Mon, Jan 27, 2020 at 09:29:16PM +0000, Igor Druzhinin wrote:
> >> ... and enable it after exiting S-state. Otherwise accumulated
> >> output in serial buffer might easily trigger the watchdog if it's
> >> still enabled after entering sync transmission mode.
> > 
> > Can't you just process the watchdog in serial_start_sync instead of
> > disabling it?
> 
> I think it would be layering violation. Plus in every other place we're
> enabling sync mode we're also disabling watchdog as well.

Not in every place, but indeed there's quite a lot of callers that
already disable the watchdog.

I wonder whether this should be put inside of console_start_sync
itself, and a parameter added to the function if not all callers want
the watchdog disabled (same for console_end_sync).

> I also think it's beneficial to disable watchdog before entering firmware.

I don't have objections.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.
Andrew Cooper Jan. 28, 2020, 11:38 a.m. UTC | #4
On 28/01/2020 11:32, Roger Pau Monné wrote:
> On Tue, Jan 28, 2020 at 10:55:00AM +0000, Igor Druzhinin wrote:
>> On 28/01/2020 10:45, Roger Pau Monné wrote:
>>> On Mon, Jan 27, 2020 at 09:29:16PM +0000, Igor Druzhinin wrote:
>>>> ... and enable it after exiting S-state. Otherwise accumulated
>>>> output in serial buffer might easily trigger the watchdog if it's
>>>> still enabled after entering sync transmission mode.
>>> Can't you just process the watchdog in serial_start_sync instead of
>>> disabling it?
>> I think it would be layering violation. Plus in every other place we're
>> enabling sync mode we're also disabling watchdog as well.
> Not in every place, but indeed there's quite a lot of callers that
> already disable the watchdog.
>
> I wonder whether this should be put inside of console_start_sync
> itself, and a parameter added to the function if not all callers want
> the watchdog disabled (same for console_end_sync).
>
>> I also think it's beneficial to disable watchdog before entering firmware.
> I don't have objections.
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Lets go with this for now.  It needs backporting, whereas API cleanup
probably wouldn't.

~Andrew
Jan Beulich Jan. 28, 2020, 1:39 p.m. UTC | #5
On 27.01.2020 22:29, Igor Druzhinin wrote:
> @@ -223,6 +224,7 @@ static int enter_state(u32 state)
>  
>      acpi_sleep_prepare(state);
>  
> +    watchdog_disable();
>      console_start_sync();
>      printk("Entering ACPI S%d state.\n", state);
>  
> @@ -281,6 +283,7 @@ static int enter_state(u32 state)
>          tboot_s3_error(error);
>  
>      console_end_sync();
> +    watchdog_enable();
>  
>      microcode_update_one(true);

Between these two there's a "goto done;" which also wants
watchdog_enable() added then.

Jan
Igor Druzhinin Jan. 28, 2020, 2:34 p.m. UTC | #6
On 28/01/2020 13:39, Jan Beulich wrote:
> On 27.01.2020 22:29, Igor Druzhinin wrote:
>> @@ -223,6 +224,7 @@ static int enter_state(u32 state)
>>  
>>      acpi_sleep_prepare(state);
>>  
>> +    watchdog_disable();
>>      console_start_sync();
>>      printk("Entering ACPI S%d state.\n", state);
>>  
>> @@ -281,6 +283,7 @@ static int enter_state(u32 state)
>>          tboot_s3_error(error);
>>  
>>      console_end_sync();
>> +    watchdog_enable();
>>  
>>      microcode_update_one(true);
> 
> Between these two there's a "goto done;" which also wants
> watchdog_enable() added then.

Indeed, thanks for noticing. Will send v2 shortly.

Igor

Patch
diff mbox series

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 8078352..62384a4 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -23,6 +23,7 @@ 
 #include <xen/domain.h>
 #include <xen/console.h>
 #include <xen/iommu.h>
+#include <xen/watchdog.h>
 #include <xen/cpu.h>
 #include <public/platform.h>
 #include <asm/tboot.h>
@@ -223,6 +224,7 @@  static int enter_state(u32 state)
 
     acpi_sleep_prepare(state);
 
+    watchdog_disable();
     console_start_sync();
     printk("Entering ACPI S%d state.\n", state);
 
@@ -281,6 +283,7 @@  static int enter_state(u32 state)
         tboot_s3_error(error);
 
     console_end_sync();
+    watchdog_enable();
 
     microcode_update_one(true);