linux-kernel - Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

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: <1321347089.26084.2.camel@minggr>
Date:	Tue, 15 Nov 2011 16:51:29 +0800
From:	Lin Ming <ming.m.lin@...el.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	"linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Jeff Garzik <jgarzik@...ox.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	James Bottomley <JBottomley@...allels.com>,
	Tejun Heo <tj@...nel.org>,
	"Huang, Ying" <ying.huang@...el.com>,
	"Zhang, Rui" <rui.zhang@...el.com>
Subject: Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

On Mon, 2011-11-14 at 23:31 +0800, Alan Stern wrote:
> On Mon, 14 Nov 2011, Lin Ming wrote:
> 
> > Current patches has a bug that system suspend fails if ata port was
> > runtime suspended.
> > 
> > disk suspend issues sync cache and stop device commands that obviously
> > need ata port to be active. So we need to runtime resume ata port first.
> 
> This is wrong.  If the port is already suspended then so are all the 
> drives below the port.  Hence there is no need to sync the cache or 
> stop the device.

Ah, got it now!

> 
> > Alan, Tejun
> > 
> > How about below fix?
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index fa3a591..ebb87fbf 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -50,6 +50,7 @@
> >  #include <linux/string_helpers.h>
> >  #include <linux/async.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/unaligned.h>
> >  
> > @@ -2762,6 +2763,14 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
> >  	if (!sdkp)
> >  		return 0;	/* this can happen */
> >  
> > +	/*
> > +	 * Resume parent device to handle sync cache and
> > +	 * stop device commands
> > +	 */
> > +	ret = pm_runtime_get_sync(dev->parent);
> > +	if (ret)
> > +		goto exit;
> > +
> >  	if (sdkp->WCE) {
> >  		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> >  		ret = sd_sync_cache(sdkp);
> 
> This is not the right approach.  You should look instead at 
> scsi_dev_type_suspend() in scsi_pm.c.  If the device is already runtime 
> suspended then the routine should return immediately.

How about below?

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d329f8b..94b60bd 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -20,6 +20,9 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
 	struct device_driver *drv;
 	int err;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	err = scsi_device_quiesce(to_scsi_device(dev));
 	if (err == 0) {
 		drv = dev->driver;


> 
> Alan Stern
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/