RE: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

RE: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[James.Smart]

In other times when I implemented this....

You need to be careful on the power-up. Many JBODs share a single
"enclosure" and that enclosure has a limited power supply. If all
drives were spun up in parallel (and a drive may take 10-15seconds
to spin up), then they can overload the enclosure's power limit.
This issue, which normally occurs when an enclosure is first powered
on, was solved by injecting sequence delays based on either jumpers
or delays based on address/slot. But this won't help the software
suspend/resume.

There were not a lot of great answers on how to solve this as it usually
required knowledge of how the hardware was packaged. What we defaulted
to was limiting spin up to never concurrently start more than N drives
on a scsi bus. N defaulted to 1, but allowed the admin to tune it.

-- james s

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org]On Behalf Of 
> Christoph Hellwig
> Sent: Thursday, September 29, 2005 4:12 AM
> To: Christoph Hellwig; Jeff Garzik; Joshua Kwan;
> linux-kernel@vger.kernel.org; linux-ide@vger.kernel.org;
> linux-scsi@vger.kernel.org; axboe@suse.de; torvalds@osdl.org;
> randy_dunlap
> Subject: Re: SATA suspend/resume (was Re: [PATCH] updated version of
> Jens' SATA suspend-to-ram patch)
> 
> 
> On Thu, Sep 29, 2005 at 08:34:37AM +0100, Christoph Hellwig wrote:
> > is an ULDD operation, not an LLDD one, and this fits the 
> layering model
> > much better.  The only complaints here are cosmetics:
> > 
> >  - generic_scsi_suspend/generic_scsi_resume are misnamed, 
> they should
> >    probably be scsi_device_suspend/resume.
> >  - while we're at it they could probably move to 
> scsi_sysfs.c to keep
> >    them static in one file - they're just a tiny bit of glue anyway.
> >  - get rid of all the CONFIG_PM ifdefs - it just clutters 
> thing up far
> >    too much.
> 
> Actually one important thing is missing, that is a way to 
> avoid spinning
> down external disks.  As a start a sysfs-controlable flag 
> should do it,
> later we can add transport-specific ways to find out whether a device
> is external.
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Stefan Richter]

I admit I haven't studied the patches. Anyway, here is what I have to 
say about what I (mis?)understood from your posts:

James Smart wrote:
> You need to be careful on the power-up. Many JBODs share a single
> "enclosure" and that enclosure has a limited power supply. If all
> drives were spun up in parallel (and a drive may take 10-15seconds
> to spin up), then they can overload the enclosure's power limit.
[...]
> There were not a lot of great answers on how to solve this as it usually
> required knowledge of how the hardware was packaged.
[...]

>>On Thu, Sep 29, 2005 at 08:34:37AM +0100, Christoph Hellwig wrote:
>>>is an ULDD operation, not an LLDD one, and this fits the layering model
>>>much better.

The operation _involves_ the high level, yes. But whether it may be/ 
must not be performed has to be controlled by the transport layer. Take 
FireWire as an example: IEEE 1394(a,b) has power management 
specifications. (NB: Its indeed in IEEE 1394, not in the SBP-2 spec.) 
One rule is that only one node on a FireWire bus may perform power 
management; the node which is allowed to do this is determined by a 
special protocol.

>>Actually one important thing is missing, that is a way to avoid spinning
>>down external disks.  As a start a sysfs-controlable flag should do it,
>>later we can add transport-specific ways to find out whether a device
>>is external.

It is not a question of external vs. internal, at least not if you 
consider more than SATA. Power management is the genuine task of 
transport layers (specifically, of transport management layers). These 
layers might need assistence from SCSI high-level protocol layer though.

IOW it's certainly correct to provide suspend/resume helpers in SCSI 
high level (probably abstracted through SCSI core), but whether these 
helpers are called or not has to be decided down in the SCSI low level, 
or even further beneath that level.
-- 
Stefan Richter
-=====-=-=-= =--= ===-=
http://arcgraph.de/sr/

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Randy.Dunlap]

On Sat, 01 Oct 2005 14:24:12 -0400 Mark Lord wrote:

> Jeff Garzik wrote:
> ..
> > Ah hah!  I found the other SCSI suspend patch:
> >     http://lwn.net/Articles/97453/
> > Anybody (Joshua?) up for reconciling and testing the two?
> 
> I just now tried out *only* "the other SCSI suspend patch",
> and by itself it hangs on resume.  Laptop computer, blank screen,
> no serial ports, no printk()s visible.
> 
> And there's one minor bug in that patch:  it uses GFP_KERNEL to
> alloc a buffer, but on resume it really should use GFP_ATOMIC instead,
> since the swap device is the same drive we're trying to resume..
> 
> > 2) sd should call START STOP UNIT on resume
> 
> That's probably why it hangs when used as-is by itself.
> I may do some further testing.
> 
> Anyone else out there playing with this yet?

Not playing with it yet, just making some changes as suggested
by Jeff and Christoph.  Patches are in
  http://www.xenotime.net/linux/scsi/

1.  http://www.xenotime.net/linux/scsi/scsi-suspend-resume.patch
2.  http://www.xenotime.net/linux/scsi/scsi-susres-startstop2.patch

and work-in-progress:  adding <spindown> ok/allowed to scsi
targets:  http://www.xenotime.net/linux/scsi/scsi-susres-stst-spin.patch
(this patch file includes 1. and 2. above)

---
~Randy

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Mark Lord]

Jeff Garzik wrote:
..
> Ah hah!  I found the other SCSI suspend patch:
>     http://lwn.net/Articles/97453/
> Anybody (Joshua?) up for reconciling and testing the two?

I just now tried out *only* "the other SCSI suspend patch",
and by itself it hangs on resume.  Laptop computer, blank screen,
no serial ports, no printk()s visible.

And there's one minor bug in that patch:  it uses GFP_KERNEL to
alloc a buffer, but on resume it really should use GFP_ATOMIC instead,
since the swap device is the same drive we're trying to resume..

> 2) sd should call START STOP UNIT on resume

That's probably why it hangs when used as-is by itself.
I may do some further testing.

Anyone else out there playing with this yet?

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Randy.Dunlap]

On Thu, 29 Sep 2005, Randy.Dunlap wrote:

> Here's Nathan Bryant's patch (from the lwn.ne article) updated
> to 2.6.14-rc2-git7 + changes that Christoph suggested (except
> that 'scsi_device_resume' name was already used, so I changed it
> to 'scsi_device_wakeup' instead).
>
> I'll get back to Jeff's suggestion(s) and the sysfs flag next,
> but others can use this as a basis if wanted.
>
> (also available from
> http://www.xenotime.net/linux/scsi/scsi-suspend-resume.patch
> )

The inline version of this patch is whitespace-damaged  :(
(thanx to pine).

It's ok for review, but please download the one above
for applying or patching.

-- 
~Randy

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Randy.Dunlap]

On Thu, 29 Sep 2005, Jeff Garzik wrote:

> Joshua Kwan wrote:
> > Hello,
> >
> > I had some time yesterday and decided to help Jens out by rediffing the
> > now-infamous SATA suspend-to-ram patch [1] against current git and
> > test-building it.
> >
> > For posterity,
> >
> > This patch adds the ata_scsi_device_resume and ata_scsi_device_suspend
> > functions (along with helpers) to put to sleep and wake up Serial ATA
> > controllers when entering sleep states, and hooks the functions into
> > each SATA controller driver so that suspend-to-RAM is possible.
> >
> > Note that this patch is a holdover patch until it is possible to
> > generalize this concept for all SCSI devices, which requires more data
> > on which devices need to be put to sleep and which don't.
> >
> > Signed-off-by: Joshua Kwan <joshk@triplehelix.org>
>
> Ah hah!  I found the other SCSI suspend patch:
>
> 	http://lwn.net/Articles/97453/
>
> Anybody (Joshua?) up for reconciling and testing the two?
>
> The main change from Jens/Joshua's patch is that we use SCSI's
> sd_shutdown() to call sync cache, eliminating the need for
> ata_flush_cache(), since the SCSI layer would now perform that.
>
> For bonus points,
>
> 1) sd should call START STOP UNIT on suspend, which eliminates the need
> for ata_standby_drive(), and completely encompasses the suspend process
> in the SCSI layer.
>
> 2) sd should call START STOP UNIT on resume -- and as a SUPER BONUS, the
> combination of these two changes ensures that there are no queue
> synchronization issues, the likes of which would require hacks like
> Jens' while-loop patch.
>
> None of these are huge changes requiring a lot of thinking/planning...
>
> Finally, ideally, we should be issuing a hardware or software reset on
> suspend.

Here's Nathan Bryant's patch (from the lwn.ne article) updated
to 2.6.14-rc2-git7 + changes that Christoph suggested (except
that 'scsi_device_resume' name was already used, so I changed it
to 'scsi_device_wakeup' instead).

I'll get back to Jeff's suggestion(s) and the sysfs flag next,
but others can use this as a basis if wanted.

(also available from
http://www.xenotime.net/linux/scsi/scsi-suspend-resume.patch
)
-- 
~Randy





 drivers/scsi/scsi_priv.h   |    2 +
 drivers/scsi/scsi_sysfs.c  |   39 +++++++++++++++++++++++++++++++++--
 drivers/scsi/sd.c          |   18 +++++++++++++++-
 include/scsi/scsi_driver.h |    2 +
 4 files changed, 58 insertions(+), 3 deletions(-)

diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/drivers/scsi/scsi_priv.h linux-2614-rc2-git7-scsi/drivers/scsi/scsi_priv.h
--- linux-2614-rc2-git7-clean/drivers/scsi/scsi_priv.h	2005-09-29 12:58:21.000000000 -0700
+++ linux-2614-rc2-git7-scsi/drivers/scsi/scsi_priv.h	2005-09-29 12:48:00.000000000 -0700
@@ -55,6 +55,8 @@ static inline void scsi_log_send(struct
 static inline void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
 	{ };
 #endif
+extern int scsi_device_suspend(struct device *dev, pm_message_t state);
+extern int scsi_device_wakeup(struct device *dev);

 /* scsi_devinfo.c */
 extern int scsi_get_device_flags(struct scsi_device *sdev,
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/drivers/scsi/scsi_sysfs.c linux-2614-rc2-git7-scsi/drivers/scsi/scsi_sysfs.c
--- linux-2614-rc2-git7-clean/drivers/scsi/scsi_sysfs.c	2005-09-29 12:58:25.000000000 -0700
+++ linux-2614-rc2-git7-scsi/drivers/scsi/scsi_sysfs.c	2005-09-29 12:56:16.000000000 -0700
@@ -14,6 +14,7 @@

 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
+#include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsi_transport.h>
@@ -264,8 +265,10 @@ static int scsi_bus_match(struct device
 }

 struct bus_type scsi_bus_type = {
-        .name		= "scsi",
-        .match		= scsi_bus_match,
+	.name		= "scsi",
+	.match		= scsi_bus_match,
+	.suspend	= scsi_device_suspend,
+	.resume		= scsi_device_wakeup,
 };

 int scsi_sysfs_register(void)
@@ -770,6 +773,38 @@ void scsi_remove_target(struct device *d
 }
 EXPORT_SYMBOL(scsi_remove_target);

+int scsi_device_suspend(struct device *dev, pm_message_t state)
+{
+	int err;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct scsi_driver *drv = to_scsi_driver(dev->driver);
+
+	err = scsi_device_quiesce(sdev);
+	if (err)
+		return err;
+
+	if (drv->suspend)
+		return drv->suspend(dev, state);
+
+	return 0;
+}
+
+int scsi_device_wakeup(struct device *dev)
+{
+	int err;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct scsi_driver *drv = to_scsi_driver(dev->driver);
+
+	if (drv->resume) {
+		err = drv->resume(dev);
+		if (err)
+			return err;
+	}
+
+	scsi_device_resume(sdev);
+	return 0;
+}
+
 int scsi_register_driver(struct device_driver *drv)
 {
 	drv->bus = &scsi_bus_type;
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/drivers/scsi/sd.c linux-2614-rc2-git7-scsi/drivers/scsi/sd.c
--- linux-2614-rc2-git7-clean/drivers/scsi/sd.c	2005-09-29 12:58:25.000000000 -0700
+++ linux-2614-rc2-git7-scsi/drivers/scsi/sd.c	2005-09-29 13:02:10.000000000 -0700
@@ -117,6 +117,8 @@ static void sd_rw_intr(struct scsi_cmnd

 static int sd_probe(struct device *);
 static int sd_remove(struct device *);
+static int sd_suspend(struct device *, pm_message_t);
+static int sd_resume(struct device *);
 static void sd_shutdown(struct device *dev);
 static void sd_rescan(struct device *);
 static int sd_init_command(struct scsi_cmnd *);
@@ -136,6 +138,8 @@ static struct scsi_driver sd_template =
 	},
 	.rescan			= sd_rescan,
 	.init_command		= sd_init_command,
+	.suspend		= sd_suspend,
+	.resume			= sd_resume,
 	.issue_flush		= sd_issue_flush,
 	.prepare_flush		= sd_prepare_flush,
 	.end_flush		= sd_end_flush,
@@ -1691,7 +1695,19 @@ static void sd_shutdown(struct device *d
 	printk(KERN_NOTICE "Synchronizing SCSI cache for disk %s: \n",
 			sdkp->disk->disk_name);
 	sd_sync_cache(sdp);
-}
+}
+
+static int sd_suspend(struct device *dev, pm_message_t state)
+{
+	sd_shutdown(dev);
+	return 0;
+}
+
+static int sd_resume(struct device *dev)
+{
+	sd_rescan(dev);
+	return 0;
+}

 /**
  *	init_sd - entry point for this driver (both when built in or when
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/include/scsi/scsi_driver.h linux-2614-rc2-git7-scsi/include/scsi/scsi_driver.h
--- linux-2614-rc2-git7-clean/include/scsi/scsi_driver.h	2005-08-28 16:41:01.000000000 -0700
+++ linux-2614-rc2-git7-scsi/include/scsi/scsi_driver.h	2005-09-29 12:47:43.000000000 -0700
@@ -13,6 +13,8 @@ struct scsi_driver {

 	int (*init_command)(struct scsi_cmnd *);
 	void (*rescan)(struct device *);
+	int (*suspend)(struct device *dev, pm_message_t state);
+	int (*resume)(struct device *dev);
 	int (*issue_flush)(struct device *, sector_t *);
 	int (*prepare_flush)(struct request_queue *, struct request *);
 	void (*end_flush)(struct request_queue *, struct request *);

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Christoph Hellwig]

On Thu, Sep 29, 2005 at 08:34:37AM +0100, Christoph Hellwig wrote:
> is an ULDD operation, not an LLDD one, and this fits the layering model
> much better.  The only complaints here are cosmetics:
> 
>  - generic_scsi_suspend/generic_scsi_resume are misnamed, they should
>    probably be scsi_device_suspend/resume.
>  - while we're at it they could probably move to scsi_sysfs.c to keep
>    them static in one file - they're just a tiny bit of glue anyway.
>  - get rid of all the CONFIG_PM ifdefs - it just clutters thing up far
>    too much.

Actually one important thing is missing, that is a way to avoid spinning
down external disks.  As a start a sysfs-controlable flag should do it,
later we can add transport-specific ways to find out whether a device
is external.

Re: SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Christoph Hellwig]

On Thu, Sep 29, 2005 at 01:21:28AM -0400, Jeff Garzik wrote:
> Ah hah!  I found the other SCSI suspend patch:
> 
> 	http://lwn.net/Articles/97453/
> 
> Anybody (Joshua?) up for reconciling and testing the two?
> 
> The main change from Jens/Joshua's patch is that we use SCSI's 
> sd_shutdown() to call sync cache, eliminating the need for 
> ata_flush_cache(), since the SCSI layer would now perform that.
> 
> For bonus points,
> 
> 1) sd should call START STOP UNIT on suspend, which eliminates the need 
> for ata_standby_drive(), and completely encompasses the suspend process 
> in the SCSI layer.
> 
> 2) sd should call START STOP UNIT on resume -- and as a SUPER BONUS, the 
> combination of these two changes ensures that there are no queue 
> synchronization issues, the likes of which would require hacks like 
> Jens' while-loop patch.
> 
> None of these are huge changes requiring a lot of thinking/planning...
> 
> Finally, ideally, we should be issuing a hardware or software reset on 
> suspend.

I like this one much more than the other patch aswell, because suspsending
is an ULDD operation, not an LLDD one, and this fits the layering model
much better.  The only complaints here are cosmetics:

 - generic_scsi_suspend/generic_scsi_resume are misnamed, they should
   probably be scsi_device_suspend/resume.
 - while we're at it they could probably move to scsi_sysfs.c to keep
   them static in one file - they're just a tiny bit of glue anyway.
 - get rid of all the CONFIG_PM ifdefs - it just clutters thing up far
   too much.

SATA suspend/resume (was Re: [PATCH] updated version of Jens' SATA suspend-to-ram patch)

[Jeff Garzik]

Joshua Kwan wrote:
> Hello,
> 
> I had some time yesterday and decided to help Jens out by rediffing the
> now-infamous SATA suspend-to-ram patch [1] against current git and
> test-building it.
> 
> For posterity,
> 
> This patch adds the ata_scsi_device_resume and ata_scsi_device_suspend
> functions (along with helpers) to put to sleep and wake up Serial ATA
> controllers when entering sleep states, and hooks the functions into
> each SATA controller driver so that suspend-to-RAM is possible.
> 
> Note that this patch is a holdover patch until it is possible to
> generalize this concept for all SCSI devices, which requires more data
> on which devices need to be put to sleep and which don't.
> 
> Signed-off-by: Joshua Kwan <joshk@triplehelix.org>

Ah hah!  I found the other SCSI suspend patch:

	http://lwn.net/Articles/97453/

Anybody (Joshua?) up for reconciling and testing the two?

The main change from Jens/Joshua's patch is that we use SCSI's 
sd_shutdown() to call sync cache, eliminating the need for 
ata_flush_cache(), since the SCSI layer would now perform that.

For bonus points,

1) sd should call START STOP UNIT on suspend, which eliminates the need 
for ata_standby_drive(), and completely encompasses the suspend process 
in the SCSI layer.

2) sd should call START STOP UNIT on resume -- and as a SUPER BONUS, the 
combination of these two changes ensures that there are no queue 
synchronization issues, the likes of which would require hacks like 
Jens' while-loop patch.

None of these are huge changes requiring a lot of thinking/planning...

Finally, ideally, we should be issuing a hardware or software reset on 
suspend.

	Jeff