Suspend support for IDE

Suspend support for IDE

[Pavel Machek]

Hi!

This adds driver support to ide-disk.c. Does it look good to you? [If
so, applying it will not hurt, altrough it might change in near
future.]

									Pavel

--- clean.pre/drivers/ide/ide-disk.c	Fri Mar  8 18:40:34 2002
+++ linux-dm.pre/drivers/ide/ide-disk.c	Fri Mar  8 18:40:07 2002
@@ -123,6 +123,8 @@
  */
 static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
 {
+	if (drive->blocked)
+		panic("ide: Request while drive blocked? You don't like your data intact?");
 	if (!(rq->flags & REQ_CMD)) {
 		blk_dump_rq_flags(rq, "do_rw_disk, bad command");
 		ide_end_request(drive, 0);
@@ -910,13 +912,36 @@
 	ide_add_setting(drive,	"max_failures",		SETTING_RW,					-1,			-1,			TYPE_INT,	0,	65535,				1,	1,	&drive->max_failures,		NULL);
 }
 
+static int idedisk_suspend(struct device *dev, u32 state, u32 level)
+{
+	int i;
+	ide_drive_t *drive = dev->driver_data;
+
+	printk("ide_disk_suspend()\n");
+	while (HWGROUP(drive)->handler)
+		schedule();
+	drive->blocked = 1;
+}
+
+static int idedisk_resume(struct device *dev, u32 level)
+{
+	ide_drive_t *drive = dev->driver_data;
+	if (!drive->blocked)
+		panic("ide: Resume but not suspended?\n");
+	drive->blocked = 0;
+}
+
+
 /* This is just a hook for the overall driver tree.
  *
  * FIXME: This is soon goig to replace the custom linked list games played up
  * to great extend between the different components of the IDE drivers.
  */
 
-static struct device_driver idedisk_devdrv = {};
+static struct device_driver idedisk_devdrv = {
+	suspend: idedisk_suspend,
+	resume: idedisk_resume,
+};
 
 static void idedisk_setup(ide_drive_t *drive)
 {
@@ -963,6 +988,7 @@
 	    sprintf(drive->device.name, "ide-disk");
 	    drive->device.driver = &idedisk_devdrv;
 	    drive->device.parent = &HWIF(drive)->device;
+	    drive->device.driver_data = drive;
 	    device_register(&drive->device);
 	}
 
--- clean.pre/include/linux/ide.h	Fri Mar  8 18:40:38 2002
+++ linux-dm.pre/include/linux/ide.h	Fri Mar  8 18:37:44 2002
@@ -410,6 +410,7 @@
 	unsigned autotune	: 2;	/* 1=autotune, 2=noautotune, 0=default */
 	unsigned remap_0_to_1	: 2;	/* 0=remap if ezdrive, 1=remap, 2=noremap */
 	unsigned ata_flash	: 1;	/* 1=present, 0=default */
+	unsigned blocked        : 1;	/* 1=powermanagment told us not to do anything, so sleep nicely */
 	unsigned	addressing;	/* : 2; 0=28-bit, 1=48-bit, 2=64-bit */
 	byte		scsi;		/* 0=default, 1=skip current ide-subdriver for ide-scsi emulation */
 	select_t	select;		/* basic drive/head select reg value */

-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: Suspend support for IDE

[Alan Cox]

> +	while (HWGROUP(drive)->handler)
> +		schedule();

You need to yield. Remember the process might be hard real time and blocking
your real work from occuring. while(foo) schedule() is always a bug

> +static int idedisk_resume(struct device *dev, u32 level)
> +{
> +	ide_drive_t *drive = dev->driver_data;
> +	if (!drive->blocked)
> +		panic("ide: Resume but not suspended?\n");
> +	drive->blocked = 0;
> +}

Also remember you must perform the sequences to wake up the drive and
restore the controller logic (and of course in the right order). Newer
disks won't just wake up when fed a random command (eg ibm microdrives)

The suspend order similarly is important - finish the current command,
then flush the disk cache, then when it completes you can tell the drive
to power down. On some systems you want to drop it back to PIO0 non DMA
before the powerdown or S4BIOS restore from disk will fail.

APM generally does all this for you, ACPI won't.

Re: Suspend support for IDE

[benh]

>> +	while (HWGROUP(drive)->handler)
>> +		schedule();
>
>You need to yield. Remember the process might be hard real time and blocking
>your real work from occuring. while(foo) schedule() is always a bug
>
>> +static int idedisk_resume(struct device *dev, u32 level)
>> +{
>> +	ide_drive_t *drive = dev->driver_data;
>> +	if (!drive->blocked)
>> +		panic("ide: Resume but not suspended?\n");
>> +	drive->blocked = 0;
>> +}

I wouldn't do it this way in fact. The IDE disk is a device on the IDE
bus, so it's up to the IDE bus to get the PM requests, and pass them down
appropriately to the low level driver. The IDE common code has to make
sure any further request is properly blocked etc... Then, the controller
itself can eventually be suspended as well.

You can see code that blocks the IDE requests in ide-pmac sleep code,
it isn't perfect (I have a few known issues when waking up from sleep
with mounted CD/DVDs for example) but except from these it appear
to work well. It is currently specific to the pmac PM scheme, but I hope to
find time to port this over to the new macanism and make it more generic
in the future. That code is appropriate for what I need, you may, as Alan
states, need additional care to please your BIOS, like switching back to
PIO0, which means also updating your controller timings.

Ben.

Re: Suspend support for IDE

[Pavel Machek]

Hi!

> > +	while (HWGROUP(drive)->handler)
> > +		schedule();
> 
> You need to yield. Remember the process might be hard real time and blocking
> your real work from occuring. while(foo) schedule() is always a bug

The process calling this is kernel thread doing powermangment; we have
it under full control. yield() is probably more intuitive, through.

> > +static int idedisk_resume(struct device *dev, u32 level)
> > +{
> > +	ide_drive_t *drive = dev->driver_data;
> > +	if (!drive->blocked)
> > +		panic("ide: Resume but not suspended?\n");
> > +	drive->blocked = 0;
> > +}
> 
> Also remember you must perform the sequences to wake up the drive and
> restore the controller logic (and of course in the right order). Newer
> disks won't just wake up when fed a random command (eg ibm
> microdrives)

Wake from S3 or S4 should look like power-up from disks perspective. I
should need no commands to do that.

Restoring right UDMA mode... Well, I'll need to do that,
probably. (What I have there is just enough to prevent disk
corruption. I'm still likely to see some bus resets, but no longer
data loose, I believe.)

> The suspend order similarly is important - finish the current
> command,

The while loop above should make sure no command is happening just
now, right?

> then flush the disk cache, then when it completes you can tell the
> drive

Disks that need cache flush are broken, anyway -- they lied us on
command completion -- right?

> to power down. 

Why should I tell the drive to power down? It is going to loose its
power, anyway (I believe in both S3 and S4).

> On some systems you want to drop it back to PIO0 non DMA
> before the powerdown or S4BIOS restore from disk will fail.


S4BIOS is not on my list just now; agreed it would be better.

-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: Suspend support for IDE

[Alan Cox]

> Wake from S3 or S4 should look like power-up from disks perspective. I
> should need no commands to do that.

Assuming the BIOS didn't do the work you need to bring the disks up as
if you were the BIOS. That means doing the controller configuration, waking
the drive as per ATA6. What does ACPI guarantee here ? If ACPI brough the
drive back into its "happy bios state" then I agree you are right. If its
like an APM resume from suspend to ram then its not always so clear cut.

Also there is the some fun about buggy drives and power up happenings. On no
account can you issue any command that might touch the platter unless you
know the drive is at full running speed when spinning up certain old drives
because the firmware in some cases forgets to check the drive is at speed
and you physically destroy the disk over time. Thankfully thats old old
drives (540Mb quantum if I remember rightly)

> > then flush the disk cache, then when it completes you can tell the
> > drive
> 
> Disks that need cache flush are broken, anyway -- they lied us on
> command completion -- right?

Wrong. Please read the spec. If you are going to be the IDE maintainer you are
wasting your time until you read and understand the specifications. If you
don't do the cache flush you will lose data on some drives. An IDE drive
is permitted to keep a write back cache. Forced writes to the media are in an
upcoming IDE command set draft (although in general you can half fake that
with a read and verify).

> Why should I tell the drive to power down? It is going to loose its
> power, anyway (I believe in both S3 and S4).

So it can shut itself down nicely and do any housework it wants to do
(like flushing the cache if the cache flush command isnt supported.. its
 optional in older ATA standards)

> > On some systems you want to drop it back to PIO0 non DMA
> > before the powerdown or S4BIOS restore from disk will fail.
> 
> S4BIOS is not on my list just now; agreed it would be better.

Alan

Re: Suspend support for IDE

[Barry K. Nathan]

> > Why should I tell the drive to power down? It is going to loose its
> > power, anyway (I believe in both S3 and S4).
> 
> So it can shut itself down nicely and do any housework it wants to do
> (like flushing the cache if the cache flush command isnt supported.. its
>  optional in older ATA standards)

Or, in the case of newer IBM TravelStars, so that the drive can unload
its head properly instead of having to do an uncontrolled emergency unload
that shortens the drive's life and makes an awful screeching noise.

-Barry K. Nathan <barryn@pobox.com>

Re: Suspend support for IDE

[Reid Hekman]

On Sat, 2002-03-09 at 18:52, Barry K. Nathan wrote: 
> > > Why should I tell the drive to power down? It is going to loose its
> > > power, anyway (I believe in both S3 and S4).
> > 
> > So it can shut itself down nicely and do any housework it wants to do
> > (like flushing the cache if the cache flush command isnt supported.. its
> >  optional in older ATA standards)
> 
> Or, in the case of newer IBM TravelStars, so that the drive can unload
> its head properly instead of having to do an uncontrolled emergency unload
> that shortens the drive's life and makes an awful screeching noise.

Eeep. Have we just set the wayback machine for 15 years ago? I don't
remember seeing PARK.COM in /sbin recently. 

Sorry, couldn't resist. 
Reid 



_________________________________________________________
Do You Yahoo!?
Get your free @yahoo.com address at http://mail.yahoo.com

Re: Suspend support for IDE

[Andre Hedrick]

On 10 Mar 2002, Reid Hekman wrote:

> On Sat, 2002-03-09 at 18:52, Barry K. Nathan wrote: 
> > > > Why should I tell the drive to power down? It is going to loose its
> > > > power, anyway (I believe in both S3 and S4).
> > > 
> > > So it can shut itself down nicely and do any housework it wants to do
> > > (like flushing the cache if the cache flush command isnt supported.. its
> > >  optional in older ATA standards)
> > 
> > Or, in the case of newer IBM TravelStars, so that the drive can unload
> > its head properly instead of having to do an uncontrolled emergency unload
> > that shortens the drive's life and makes an awful screeching noise.
> 
> Eeep. Have we just set the wayback machine for 15 years ago? I don't
> remember seeing PARK.COM in /sbin recently. 
> 
> Sorry, couldn't resist. 

No, I put it in the NOTIFY calls with some help from Tim Hockin!
Since the end-user generally does not need to know how to deal w/ these
issues, the kernel had better get smart but who knows if it made in in
there in 2.4 yet and whether it got broke in 2.5 or not.

Regards,

Andre Hedrick
Linux Disk Certification Project                Linux ATA Development

Re: Suspend support for IDE

[Rogier Wolff]

Alan Cox wrote:
> Also there is the some fun about buggy drives and power up happenings. On no
> account can you issue any command that might touch the platter unless you
> know the drive is at full running speed when spinning up certain old drives
> because the firmware in some cases forgets to check the drive is at speed
> and you physically destroy the disk over time. Thankfully thats old old
> drives (540Mb quantum if I remember rightly)

That could be 1.6G or 2.5G WD drives. AC31600, AC32500. 

			Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2137555 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
* There are old pilots, and there are bold pilots. 
* There are also old, bald pilots. 

Re: Suspend support for IDE

[Derek J Witt]

Does the IDE blacklist include those drives that malfunction if they're not
using the correct connector on the IDE ribbon cable?

I found out a few weeks ago that my Caviar WD36400 is  that particular.
Upon putting that drive back on the middle connector on secondary, my box
was then able to suspend and resume without the drive disconnecting on me.

-- Derek Witt.

----- Original Message -----
From: "Rogier Wolff" <R.E.Wolff@BitWizard.nl>
To: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
Cc: "Pavel Machek" <pavel@ucw.cz>; <dalecki@evision-ventures.com>; "kernel
list" <linux-kernel@vger.kernel.org>
Sent: Sunday, March 10, 2002 2:23 AM
Subject: Re: Suspend support for IDE


> Alan Cox wrote:
> > Also there is the some fun about buggy drives and power up happenings.
On no
> > account can you issue any command that might touch the platter unless
you
> > know the drive is at full running speed when spinning up certain old
drives
> > because the firmware in some cases forgets to check the drive is at
speed
> > and you physically destroy the disk over time. Thankfully thats old old
> > drives (540Mb quantum if I remember rightly)
>
> That could be 1.6G or 2.5G WD drives. AC31600, AC32500.
>
> Roger.
>
> --
> ** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2137555 **
> *-- BitWizard writes Linux device drivers for any device you may have! --*
> * There are old pilots, and there are bold pilots.
> * There are also old, bald pilots.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: Suspend support for IDE

[Alan Cox]

> Does the IDE blacklist include those drives that malfunction if they're not
> using the correct connector on the IDE ribbon cable?

Thats not something the drive is even aware of unless the cable is using 
CS (cable select) in which case its a bios/host master-slave issue

Re: Suspend support for IDE

[Pavel Machek]

Hi!

> > > then flush the disk cache, then when it completes you can tell the
> > > drive
> > 
> > Disks that need cache flush are broken, anyway -- they lied us on
> > command completion -- right?
> 
> Wrong. Please read the spec. If you are going to be the IDE
> maintainer you are

I'm *NOT* going to be the IDE maintainer. Here's pavel!
									Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Suspend support for IDE

[Martin Dalecki]

Pavel Machek wrote:

> Wake from S3 or S4 should look like power-up from disks perspective. I
> should need no commands to do that.
> 
> Restoring right UDMA mode... Well, I'll need to do that,
> probably. (What I have there is just enough to prevent disk
> corruption. I'm still likely to see some bus resets, but no longer
> data loose, I believe.)

Yeep. But doing this will only be possible in few weeks, when
the corresponding setup code is really modularized and note
the current "ifdef whatever" buch of collected fixes.
However for certain your patch is a starting point and
we are in odd series anyway...

>>The suspend order similarly is important - finish the current
>>command,
>>
> 
> The while loop above should make sure no command is happening just
> now, right?

Beware of the long houl interrupts found by ide_add_timer

>>then flush the disk cache, then when it completes you can tell the
>>drive
>>
> 
> Disks that need cache flush are broken, anyway -- they lied us on
> command completion -- right?

;-)... And the games they wan't to play on the data registers are
silly as well... if you ask me... well personally they are running
directly into a "backword compatibility to our own mistakes" trap.
The only comparable thing to this was the QIC interfaces abusing the
floppy disk stepper line for *serial* command transfer.

Re: Suspend support for IDE

[Martin Dalecki]

Pavel Machek wrote:
> Hi!
> 
> This adds driver support to ide-disk.c. Does it look good to you? [If
> so, applying it will not hurt, altrough it might change in near
> future.]

Since I'm open for change anyway and it apprently doesn't hurt my
eyes ;-). I'm gald to include it. (God would I love to
delete the silly suspen partition on my noebook and suspend to
file instead!).

(I will just replace the panic check by a BUG_ON check...)

Tank you very much for your efforts to implement this!

> --- clean.pre/drivers/ide/ide-disk.c	Fri Mar  8 18:40:34 2002
> +++ linux-dm.pre/drivers/ide/ide-disk.c	Fri Mar  8 18:40:07 2002
> @@ -123,6 +123,8 @@
>   */
>  static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
>  {
> +	if (drive->blocked)
> +		panic("ide: Request while drive blocked? You don't like your data intact?");
>  	if (!(rq->flags & REQ_CMD)) {
>  		blk_dump_rq_flags(rq, "do_rw_disk, bad command");
>  		ide_end_request(drive, 0);
> @@ -910,13 +912,36 @@
>  	ide_add_setting(drive,	"max_failures",		SETTING_RW,					-1,			-1,			TYPE_INT,	0,	65535,				1,	1,	&drive->max_failures,		NULL);
>  }
>  
> +static int idedisk_suspend(struct device *dev, u32 state, u32 level)
> +{
> +	int i;
> +	ide_drive_t *drive = dev->driver_data;
> +
> +	printk("ide_disk_suspend()\n");
> +	while (HWGROUP(drive)->handler)
> +		schedule();
> +	drive->blocked = 1;
> +}
> +
> +static int idedisk_resume(struct device *dev, u32 level)
> +{
> +	ide_drive_t *drive = dev->driver_data;
> +	if (!drive->blocked)
> +		panic("ide: Resume but not suspended?\n");
> +	drive->blocked = 0;
> +}
> +
> +
>  /* This is just a hook for the overall driver tree.
>   *
>   * FIXME: This is soon goig to replace the custom linked list games played up
>   * to great extend between the different components of the IDE drivers.
>   */
>  
> -static struct device_driver idedisk_devdrv = {};
> +static struct device_driver idedisk_devdrv = {
> +	suspend: idedisk_suspend,
> +	resume: idedisk_resume,
> +};
>  
>  static void idedisk_setup(ide_drive_t *drive)
>  {
> @@ -963,6 +988,7 @@
>  	    sprintf(drive->device.name, "ide-disk");
>  	    drive->device.driver = &idedisk_devdrv;
>  	    drive->device.parent = &HWIF(drive)->device;
> +	    drive->device.driver_data = drive;
>  	    device_register(&drive->device);
>  	}
>  
> --- clean.pre/include/linux/ide.h	Fri Mar  8 18:40:38 2002
> +++ linux-dm.pre/include/linux/ide.h	Fri Mar  8 18:37:44 2002
> @@ -410,6 +410,7 @@
>  	unsigned autotune	: 2;	/* 1=autotune, 2=noautotune, 0=default */
>  	unsigned remap_0_to_1	: 2;	/* 0=remap if ezdrive, 1=remap, 2=noremap */
>  	unsigned ata_flash	: 1;	/* 1=present, 0=default */
> +	unsigned blocked        : 1;	/* 1=powermanagment told us not to do anything, so sleep nicely */
>  	unsigned	addressing;	/* : 2; 0=28-bit, 1=48-bit, 2=64-bit */
>  	byte		scsi;		/* 0=default, 1=skip current ide-subdriver for ide-scsi emulation */
>  	select_t	select;		/* basic drive/head select reg value */
> 
> 



-- 
- phone: +49 214 8656 283
- job:   eVision-Ventures AG, LEV .de (MY OPINIONS ARE MY OWN!)
- langs: de_DE.ISO8859-1, en_US, pl_PL.ISO8859-2, last ressort: ru_RU.KOI8-R

Re: Suspend support for IDE

[Derek J Witt]

yeah, these are 18" cables. I previously had my caviar setup as a secondary
master and not cable-select as far I could remember.  I  was using the end
connector due to space constraints in the case and the cable-length
restraints. I did attempt to have both hard drives on separate controllers.
I was trying to setup my swaps in a raid-like fashion. Once  I moved my
drives around in the case, I moved the caviar back to primary slave using
the end connector. It was using the end connector on secondary master due
not being able to stretch the middle connector to the caviar.

I was having the cable twist some strange ways just to get the cable to fit
properly.    This hard drive came out of an old gateway. It was setup as a
slave in that box too.

I suppose risking screwing up the IDE cable just to get it to  reach or fit
properly in a mid-tower isn't worth losing data via CRC errors...

I now have this IDE setup:

Primary master: Quantum Fireball SE6.4A
Primary slave:  Western Digital WDC AC36400L
Secondary master: Acer ATAPI CD-ROM 40x N0AP
Secondary slave: Acer ATAPI CD-RW 2x6x CRW6206A.

I got the CD-ROM in the middle 5 1/4" bay, the CD-RW in the bottom 5 1/4"
bay, the 3 1/2" floppy in the top 3 1/2" bay, the Caviar in the middle 3
1/2" bay, and the Quantum in the 3rd 3 1/2" bay below the floppy.  I was
using an  IDE cable from the old AT case I had. I think it was a faulty
cable in my situation. I have since replaced the cable and the cd drives are
working just fine even with dma enabled. I'm using an ATA66 cable for the
hard drives.

-- Derek Witt.

----- Original Message -----
From: "Mark Hahn" <hahn@physics.mcmaster.ca>
To: "Derek J Witt" <djw@flinthills.com>
Sent: Sunday, March 10, 2002 5:02 PM
Subject: Re: Suspend support for IDE


> > Does the IDE blacklist include those drives that malfunction if they're
not
> > using the correct connector on the IDE ribbon cable?
>
> no.  there is no specificity for connector unless you're using
cable-select.
>
> > I found out a few weeks ago that my Caviar WD36400 is  that particular.
> > Upon putting that drive back on the middle connector on secondary, my
box
> > was then able to suspend and resume without the drive disconnecting on
me.
>
> are your cables <=18", with both *ends* plugged in?  the spec requires
that.
>
>

Re: Suspend support for IDE

[Mark Hahn]

> I suppose risking screwing up the IDE cable just to get it to  reach or fit
> properly in a mid-tower isn't worth losing data via CRC errors...

uh, that's an oxymoron - you specifically don't lose data because 
of CRC checking.  

> I now have this IDE setup:
> 
> Primary master: Quantum Fireball SE6.4A
> Primary slave:  Western Digital WDC AC36400L
> Secondary master: Acer ATAPI CD-ROM 40x N0AP
> Secondary slave: Acer ATAPI CD-RW 2x6x CRW6206A.

it's fairly common to have drives from different vendors, 
especially oldish ones, not get along as master/slave.

> working just fine even with dma enabled. I'm using an ATA66 cable for the
> hard drives.

not to be *too* persnickety, but there's no such thing as "ata66 cable", 
just 40 or 80-conductor...