* [PATCH] pvops: fix "xm save -c" issue @ 2010-12-14 5:31 Kenji Wakamiya 2010-12-14 14:59 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 12+ messages in thread From: Kenji Wakamiya @ 2010-12-14 5:31 UTC (permalink / raw) To: xen-devel [-- Attachment #1: Type: text/plain, Size: 341 bytes --] Hi, I'm investigating the issue of "xm save -c" in case of PV guests. Then, I tried to prevent calling dpm_resume_end() when suspend was canceled. It seems to work fine about blk and net. How about this? Thanks, Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> -- Kenji Wakamiya [-- Attachment #2: xen-pvops-dpm.diff --] [-- Type: text/plain, Size: 413 bytes --] diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c index 0b50906..3dcc270 100644 --- a/drivers/xen/manage.c +++ b/drivers/xen/manage.c @@ -148,11 +148,10 @@ out_resume: if (!cancelled) { xen_arch_resume(); xs_resume(); + dpm_resume_end(PMSG_RESUME); } else xs_suspend_cancel(); - dpm_resume_end(PMSG_RESUME); - /* Make sure timer events get retriggered on all CPUs */ clock_was_set(); [-- Attachment #3: Type: text/plain, Size: 138 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2010-12-14 5:31 [PATCH] pvops: fix "xm save -c" issue Kenji Wakamiya @ 2010-12-14 14:59 ` Konrad Rzeszutek Wilk 2010-12-15 4:18 ` Kenji Wakamiya 0 siblings, 1 reply; 12+ messages in thread From: Konrad Rzeszutek Wilk @ 2010-12-14 14:59 UTC (permalink / raw) To: Kenji Wakamiya; +Cc: xen-devel On Tue, Dec 14, 2010 at 02:31:36PM +0900, Kenji Wakamiya wrote: > Hi, > > I'm investigating the issue of "xm save -c" in case of PV guests. > Then, I tried to prevent calling dpm_resume_end() when suspend was > canceled. It seems to work fine about blk and net. > How about this? Could you give more details of what the failure is? > > Thanks, > > Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> > Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> > -- > Kenji Wakamiya > > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c > index 0b50906..3dcc270 100644 > --- a/drivers/xen/manage.c > +++ b/drivers/xen/manage.c > @@ -148,11 +148,10 @@ out_resume: > if (!cancelled) { > xen_arch_resume(); > xs_resume(); > + dpm_resume_end(PMSG_RESUME); > } else > xs_suspend_cancel(); > > - dpm_resume_end(PMSG_RESUME); > - > /* Make sure timer events get retriggered on all CPUs */ > clock_was_set(); > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2010-12-14 14:59 ` Konrad Rzeszutek Wilk @ 2010-12-15 4:18 ` Kenji Wakamiya 2010-12-15 15:40 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 12+ messages in thread From: Kenji Wakamiya @ 2010-12-15 4:18 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel Hi Konrad, thank you for your response, Konrad Rzeszutek Wilk wrote: > On Tue, Dec 14, 2010 at 02:31:36PM +0900, Kenji Wakamiya wrote: >> Hi, >> >> I'm investigating the issue of "xm save -c" in case of PV guests. >> Then, I tried to prevent calling dpm_resume_end() when suspend was >> canceled. It seems to work fine about blk and net. >> How about this? > > > Could you give more details of what the failure is? "xm save -c" saves domain state to storage and leaves it running after creating snapshot. When this command is executed for a pvops guest (xen/stable-2.6.32.x), the guest will hang up after accessing to devices. When the guest is unapused, stop_machine() in manage.c returns as the suspend is cancelled. In that case, I think dpm_resume_end() should not be called after stop_machine(). I tested vbd and net, the guest did not hang. But I'm not sure if this is a right way... Thanks, >> Thanks, >> >> Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> >> Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> >> -- >> Kenji Wakamiya >> > >> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c >> index 0b50906..3dcc270 100644 >> --- a/drivers/xen/manage.c >> +++ b/drivers/xen/manage.c >> @@ -148,11 +148,10 @@ out_resume: >> if (!cancelled) { >> xen_arch_resume(); >> xs_resume(); >> + dpm_resume_end(PMSG_RESUME); >> } else >> xs_suspend_cancel(); >> >> - dpm_resume_end(PMSG_RESUME); >> - >> /* Make sure timer events get retriggered on all CPUs */ >> clock_was_set(); >> > >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2010-12-15 4:18 ` Kenji Wakamiya @ 2010-12-15 15:40 ` Konrad Rzeszutek Wilk 2010-12-16 5:49 ` Kenji Wakamiya 0 siblings, 1 reply; 12+ messages in thread From: Konrad Rzeszutek Wilk @ 2010-12-15 15:40 UTC (permalink / raw) To: Kenji Wakamiya; +Cc: xen-devel On Wed, Dec 15, 2010 at 01:18:06PM +0900, Kenji Wakamiya wrote: > Hi Konrad, thank you for your response, > > Konrad Rzeszutek Wilk wrote: > >On Tue, Dec 14, 2010 at 02:31:36PM +0900, Kenji Wakamiya wrote: > >>Hi, > >> > >>I'm investigating the issue of "xm save -c" in case of PV guests. > >>Then, I tried to prevent calling dpm_resume_end() when suspend was > >>canceled. It seems to work fine about blk and net. > >>How about this? > > > > > >Could you give more details of what the failure is? > > "xm save -c" saves domain state to storage and leaves it running after > creating snapshot. When this command is executed for a pvops > guest (xen/stable-2.6.32.x), the guest will hang up after accessing to > devices. > > When the guest is unapused, stop_machine() in manage.c returns as the > suspend is cancelled. In that case, I think dpm_resume_end() should > not be called after stop_machine(). > > I tested vbd and net, the guest did not hang. OK. Does 'xm save' (so no -c) and then resume work with this patch (I would think so, but I am curious whether you tested this). > But I'm not sure if this is a right way... Looks OK to me, just want to make sure that the normal 'xm save' still works after this. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2010-12-15 15:40 ` Konrad Rzeszutek Wilk @ 2010-12-16 5:49 ` Kenji Wakamiya 2010-12-17 9:43 ` Ian Campbell 0 siblings, 1 reply; 12+ messages in thread From: Kenji Wakamiya @ 2010-12-16 5:49 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel Hi Konrad, Konrad Rzeszutek Wilk wrote: >> When the guest is unapused, stop_machine() in manage.c returns as the >> suspend is cancelled. In that case, I think dpm_resume_end() should >> not be called after stop_machine(). >> >> I tested vbd and net, the guest did not hang. > > OK. Does 'xm save' (so no -c) and then resume work with this patch > (I would think so, but I am curious whether you tested this). > >> But I'm not sure if this is a right way... > > Looks OK to me, just want to make sure that the normal 'xm save' still > works after this. I tested normal 'xm save' with this patch, and made sure it works well. When normal save/restore is performed, the domain is suspended without cancel, unlike 'save -c'. So, dpm_resume_end() is called. This scenario haven't changed even after applying this patch. Thanks, -- Kenji Wakamiya ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2010-12-16 5:49 ` Kenji Wakamiya @ 2010-12-17 9:43 ` Ian Campbell 2011-01-10 17:01 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 12+ messages in thread From: Ian Campbell @ 2010-12-17 9:43 UTC (permalink / raw) To: Kenji Wakamiya; +Cc: xen-devel, Konrad Rzeszutek Wilk On Thu, 2010-12-16 at 05:49 +0000, Kenji Wakamiya wrote: > Hi Konrad, > > Konrad Rzeszutek Wilk wrote: > >> When the guest is unapused, stop_machine() in manage.c returns as the > >> suspend is cancelled. In that case, I think dpm_resume_end() should > >> not be called after stop_machine(). > >> > >> I tested vbd and net, the guest did not hang. > > > > OK. Does 'xm save' (so no -c) and then resume work with this patch > > (I would think so, but I am curious whether you tested this). > > > >> But I'm not sure if this is a right way... > > > > Looks OK to me, just want to make sure that the normal 'xm save' still > > works after this. > > I tested normal 'xm save' with this patch, and made sure it works well. > > When normal save/restore is performed, the domain is suspended without > cancel, unlike 'save -c'. So, dpm_resume_end() is called. This > scenario haven't changed even after applying this patch. With this change how is the effect of dpm_suspend_start undone in the suspend cancelled case? Currently we have dpm_suspend_start(PMSG_SUSPEND) xs_suspend dpm_suspend_noirq(PMSG_SUSPEND) SUSPEND dpm_resume_noirq(PMSG_RESUME) xs_resume or xs_supend_cancel dpm_resume_end(PMSG_RESUME) Which seems nicely nested and logical but by only calling dpm_resume_end in the non-cancelled case we seem to be unbalancing things. Do we need some sort of dpm_resume_cancel, or some way of pushing the cancelled flag down into the individual xenbus_device.resume handlers? Should we maybe simply be using a difference PMSG_XXX in the cancelled case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? Looks like to propagate the PMSG_* to the actual device resume functions we would need to provide a pm_ops for the struct bus xenbus_frontend instead of relying on the legacy handlers. This is probably a independently good idea anyway. Ian. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2010-12-17 9:43 ` Ian Campbell @ 2011-01-10 17:01 ` Konrad Rzeszutek Wilk 2011-01-21 5:35 ` Kenji Wakamiya 0 siblings, 1 reply; 12+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-01-10 17:01 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Kenji Wakamiya On Fri, Dec 17, 2010 at 09:43:16AM +0000, Ian Campbell wrote: > On Thu, 2010-12-16 at 05:49 +0000, Kenji Wakamiya wrote: > > Hi Konrad, > > > > Konrad Rzeszutek Wilk wrote: > > >> When the guest is unapused, stop_machine() in manage.c returns as the > > >> suspend is cancelled. In that case, I think dpm_resume_end() should > > >> not be called after stop_machine(). > > >> > > >> I tested vbd and net, the guest did not hang. > > > > > > OK. Does 'xm save' (so no -c) and then resume work with this patch > > > (I would think so, but I am curious whether you tested this). > > > > > >> But I'm not sure if this is a right way... > > > > > > Looks OK to me, just want to make sure that the normal 'xm save' still > > > works after this. > > > > I tested normal 'xm save' with this patch, and made sure it works well. > > > > When normal save/restore is performed, the domain is suspended without > > cancel, unlike 'save -c'. So, dpm_resume_end() is called. This > > scenario haven't changed even after applying this patch. > > With this change how is the effect of dpm_suspend_start undone in the > suspend cancelled case? > > Currently we have > dpm_suspend_start(PMSG_SUSPEND) > xs_suspend > dpm_suspend_noirq(PMSG_SUSPEND) > SUSPEND > dpm_resume_noirq(PMSG_RESUME) > xs_resume or xs_supend_cancel > dpm_resume_end(PMSG_RESUME) > > Which seems nicely nested and logical but by only calling dpm_resume_end > in the non-cancelled case we seem to be unbalancing things. > > Do we need some sort of dpm_resume_cancel, or some way of pushing the > cancelled flag down into the individual xenbus_device.resume handlers? > > Should we maybe simply be using a difference PMSG_XXX in the cancelled > case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? > > Looks like to propagate the PMSG_* to the actual device resume functions > we would need to provide a pm_ops for the struct bus xenbus_frontend > instead of relying on the legacy handlers. This is probably a > independently good idea anyway. ping? Kenji any ideas or patches to address Ian's comments? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2011-01-10 17:01 ` Konrad Rzeszutek Wilk @ 2011-01-21 5:35 ` Kenji Wakamiya 2011-01-21 6:28 ` SUZUKI, Kazuhiro 0 siblings, 1 reply; 12+ messages in thread From: Kenji Wakamiya @ 2011-01-21 5:35 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel, Ian Campbell Hi Konrand, and sorry for very late response. (2011/01/11 2:01), Konrad Rzeszutek Wilk wrote: >> With this change how is the effect of dpm_suspend_start undone in the >> suspend cancelled case? >> >> Currently we have >> dpm_suspend_start(PMSG_SUSPEND) >> xs_suspend >> dpm_suspend_noirq(PMSG_SUSPEND) >> SUSPEND >> dpm_resume_noirq(PMSG_RESUME) >> xs_resume or xs_supend_cancel >> dpm_resume_end(PMSG_RESUME) >> >> Which seems nicely nested and logical but by only calling dpm_resume_end >> in the non-cancelled case we seem to be unbalancing things. >> >> Do we need some sort of dpm_resume_cancel, or some way of pushing the >> cancelled flag down into the individual xenbus_device.resume handlers? >> >> Should we maybe simply be using a difference PMSG_XXX in the cancelled >> case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? >> >> Looks like to propagate the PMSG_* to the actual device resume functions >> we would need to provide a pm_ops for the struct bus xenbus_frontend >> instead of relying on the legacy handlers. This is probably a >> independently good idea anyway. > > ping? > > Kenji any ideas or patches to address Ian's comments? My colleague made a patch which reflected Ian's comments, so I will ask him to post it. Please wait a little. Thanks, Kenji ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2011-01-21 5:35 ` Kenji Wakamiya @ 2011-01-21 6:28 ` SUZUKI, Kazuhiro 2011-01-21 9:14 ` Ian Campbell 0 siblings, 1 reply; 12+ messages in thread From: SUZUKI, Kazuhiro @ 2011-01-21 6:28 UTC (permalink / raw) To: xen-devel; +Cc: Ian.Campbell, konrad.wilk [-- Attachment #1: Type: Text/Plain, Size: 2020 bytes --] Hi, The following patch fixes 'xm save -c' issue. We defined 'PMSG_CANCEL' message for suspend cancel situation and suspend_cancel handler in pm_ops struct. If the suspend_cancel is defined, suspend_cancel() is called instead of resume(). Thanks, KAZ Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> From: Kenji Wakamiya <wkenji@jp.fujitsu.com> Subject: Re: [Xen-devel] [PATCH] pvops: fix "xm save -c" issue Date: Fri, 21 Jan 2011 14:35:09 +0900 > Hi Konrand, and sorry for very late response. > > (2011/01/11 2:01), Konrad Rzeszutek Wilk wrote: >>> With this change how is the effect of dpm_suspend_start undone in the >>> suspend cancelled case? >>> >>> Currently we have >>> dpm_suspend_start(PMSG_SUSPEND) >>> xs_suspend >>> dpm_suspend_noirq(PMSG_SUSPEND) >>> SUSPEND >>> dpm_resume_noirq(PMSG_RESUME) >>> xs_resume or xs_supend_cancel >>> dpm_resume_end(PMSG_RESUME) >>> >>> Which seems nicely nested and logical but by only calling dpm_resume_end >>> in the non-cancelled case we seem to be unbalancing things. >>> >>> Do we need some sort of dpm_resume_cancel, or some way of pushing the >>> cancelled flag down into the individual xenbus_device.resume handlers? >>> >>> Should we maybe simply be using a difference PMSG_XXX in the cancelled >>> case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? >>> >>> Looks like to propagate the PMSG_* to the actual device resume functions >>> we would need to provide a pm_ops for the struct bus xenbus_frontend >>> instead of relying on the legacy handlers. This is probably a >>> independently good idea anyway. >> >> ping? >> >> Kenji any ideas or patches to address Ian's comments? > > My colleague made a patch which reflected Ian's comments, so I will ask > him to post it. Please wait a little. > > Thanks, > Kenji > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel [-- Attachment #2: fix-checkpoint.patch --] [-- Type: Text/X-Patch, Size: 6186 bytes --] diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 8aa2443..b743e8f 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -181,6 +181,13 @@ static int pm_op(struct device *dev, suspend_report_result(ops->suspend, error); } break; + case PM_EVENT_CANCEL: + if (ops->suspend_cancel) { + error = ops->suspend_cancel(dev); + suspend_report_result(ops->suspend_cancel, error); + break; + } + /* Fall through */ case PM_EVENT_RESUME: if (ops->resume) { error = ops->resume(dev); @@ -291,6 +298,8 @@ static char *pm_verb(int event) switch (event) { case PM_EVENT_SUSPEND: return "suspend"; + case PM_EVENT_CANCEL: + return "cancel"; case PM_EVENT_RESUME: return "resume"; case PM_EVENT_FREEZE: diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 3f71199..22c6288 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -1293,7 +1293,7 @@ static void xennet_disconnect_backend(struct netfront_info *info) info->rx.sring = NULL; } -static int netfront_suspend(struct xenbus_device *dev, pm_message_t state) +static int netfront_suspend(struct xenbus_device *dev) { struct netfront_info *info = dev_get_drvdata(&dev->dev); struct hrtimer *timer = &info->smart_poll.timer; diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c index 0b50906..845afb8 100644 --- a/drivers/xen/manage.c +++ b/drivers/xen/manage.c @@ -148,10 +148,11 @@ out_resume: if (!cancelled) { xen_arch_resume(); xs_resume(); - } else + dpm_resume_end(PMSG_RESUME); + } else { xs_suspend_cancel(); - - dpm_resume_end(PMSG_RESUME); + dpm_resume_end(PMSG_CANCEL); + } /* Make sure timer events get retriggered on all CPUs */ clock_was_set(); diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index 3a83ba2..019337a 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -575,7 +575,7 @@ void xenbus_dev_changed(const char *node, struct xen_bus_type *bus) } EXPORT_SYMBOL_GPL(xenbus_dev_changed); -int xenbus_dev_suspend(struct device *dev, pm_message_t state) +int xenbus_dev_suspend(struct device *dev) { int err = 0; struct xenbus_driver *drv; @@ -587,7 +587,7 @@ int xenbus_dev_suspend(struct device *dev, pm_message_t state) return 0; drv = to_xenbus_driver(dev->driver); if (drv->suspend) - err = drv->suspend(xdev, state); + err = drv->suspend(xdev); if (err) printk(KERN_WARNING "xenbus: suspend %s failed: %i\n", dev_name(dev), err); @@ -595,6 +595,14 @@ int xenbus_dev_suspend(struct device *dev, pm_message_t state) } EXPORT_SYMBOL_GPL(xenbus_dev_suspend); +int xenbus_dev_suspend_cancel(struct device *dev) +{ + /* Do nothing */ + DPRINTK("cancel"); + return 0; +} +EXPORT_SYMBOL_GPL(xenbus_dev_suspend_cancel); + int xenbus_dev_resume(struct device *dev) { int err; diff --git a/drivers/xen/xenbus/xenbus_probe.h b/drivers/xen/xenbus/xenbus_probe.h index 0e5fc4c..bf4a793 100644 --- a/drivers/xen/xenbus/xenbus_probe.h +++ b/drivers/xen/xenbus/xenbus_probe.h @@ -62,7 +62,8 @@ extern void xenbus_dev_changed(const char *node, struct xen_bus_type *bus); extern void xenbus_dev_shutdown(struct device *_dev); -extern int xenbus_dev_suspend(struct device *dev, pm_message_t state); +extern int xenbus_dev_suspend(struct device *dev); +extern int xenbus_dev_suspend_cancel(struct device *dev); extern int xenbus_dev_resume(struct device *dev); extern void xenbus_otherend_changed(struct xenbus_watch *watch, diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c index 5413248..928fc10 100644 --- a/drivers/xen/xenbus/xenbus_probe_frontend.c +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c @@ -82,6 +82,11 @@ static struct device_attribute xenbus_frontend_dev_attrs[] = { __ATTR_NULL }; +static struct dev_pm_ops xenbus_pm_ops = { + .suspend = xenbus_dev_suspend, + .suspend_cancel = xenbus_dev_suspend_cancel, + .resume = xenbus_dev_resume, +}; static struct xen_bus_type xenbus_frontend = { .root = "device", @@ -98,8 +103,7 @@ static struct xen_bus_type xenbus_frontend = { .shutdown = xenbus_dev_shutdown, .dev_attrs= xenbus_frontend_dev_attrs, - .suspend = xenbus_dev_suspend, - .resume = xenbus_dev_resume, + .pm = &xenbus_pm_ops, }, }; diff --git a/include/linux/pm.h b/include/linux/pm.h index 3b7e04b..aa89839 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -197,6 +197,7 @@ struct dev_pm_ops { int (*prepare)(struct device *dev); void (*complete)(struct device *dev); int (*suspend)(struct device *dev); + int (*suspend_cancel)(struct device *dev); int (*resume)(struct device *dev); int (*freeze)(struct device *dev); int (*thaw)(struct device *dev); @@ -291,6 +292,7 @@ struct dev_pm_ops name = { \ #define PM_EVENT_USER 0x0100 #define PM_EVENT_REMOTE 0x0200 #define PM_EVENT_AUTO 0x0400 +#define PM_EVENT_CANCEL 0x0800 #define PM_EVENT_SLEEP (PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE) #define PM_EVENT_USER_SUSPEND (PM_EVENT_USER | PM_EVENT_SUSPEND) @@ -308,6 +310,7 @@ struct dev_pm_ops name = { \ #define PMSG_THAW ((struct pm_message){ .event = PM_EVENT_THAW, }) #define PMSG_RESTORE ((struct pm_message){ .event = PM_EVENT_RESTORE, }) #define PMSG_RECOVER ((struct pm_message){ .event = PM_EVENT_RECOVER, }) +#define PMSG_CANCEL ((struct pm_message){ .event = PM_EVENT_CANCEL, }) #define PMSG_USER_SUSPEND ((struct pm_message) \ { .event = PM_EVENT_USER_SUSPEND, }) #define PMSG_USER_RESUME ((struct pm_message) \ diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index 542ca7c..23e7f25 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -91,7 +91,7 @@ struct xenbus_driver { void (*otherend_changed)(struct xenbus_device *dev, enum xenbus_state backend_state); int (*remove)(struct xenbus_device *dev); - int (*suspend)(struct xenbus_device *dev, pm_message_t state); + int (*suspend)(struct xenbus_device *dev); int (*resume)(struct xenbus_device *dev); int (*uevent)(struct xenbus_device *, struct kobj_uevent_env *); struct device_driver driver; [-- Attachment #3: Type: text/plain, Size: 138 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2011-01-21 6:28 ` SUZUKI, Kazuhiro @ 2011-01-21 9:14 ` Ian Campbell 2011-01-25 7:23 ` SUZUKI, Kazuhiro 0 siblings, 1 reply; 12+ messages in thread From: Ian Campbell @ 2011-01-21 9:14 UTC (permalink / raw) To: SUZUKI, Kazuhiro; +Cc: xen-devel, konrad.wilk On Fri, 2011-01-21 at 06:28 +0000, SUZUKI, Kazuhiro wrote: > Hi, > > The following patch fixes 'xm save -c' issue. > We defined 'PMSG_CANCEL' message for suspend cancel situation and > suspend_cancel handler in pm_ops struct. > If the suspend_cancel is defined, suspend_cancel() is called instead > of resume(). Thanks. I like the general shape of this patch but the core pm infrastructure change needs to be split out and sent upstream via the power management maintainer. I think this is Rafael J. Wysocki <rjw@sisk.pl> and the linux-pm@lists.linux-foundation.org list. If you also split out the Xen conversion to dev_pm_ops, without the suspend_cancel bit, then I think we can take that bit now and then you can resend the bit which adds the suspend_cancel bits once the core stuff is upstream. Ian. > > Thanks, > KAZ > > Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> > Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> > > > From: Kenji Wakamiya <wkenji@jp.fujitsu.com> > Subject: Re: [Xen-devel] [PATCH] pvops: fix "xm save -c" issue > Date: Fri, 21 Jan 2011 14:35:09 +0900 > > > Hi Konrand, and sorry for very late response. > > > > (2011/01/11 2:01), Konrad Rzeszutek Wilk wrote: > >>> With this change how is the effect of dpm_suspend_start undone in the > >>> suspend cancelled case? > >>> > >>> Currently we have > >>> dpm_suspend_start(PMSG_SUSPEND) > >>> xs_suspend > >>> dpm_suspend_noirq(PMSG_SUSPEND) > >>> SUSPEND > >>> dpm_resume_noirq(PMSG_RESUME) > >>> xs_resume or xs_supend_cancel > >>> dpm_resume_end(PMSG_RESUME) > >>> > >>> Which seems nicely nested and logical but by only calling dpm_resume_end > >>> in the non-cancelled case we seem to be unbalancing things. > >>> > >>> Do we need some sort of dpm_resume_cancel, or some way of pushing the > >>> cancelled flag down into the individual xenbus_device.resume handlers? > >>> > >>> Should we maybe simply be using a difference PMSG_XXX in the cancelled > >>> case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? > >>> > >>> Looks like to propagate the PMSG_* to the actual device resume functions > >>> we would need to provide a pm_ops for the struct bus xenbus_frontend > >>> instead of relying on the legacy handlers. This is probably a > >>> independently good idea anyway. > >> > >> ping? > >> > >> Kenji any ideas or patches to address Ian's comments? > > > > My colleague made a patch which reflected Ian's comments, so I will ask > > him to post it. Please wait a little. > > > > Thanks, > > Kenji > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2011-01-21 9:14 ` Ian Campbell @ 2011-01-25 7:23 ` SUZUKI, Kazuhiro 2011-01-25 16:32 ` Ian Campbell 0 siblings, 1 reply; 12+ messages in thread From: SUZUKI, Kazuhiro @ 2011-01-25 7:23 UTC (permalink / raw) To: Ian.Campbell; +Cc: xen-devel, konrad.wilk [-- Attachment #1: Type: Text/Plain, Size: 3212 bytes --] Hi Ian, I split the patch into pm infrastructure part and xen part. Actually, I've never post to linux community, so I'm not sure what to do. Thanks, KAZ Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> From: Ian Campbell <Ian.Campbell@eu.citrix.com> Subject: Re: [Xen-devel] [PATCH] pvops: fix "xm save -c" issue Date: Fri, 21 Jan 2011 09:14:30 +0000 > On Fri, 2011-01-21 at 06:28 +0000, SUZUKI, Kazuhiro wrote: >> Hi, >> >> The following patch fixes 'xm save -c' issue. >> We defined 'PMSG_CANCEL' message for suspend cancel situation and >> suspend_cancel handler in pm_ops struct. >> If the suspend_cancel is defined, suspend_cancel() is called instead >> of resume(). > > Thanks. I like the general shape of this patch but the core pm > infrastructure change needs to be split out and sent upstream via the > power management maintainer. I think this is Rafael J. Wysocki > <rjw@sisk.pl> and the linux-pm@lists.linux-foundation.org list. > > If you also split out the Xen conversion to dev_pm_ops, without the > suspend_cancel bit, then I think we can take that bit now and then you > can resend the bit which adds the suspend_cancel bits once the core > stuff is upstream. > > Ian. > >> >> Thanks, >> KAZ >> >> Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> >> Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> >> >> >> From: Kenji Wakamiya <wkenji@jp.fujitsu.com> >> Subject: Re: [Xen-devel] [PATCH] pvops: fix "xm save -c" issue >> Date: Fri, 21 Jan 2011 14:35:09 +0900 >> >> > Hi Konrand, and sorry for very late response. >> > >> > (2011/01/11 2:01), Konrad Rzeszutek Wilk wrote: >> >>> With this change how is the effect of dpm_suspend_start undone in the >> >>> suspend cancelled case? >> >>> >> >>> Currently we have >> >>> dpm_suspend_start(PMSG_SUSPEND) >> >>> xs_suspend >> >>> dpm_suspend_noirq(PMSG_SUSPEND) >> >>> SUSPEND >> >>> dpm_resume_noirq(PMSG_RESUME) >> >>> xs_resume or xs_supend_cancel >> >>> dpm_resume_end(PMSG_RESUME) >> >>> >> >>> Which seems nicely nested and logical but by only calling dpm_resume_end >> >>> in the non-cancelled case we seem to be unbalancing things. >> >>> >> >>> Do we need some sort of dpm_resume_cancel, or some way of pushing the >> >>> cancelled flag down into the individual xenbus_device.resume handlers? >> >>> >> >>> Should we maybe simply be using a difference PMSG_XXX in the cancelled >> >>> case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? >> >>> >> >>> Looks like to propagate the PMSG_* to the actual device resume functions >> >>> we would need to provide a pm_ops for the struct bus xenbus_frontend >> >>> instead of relying on the legacy handlers. This is probably a >> >>> independently good idea anyway. >> >> >> >> ping? >> >> >> >> Kenji any ideas or patches to address Ian's comments? >> > >> > My colleague made a patch which reflected Ian's comments, so I will ask >> > him to post it. Please wait a little. >> > >> > Thanks, >> > Kenji >> > >> > >> > _______________________________________________ >> > Xen-devel mailing list >> > Xen-devel@lists.xensource.com >> > http://lists.xensource.com/xen-devel > > [-- Attachment #2: fix-checkpoint-linux.patch --] [-- Type: Text/X-Patch, Size: 2004 bytes --] diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 8aa2443..b743e8f 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -181,6 +181,13 @@ static int pm_op(struct device *dev, suspend_report_result(ops->suspend, error); } break; + case PM_EVENT_CANCEL: + if (ops->suspend_cancel) { + error = ops->suspend_cancel(dev); + suspend_report_result(ops->suspend_cancel, error); + break; + } + /* Fall through */ case PM_EVENT_RESUME: if (ops->resume) { error = ops->resume(dev); @@ -291,6 +298,8 @@ static char *pm_verb(int event) switch (event) { case PM_EVENT_SUSPEND: return "suspend"; + case PM_EVENT_CANCEL: + return "cancel"; case PM_EVENT_RESUME: return "resume"; case PM_EVENT_FREEZE: diff --git a/include/linux/pm.h b/include/linux/pm.h index 3b7e04b..aa89839 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -197,6 +197,7 @@ struct dev_pm_ops { int (*prepare)(struct device *dev); void (*complete)(struct device *dev); int (*suspend)(struct device *dev); + int (*suspend_cancel)(struct device *dev); int (*resume)(struct device *dev); int (*freeze)(struct device *dev); int (*thaw)(struct device *dev); @@ -291,6 +292,7 @@ struct dev_pm_ops name = { \ #define PM_EVENT_USER 0x0100 #define PM_EVENT_REMOTE 0x0200 #define PM_EVENT_AUTO 0x0400 +#define PM_EVENT_CANCEL 0x0800 #define PM_EVENT_SLEEP (PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE) #define PM_EVENT_USER_SUSPEND (PM_EVENT_USER | PM_EVENT_SUSPEND) @@ -308,6 +310,7 @@ struct dev_pm_ops name = { \ #define PMSG_THAW ((struct pm_message){ .event = PM_EVENT_THAW, }) #define PMSG_RESTORE ((struct pm_message){ .event = PM_EVENT_RESTORE, }) #define PMSG_RECOVER ((struct pm_message){ .event = PM_EVENT_RECOVER, }) +#define PMSG_CANCEL ((struct pm_message){ .event = PM_EVENT_CANCEL, }) #define PMSG_USER_SUSPEND ((struct pm_message) \ { .event = PM_EVENT_USER_SUSPEND, }) #define PMSG_USER_RESUME ((struct pm_message) \ [-- Attachment #3: fix-checkpoint-xen.patch --] [-- Type: Text/X-Patch, Size: 4182 bytes --] diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 3f71199..22c6288 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -1293,7 +1293,7 @@ static void xennet_disconnect_backend(struct netfront_info *info) info->rx.sring = NULL; } -static int netfront_suspend(struct xenbus_device *dev, pm_message_t state) +static int netfront_suspend(struct xenbus_device *dev) { struct netfront_info *info = dev_get_drvdata(&dev->dev); struct hrtimer *timer = &info->smart_poll.timer; diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c index 0b50906..845afb8 100644 --- a/drivers/xen/manage.c +++ b/drivers/xen/manage.c @@ -148,10 +148,11 @@ out_resume: if (!cancelled) { xen_arch_resume(); xs_resume(); - } else + dpm_resume_end(PMSG_RESUME); + } else { xs_suspend_cancel(); - - dpm_resume_end(PMSG_RESUME); + dpm_resume_end(PMSG_CANCEL); + } /* Make sure timer events get retriggered on all CPUs */ clock_was_set(); diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index 3a83ba2..019337a 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -575,7 +575,7 @@ void xenbus_dev_changed(const char *node, struct xen_bus_type *bus) } EXPORT_SYMBOL_GPL(xenbus_dev_changed); -int xenbus_dev_suspend(struct device *dev, pm_message_t state) +int xenbus_dev_suspend(struct device *dev) { int err = 0; struct xenbus_driver *drv; @@ -587,7 +587,7 @@ int xenbus_dev_suspend(struct device *dev, pm_message_t state) return 0; drv = to_xenbus_driver(dev->driver); if (drv->suspend) - err = drv->suspend(xdev, state); + err = drv->suspend(xdev); if (err) printk(KERN_WARNING "xenbus: suspend %s failed: %i\n", dev_name(dev), err); @@ -595,6 +595,14 @@ int xenbus_dev_suspend(struct device *dev, pm_message_t state) } EXPORT_SYMBOL_GPL(xenbus_dev_suspend); +int xenbus_dev_suspend_cancel(struct device *dev) +{ + /* Do nothing */ + DPRINTK("cancel"); + return 0; +} +EXPORT_SYMBOL_GPL(xenbus_dev_suspend_cancel); + int xenbus_dev_resume(struct device *dev) { int err; diff --git a/drivers/xen/xenbus/xenbus_probe.h b/drivers/xen/xenbus/xenbus_probe.h index 0e5fc4c..bf4a793 100644 --- a/drivers/xen/xenbus/xenbus_probe.h +++ b/drivers/xen/xenbus/xenbus_probe.h @@ -62,7 +62,8 @@ extern void xenbus_dev_changed(const char *node, struct xen_bus_type *bus); extern void xenbus_dev_shutdown(struct device *_dev); -extern int xenbus_dev_suspend(struct device *dev, pm_message_t state); +extern int xenbus_dev_suspend(struct device *dev); +extern int xenbus_dev_suspend_cancel(struct device *dev); extern int xenbus_dev_resume(struct device *dev); extern void xenbus_otherend_changed(struct xenbus_watch *watch, diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c index 5413248..928fc10 100644 --- a/drivers/xen/xenbus/xenbus_probe_frontend.c +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c @@ -82,6 +82,11 @@ static struct device_attribute xenbus_frontend_dev_attrs[] = { __ATTR_NULL }; +static struct dev_pm_ops xenbus_pm_ops = { + .suspend = xenbus_dev_suspend, + .suspend_cancel = xenbus_dev_suspend_cancel, + .resume = xenbus_dev_resume, +}; static struct xen_bus_type xenbus_frontend = { .root = "device", @@ -98,8 +103,7 @@ static struct xen_bus_type xenbus_frontend = { .shutdown = xenbus_dev_shutdown, .dev_attrs= xenbus_frontend_dev_attrs, - .suspend = xenbus_dev_suspend, - .resume = xenbus_dev_resume, + .pm = &xenbus_pm_ops, }, }; diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index 542ca7c..23e7f25 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -91,7 +91,7 @@ struct xenbus_driver { void (*otherend_changed)(struct xenbus_device *dev, enum xenbus_state backend_state); int (*remove)(struct xenbus_device *dev); - int (*suspend)(struct xenbus_device *dev, pm_message_t state); + int (*suspend)(struct xenbus_device *dev); int (*resume)(struct xenbus_device *dev); int (*uevent)(struct xenbus_device *, struct kobj_uevent_env *); struct device_driver driver; [-- Attachment #4: Type: text/plain, Size: 138 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] pvops: fix "xm save -c" issue 2011-01-25 7:23 ` SUZUKI, Kazuhiro @ 2011-01-25 16:32 ` Ian Campbell 0 siblings, 0 replies; 12+ messages in thread From: Ian Campbell @ 2011-01-25 16:32 UTC (permalink / raw) To: SUZUKI, Kazuhiro; +Cc: xen-devel, konrad.wilk On Tue, 2011-01-25 at 07:23 +0000, SUZUKI, Kazuhiro wrote: > Hi Ian, > > I split the patch into pm infrastructure part and xen part. > Actually, I've never post to linux community, so I'm not sure what to > do. Documentation/SubmittingPatches in the Linux source tree has some good guidance. In general you need to post your patches inline (not as attachments which is a difference from the Xen maintainer's policy) with a changelog message, in this case explaining the need for the new event type and explaining the use cases etc, and a signed-off-by. You should send to the subsystem maintainer and the relevant mailing list, the MAINTAINERS file in the Linux source would help figure out who they are but in this case I think it is Rafael J. Wysocki <rjw@sisk.pl> and the linux-pm@lists.linux-foundation.org list. You should cc xen-devel as well. It will be interesting to see what upstream make of the use case. I suspect there might be some common ground with the ability to abort a hibernation attempt on native for example. Ian. > > Thanks, > KAZ > > Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> > Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> > > > From: Ian Campbell <Ian.Campbell@eu.citrix.com> > Subject: Re: [Xen-devel] [PATCH] pvops: fix "xm save -c" issue > Date: Fri, 21 Jan 2011 09:14:30 +0000 > > > On Fri, 2011-01-21 at 06:28 +0000, SUZUKI, Kazuhiro wrote: > >> Hi, > >> > >> The following patch fixes 'xm save -c' issue. > >> We defined 'PMSG_CANCEL' message for suspend cancel situation and > >> suspend_cancel handler in pm_ops struct. > >> If the suspend_cancel is defined, suspend_cancel() is called instead > >> of resume(). > > > > Thanks. I like the general shape of this patch but the core pm > > infrastructure change needs to be split out and sent upstream via the > > power management maintainer. I think this is Rafael J. Wysocki > > <rjw@sisk.pl> and the linux-pm@lists.linux-foundation.org list. > > > > If you also split out the Xen conversion to dev_pm_ops, without the > > suspend_cancel bit, then I think we can take that bit now and then you > > can resend the bit which adds the suspend_cancel bits once the core > > stuff is upstream. > > > > Ian. > > > >> > >> Thanks, > >> KAZ > >> > >> Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com> > >> Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com> > >> > >> > >> From: Kenji Wakamiya <wkenji@jp.fujitsu.com> > >> Subject: Re: [Xen-devel] [PATCH] pvops: fix "xm save -c" issue > >> Date: Fri, 21 Jan 2011 14:35:09 +0900 > >> > >> > Hi Konrand, and sorry for very late response. > >> > > >> > (2011/01/11 2:01), Konrad Rzeszutek Wilk wrote: > >> >>> With this change how is the effect of dpm_suspend_start undone in the > >> >>> suspend cancelled case? > >> >>> > >> >>> Currently we have > >> >>> dpm_suspend_start(PMSG_SUSPEND) > >> >>> xs_suspend > >> >>> dpm_suspend_noirq(PMSG_SUSPEND) > >> >>> SUSPEND > >> >>> dpm_resume_noirq(PMSG_RESUME) > >> >>> xs_resume or xs_supend_cancel > >> >>> dpm_resume_end(PMSG_RESUME) > >> >>> > >> >>> Which seems nicely nested and logical but by only calling dpm_resume_end > >> >>> in the non-cancelled case we seem to be unbalancing things. > >> >>> > >> >>> Do we need some sort of dpm_resume_cancel, or some way of pushing the > >> >>> cancelled flag down into the individual xenbus_device.resume handlers? > >> >>> > >> >>> Should we maybe simply be using a difference PMSG_XXX in the cancelled > >> >>> case? Is this what one of PMSG_RESTORE or PMSG_RECOVER means? > >> >>> > >> >>> Looks like to propagate the PMSG_* to the actual device resume functions > >> >>> we would need to provide a pm_ops for the struct bus xenbus_frontend > >> >>> instead of relying on the legacy handlers. This is probably a > >> >>> independently good idea anyway. > >> >> > >> >> ping? > >> >> > >> >> Kenji any ideas or patches to address Ian's comments? > >> > > >> > My colleague made a patch which reflected Ian's comments, so I will ask > >> > him to post it. Please wait a little. > >> > > >> > Thanks, > >> > Kenji > >> > > >> > > >> > _______________________________________________ > >> > Xen-devel mailing list > >> > Xen-devel@lists.xensource.com > >> > http://lists.xensource.com/xen-devel > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-01-25 16:32 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-12-14 5:31 [PATCH] pvops: fix "xm save -c" issue Kenji Wakamiya 2010-12-14 14:59 ` Konrad Rzeszutek Wilk 2010-12-15 4:18 ` Kenji Wakamiya 2010-12-15 15:40 ` Konrad Rzeszutek Wilk 2010-12-16 5:49 ` Kenji Wakamiya 2010-12-17 9:43 ` Ian Campbell 2011-01-10 17:01 ` Konrad Rzeszutek Wilk 2011-01-21 5:35 ` Kenji Wakamiya 2011-01-21 6:28 ` SUZUKI, Kazuhiro 2011-01-21 9:14 ` Ian Campbell 2011-01-25 7:23 ` SUZUKI, Kazuhiro 2011-01-25 16:32 ` Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.