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: <eebc26b8-f1b1-3bea-5366-dd77f063237e@oracle.com>
Date:   Sun, 13 Sep 2020 12:11:47 -0400
From:   boris.ostrovsky@...cle.com
To:     Anchal Agarwal <anchalag@...zon.com>, 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 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.


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