linux-kernel - Re: [PATCH v3 02/11] xenbus: add freeze/thaw/restore callbacks support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200915195643.GA28542@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>
Date:   Tue, 15 Sep 2020 19:56:43 +0000
From:   Anchal Agarwal <anchalag@...zon.com>
To:     <boris.ostrovsky@...cle.com>
CC:     <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <hpa@...or.com>, <x86@...nel.org>, <jgross@...e.com>,
        <linux-pm@...r.kernel.org>, <linux-mm@...ck.org>,
        <kamatam@...zon.com>, <sstabellini@...nel.org>,
        <konrad.wilk@...cle.com>, <roger.pau@...rix.com>,
        <axboe@...nel.dk>, <davem@...emloft.net>, <rjw@...ysocki.net>,
        <len.brown@...el.com>, <pavel@....cz>, <peterz@...radead.org>,
        <eduval@...zon.com>, <sblbir@...zon.com>,
        <xen-devel@...ts.xenproject.org>, <vkuznets@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <dwmw@...zon.co.uk>, <benh@...nel.crashing.org>
Subject: Re: [PATCH v3 02/11] xenbus: add freeze/thaw/restore callbacks support

On Sun, Sep 13, 2020 at 12:11:47PM -0400, boris.ostrovsky@...cle.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@...zon.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
>