Re: [RFC PATCH 01/12] xen/manage: keep track of the on-going suspend mode

[Balbir Singh]

On Wed, Jun 13, 2018 at 6:56 AM, Anchal Agarwal <anchalag@xxxxxxxxxx> wrote:
> From: Munehisa Kamata <kamatam@xxxxxxxxxx>
>
> To differentiate between Xen suspend, PM suspend and PM hibernation,
> keep track of the on-going suspend mode by mainly using a new PM
> notifier. Since Xen suspend doesn't have corresponding PM event, its
> main logic is modfied to acquire pm_mutex and set the current mode.
>

Why do we need to differentiate between them? The changelog does not
explain how Xen Suspend is different from PM suspend. The difference
could be what is injected into the guest vs what the guest decides to
do. How do we use the new suspend_mode?

> Note that we may see deadlock if PM suspend/hibernation is interrupted
> by Xen suspend. PM suspend/hibernation depends on xenwatch thread to
> process xenbus state transactions, but the thread will sleep to wait
> pm_mutex which is already held by PM suspend/hibernation context in the
> scenario. Though, acquirng pm_mutex is still right thing to do, and we
> would need to modify Xen shutdown code to avoid the issue. This will be
> fixed by a separate patch.
>
> Signed-off-by: Munehisa Kamata <kamatam@xxxxxxxxxx>
> Signed-off-by: Anchal Agarwal <anchalag@xxxxxxxxxx>
> Reviewed-by: Sebastian Biemueller <sbiemue@xxxxxxxxxx>
> Reviewed-by: Munehisa Kamata <kamatam@xxxxxxxxxx>
> Reviewed-by: Eduardo Valentin <eduval@xxxxxxxxxx>
> ---
>  drivers/xen/manage.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 8835065..8f9ea87 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -13,6 +13,7 @@
>  #include <linux/freezer.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/export.h>
> +#include <linux/suspend.h>
>
>  #include <xen/xen.h>
>  #include <xen/xenbus.h>
> @@ -39,6 +40,16 @@ enum shutdown_state {
>  /* Ignore multiple shutdown requests. */
>  static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
>
> +enum suspend_modes {
> +       NO_SUSPEND = 0,
> +       XEN_SUSPEND,
> +       PM_SUSPEND,
> +       PM_HIBERNATION,
> +};


Why do the enums range across namespaces -- between NO, XEN and PM?
Can we please be consistent XEN_WATCH_SUSPEND, XEN_PM_SUSPEND. etc?

Balbir Singh.