| Message ID | 2d3a7ed32bf38e13e0141a631a453b6e4c7ba5dc.1598042152.git.anchalag@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix PM hibernation in Xen guests | expand |
On 8/21/20 6:26 PM, Anchal Agarwal wrote: > From: Munehisa Kamata <kamatam@amazon.com> > > Since commit b3e96c0c7562 ("xen: use freeze/restore/thaw PM events for > suspend/resume/chkpt"), xenbus uses PMSG_FREEZE, PMSG_THAW and > PMSG_RESTORE events for Xen suspend. However, they're actually assigned > to xenbus_dev_suspend(), xenbus_dev_cancel() and xenbus_dev_resume() > respectively, and only suspend and resume callbacks are supported at > driver level. To support PM suspend and PM hibernation, modify the bus > level PM callbacks to invoke not only device driver's suspend/resume but > also freeze/thaw/restore. > > Note that we'll use freeze/restore callbacks even for PM suspend whereas > suspend/resume callbacks are normally used in the case, becausae the > existing xenbus device drivers already have suspend/resume callbacks > specifically designed for Xen suspend. Something is wrong with this sentence. Or with my brain --- I can't quite parse this. And please be consistent with "PM suspend" vs. "PM hibernation". > So we can allow the device > drivers to keep the existing callbacks wihtout modification. > > @@ -599,16 +600,33 @@ int xenbus_dev_suspend(struct device *dev) > struct xenbus_driver *drv; > struct xenbus_device *xdev > = container_of(dev, struct xenbus_device, dev); > + bool xen_suspend = is_xen_suspend(); > > DPRINTK("%s", xdev->nodename); > > if (dev->driver == NULL) > return 0; > drv = to_xenbus_driver(dev->driver); > - if (drv->suspend) > - err = drv->suspend(xdev); > - if (err) > - dev_warn(dev, "suspend failed: %i\n", err); > + if (xen_suspend) { > + if (drv->suspend) > + err = drv->suspend(xdev); > + } else { > + if (drv->freeze) { 'else if' (to avoid extra indent level). In xenbus_dev_resume() too. > + err = drv->freeze(xdev); > + if (!err) { > + free_otherend_watch(xdev); > + free_otherend_details(xdev); > + return 0; > + } > + } > + } > + > + if (err) { > + dev_warn(&xdev->dev, Is there a reason why you replaced dev with xdev->dev (here and elsewhere)? > "%s %s failed: %d\n", xen_suspend ? > + "suspend" : "freeze", xdev->nodename, err); > + return err; > + } > + > > @@ -653,8 +683,44 @@ EXPORT_SYMBOL_GPL(xenbus_dev_resume); > > int xenbus_dev_cancel(struct device *dev) > { > - /* Do nothing */ > - DPRINTK("cancel"); > + int err; > + struct xenbus_driver *drv; > + struct xenbus_device *xendev = to_xenbus_device(dev); xdev for consistency please. > + bool xen_suspend = is_xen_suspend(); No need for this, you use it only once anyway. -boris
On Sun, Sep 13, 2020 at 12:11:47PM -0400, boris.ostrovsky@oracle.com wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > On 8/21/20 6:26 PM, Anchal Agarwal wrote: > > From: Munehisa Kamata <kamatam@amazon.com> > > > > Since commit b3e96c0c7562 ("xen: use freeze/restore/thaw PM events for > > suspend/resume/chkpt"), xenbus uses PMSG_FREEZE, PMSG_THAW and > > PMSG_RESTORE events for Xen suspend. However, they're actually assigned > > to xenbus_dev_suspend(), xenbus_dev_cancel() and xenbus_dev_resume() > > respectively, and only suspend and resume callbacks are supported at > > driver level. To support PM suspend and PM hibernation, modify the bus > > level PM callbacks to invoke not only device driver's suspend/resume but > > also freeze/thaw/restore. > > > > Note that we'll use freeze/restore callbacks even for PM suspend whereas > > suspend/resume callbacks are normally used in the case, becausae the > > existing xenbus device drivers already have suspend/resume callbacks > > specifically designed for Xen suspend. > > > Something is wrong with this sentence. Or with my brain --- I can't > quite parse this. > The message is trying to say that that freeze/thaw/restore callbacks will be used for both PM SUSPEND and PM HIBERNATION. Since, we are only focussing on PM hibernation, I will remove all wordings of PM suspend from this message to avoid confusion. I left it there in case someone wants to pick it up in future knowing framework is already present. > > And please be consistent with "PM suspend" vs. "PM hibernation". > I should remove PM suspend from everywhere since the mode is not tested for. > > > So we can allow the device > > drivers to keep the existing callbacks wihtout modification. > > > > > > @@ -599,16 +600,33 @@ int xenbus_dev_suspend(struct device *dev) > > struct xenbus_driver *drv; > > struct xenbus_device *xdev > > = container_of(dev, struct xenbus_device, dev); > > + bool xen_suspend = is_xen_suspend(); > > > > DPRINTK("%s", xdev->nodename); > > > > if (dev->driver == NULL) > > return 0; > > drv = to_xenbus_driver(dev->driver); > > - if (drv->suspend) > > - err = drv->suspend(xdev); > > - if (err) > > - dev_warn(dev, "suspend failed: %i\n", err); > > + if (xen_suspend) { > > + if (drv->suspend) > > + err = drv->suspend(xdev); > > + } else { > > + if (drv->freeze) { > > > 'else if' (to avoid extra indent level). In xenbus_dev_resume() too. > > > > + err = drv->freeze(xdev); > > + if (!err) { > > + free_otherend_watch(xdev); > > + free_otherend_details(xdev); > > + return 0; > > + } > > + } > > + } > > + > > + if (err) { > > + dev_warn(&xdev->dev, > > > Is there a reason why you replaced dev with xdev->dev (here and elsewhere)? > > Nope, they should be same. We can use dev here too. I should probably just use dev. > > "%s %s failed: %d\n", xen_suspend ? > > + "suspend" : "freeze", xdev->nodename, err); > > + return err; > > + } > > + > > > > > @@ -653,8 +683,44 @@ EXPORT_SYMBOL_GPL(xenbus_dev_resume); > > > > int xenbus_dev_cancel(struct device *dev) > > { > > - /* Do nothing */ > > - DPRINTK("cancel"); > > + int err; > > + struct xenbus_driver *drv; > > + struct xenbus_device *xendev = to_xenbus_device(dev); > > > xdev for consistency please. > Yes this I left unchanged, it should be consistent with xdev. > > > + bool xen_suspend = is_xen_suspend(); > > > No need for this, you use it only once anyway. > > > -boris > Thanks, Anchal >
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index 38725d97d909..e8ad7879bcd3 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -50,6 +50,7 @@ #include <linux/io.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/suspend.h> #include <asm/page.h> #include <asm/xen/hypervisor.h> @@ -599,16 +600,33 @@ int xenbus_dev_suspend(struct device *dev) struct xenbus_driver *drv; struct xenbus_device *xdev = container_of(dev, struct xenbus_device, dev); + bool xen_suspend = is_xen_suspend(); DPRINTK("%s", xdev->nodename); if (dev->driver == NULL) return 0; drv = to_xenbus_driver(dev->driver); - if (drv->suspend) - err = drv->suspend(xdev); - if (err) - dev_warn(dev, "suspend failed: %i\n", err); + if (xen_suspend) { + if (drv->suspend) + err = drv->suspend(xdev); + } else { + if (drv->freeze) { + err = drv->freeze(xdev); + if (!err) { + free_otherend_watch(xdev); + free_otherend_details(xdev); + return 0; + } + } + } + + if (err) { + dev_warn(&xdev->dev, "%s %s failed: %d\n", xen_suspend ? + "suspend" : "freeze", xdev->nodename, err); + return err; + } + return 0; } EXPORT_SYMBOL_GPL(xenbus_dev_suspend); @@ -619,6 +637,7 @@ int xenbus_dev_resume(struct device *dev) struct xenbus_driver *drv; struct xenbus_device *xdev = container_of(dev, struct xenbus_device, dev); + bool xen_suspend = is_xen_suspend(); DPRINTK("%s", xdev->nodename); @@ -627,23 +646,34 @@ int xenbus_dev_resume(struct device *dev) drv = to_xenbus_driver(dev->driver); err = talk_to_otherend(xdev); if (err) { - dev_warn(dev, "resume (talk_to_otherend) failed: %i\n", err); + dev_warn(&xdev->dev, "%s (talk_to_otherend) %s failed: %d\n", + xen_suspend ? "resume" : "restore", + xdev->nodename, err); return err; } - xdev->state = XenbusStateInitialising; + if (xen_suspend) { + xdev->state = XenbusStateInitialising; + if (drv->resume) + err = drv->resume(xdev); + } else { + if (drv->restore) + err = drv->restore(xdev); + } - if (drv->resume) { - err = drv->resume(xdev); - if (err) { - dev_warn(dev, "resume failed: %i\n", err); - return err; - } + if (err) { + dev_warn(&xdev->dev, "%s %s failed: %d\n", + xen_suspend ? "resume" : "restore", + xdev->nodename, err); + return err; } err = watch_otherend(xdev); if (err) { - dev_warn(dev, "resume (watch_otherend) failed: %d\n", err); + dev_warn(&xdev->dev, "%s (watch_otherend) %s failed: %d.\n", + xen_suspend ? "resume" : "restore", + xdev->nodename, err); + return err; } @@ -653,8 +683,44 @@ EXPORT_SYMBOL_GPL(xenbus_dev_resume); int xenbus_dev_cancel(struct device *dev) { - /* Do nothing */ - DPRINTK("cancel"); + int err; + struct xenbus_driver *drv; + struct xenbus_device *xendev = to_xenbus_device(dev); + bool xen_suspend = is_xen_suspend(); + + if (xen_suspend) { + /* Do nothing */ + DPRINTK("cancel"); + return 0; + } + + DPRINTK("%s", xendev->nodename); + + if (dev->driver == NULL) + return 0; + drv = to_xenbus_driver(dev->driver); + err = talk_to_otherend(xendev); + if (err) { + dev_warn(&xendev->dev, "thaw (talk_to_otherend) %s failed: %d.\n", + xendev->nodename, err); + return err; + } + + if (drv->thaw) { + err = drv->thaw(xendev); + if (err) { + dev_warn(&xendev->dev, "thaw %s failed: %d\n", xendev->nodename, err); + return err; + } + } + + err = watch_otherend(xendev); + if (err) { + dev_warn(&xendev->dev, "thaw (watch_otherend) %s failed: %d.\n", + xendev->nodename, err); + return err; + } + return 0; } EXPORT_SYMBOL_GPL(xenbus_dev_cancel); diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index 5a8315e6d8a6..8da964763255 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -104,6 +104,9 @@ struct xenbus_driver { int (*remove)(struct xenbus_device *dev); int (*suspend)(struct xenbus_device *dev); int (*resume)(struct xenbus_device *dev); + int (*freeze)(struct xenbus_device *dev); + int (*thaw)(struct xenbus_device *dev); + int (*restore)(struct xenbus_device *dev); int (*uevent)(struct xenbus_device *, struct kobj_uevent_env *); struct device_driver driver; int (*read_otherend_details)(struct xenbus_device *dev);