| Message ID | 1580160556-25621-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/suspend: disable watchdog before calling console_start_sync() | expand |
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.
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
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.
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
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
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
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);
... 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(+)