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:

quoted

From: Munehisa Kamata <redacted>

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.

quoted

 So we can allow the device
drivers to keep the existing callbacks wihtout modification.

quoted

@@ -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.

quoted

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

quoted

 "%s %s failed: %d\n", xen_suspend ?
+                             "suspend" : "freeze", xdev->nodename, err);
+             return err;
+     }
+

quoted

@@ -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.

quoted

+     bool xen_suspend = is_xen_suspend();

No need for this, you use it only once anyway.


-boris

Thanks,
Anchal