[v1] x86/suspend: disable watchdog before calling console_start_sync()

[Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Igor Druzhinin 4 years, 2 months ago

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

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);
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Roger Pau Monné 4 years, 2 months ago

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Igor Druzhinin 4 years, 2 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Roger Pau Monné 4 years, 2 months ago

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Andrew Cooper 4 years, 2 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Jan Beulich 4 years, 2 months ago

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/suspend: disable watchdog before calling console_start_sync()

Posted by Igor Druzhinin 4 years, 2 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel