[ide-dev 3/5] generic Power Management for IDE devices

[ide-dev 3/5] generic Power Management for IDE devices

[Bartlomiej Zolnierkiewicz]

Move PM code from ide-cd.c and ide-disk.c to IDE core so:
* PM is supported for other ATAPI devices (floppy, tape)
* PM is supported even if specific driver is not loaded

diff -Nru a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
--- a/drivers/ide/ide-cd.c	2005-01-21 23:53:31 +01:00
+++ b/drivers/ide/ide-cd.c	2005-01-21 23:53:31 +01:00
@@ -3251,45 +3251,6 @@

 static int ide_cdrom_attach (ide_drive_t *drive);

-/*
- * Power Management state machine.
- *
- * We don't do much for CDs right now.
- */
-
-static void ide_cdrom_complete_power_step (ide_drive_t *drive, struct request *rq, u8 stat, u8 error)
-{
-}
-
-static ide_startstop_t ide_cdrom_start_power_step (ide_drive_t *drive, struct request *rq)
-{
-	ide_task_t *args = rq->special;
-
-	memset(args, 0, sizeof(*args));
-
-	switch (rq->pm->pm_step) {
-	case ide_pm_state_start_suspend:
-		break;
-
-	case ide_pm_state_start_resume:	/* Resume step 1 (restore DMA) */
-		/*
-		 * Right now, all we do is call hwif->ide_dma_check(drive),
-		 * we could be smarter and check for current xfer_speed
-		 * in struct drive etc...
-		 * Also, this step could be implemented as a generic helper
-		 * as most subdrivers will use it.
-		 */
-		if ((drive->id->capability & 1) == 0)
-			break;
-		if (HWIF(drive)->ide_dma_check == NULL)
-			break;
-		HWIF(drive)->ide_dma_check(drive);
-		break;
-	}
-	rq->pm->pm_step = ide_pm_state_completed;
-	return ide_stopped;
-}
-
 static ide_driver_t ide_cdrom_driver = {
 	.owner			= THIS_MODULE,
 	.name			= "ide-cdrom",
@@ -3302,8 +3263,6 @@
 	.capacity		= ide_cdrom_capacity,
 	.attach			= ide_cdrom_attach,
 	.drives			= LIST_HEAD_INIT(ide_cdrom_driver.drives),
-	.start_power_step	= ide_cdrom_start_power_step,
-	.complete_power_step	= ide_cdrom_complete_power_step,
 };

 static int idecd_open(struct inode * inode, struct file * file)
diff -Nru a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c	2005-01-21 23:53:31 +01:00
+++ b/drivers/ide/ide-disk.c	2005-01-21 23:53:31 +01:00
@@ -855,90 +855,6 @@
  	ide_add_setting(drive,	"max_failures",		SETTING_RW,					-1,			-1,			TYPE_INT,	0,	65535,				1,	1,	&drive->max_failures,		NULL);
 }

-/*
- * Power Management state machine. This one is rather trivial for now,
- * we should probably add more, like switching back to PIO on suspend
- * to help some BIOSes, re-do the door locking on resume, etc...
- */
-
-enum {
-	idedisk_pm_flush_cache	= ide_pm_state_start_suspend,
-	idedisk_pm_standby,
-
-	idedisk_pm_idle		= ide_pm_state_start_resume,
-	idedisk_pm_restore_dma,
-};
-
-static void idedisk_complete_power_step (ide_drive_t *drive, struct request *rq, u8 stat, u8 error)
-{
-	switch (rq->pm->pm_step) {
-	case idedisk_pm_flush_cache:	/* Suspend step 1 (flush cache) complete */
-		if (rq->pm->pm_state == 4)
-			rq->pm->pm_step = ide_pm_state_completed;
-		else
-			rq->pm->pm_step = idedisk_pm_standby;
-		break;
-	case idedisk_pm_standby:	/* Suspend step 2 (standby) complete */
-		rq->pm->pm_step = ide_pm_state_completed;
-		break;
-	case idedisk_pm_idle:		/* Resume step 1 (idle) complete */
-		rq->pm->pm_step = idedisk_pm_restore_dma;
-		break;
-	}
-}
-
-static ide_startstop_t idedisk_start_power_step (ide_drive_t *drive, struct request *rq)
-{
-	ide_task_t *args = rq->special;
-
-	memset(args, 0, sizeof(*args));
-
-	switch (rq->pm->pm_step) {
-	case idedisk_pm_flush_cache:	/* Suspend step 1 (flush cache) */
-		/* Not supported? Switch to next step now. */
-		if (!drive->wcache || !ide_id_has_flush_cache(drive->id)) {
-			idedisk_complete_power_step(drive, rq, 0, 0);
-			return ide_stopped;
-		}
-		if (ide_id_has_flush_cache_ext(drive->id))
-			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
-		else
-			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
-		args->command_type = IDE_DRIVE_TASK_NO_DATA;
-		args->handler	   = &task_no_data_intr;
-		return do_rw_taskfile(drive, args);
-
-	case idedisk_pm_standby:	/* Suspend step 2 (standby) */
-		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_STANDBYNOW1;
-		args->command_type = IDE_DRIVE_TASK_NO_DATA;
-		args->handler	   = &task_no_data_intr;
-		return do_rw_taskfile(drive, args);
-
-	case idedisk_pm_idle:		/* Resume step 1 (idle) */
-		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_IDLEIMMEDIATE;
-		args->command_type = IDE_DRIVE_TASK_NO_DATA;
-		args->handler = task_no_data_intr;
-		return do_rw_taskfile(drive, args);
-
-	case idedisk_pm_restore_dma:	/* Resume step 2 (restore DMA) */
-		/*
-		 * Right now, all we do is call hwif->ide_dma_check(drive),
-		 * we could be smarter and check for current xfer_speed
-		 * in struct drive etc...
-		 * Also, this step could be implemented as a generic helper
-		 * as most subdrivers will use it
-		 */
-		if ((drive->id->capability & 1) == 0)
-			break;
-		if (HWIF(drive)->ide_dma_check == NULL)
-			break;
-		HWIF(drive)->ide_dma_check(drive);
-		break;
-	}
-	rq->pm->pm_step = ide_pm_state_completed;
-	return ide_stopped;
-}
-
 static void idedisk_setup (ide_drive_t *drive)
 {
 	struct hd_driveid *id = drive->id;
@@ -1181,8 +1097,6 @@
 	.proc			= idedisk_proc,
 	.attach			= idedisk_attach,
 	.drives			= LIST_HEAD_INIT(idedisk_driver.drives),
-	.start_power_step	= idedisk_start_power_step,
-	.complete_power_step	= idedisk_complete_power_step,
 };

 static int idedisk_open(struct inode *inode, struct file *filp)
diff -Nru a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c	2005-01-21 23:53:31 +01:00
+++ b/drivers/ide/ide-io.c	2005-01-21 23:53:31 +01:00
@@ -189,6 +189,93 @@
 }
 EXPORT_SYMBOL(ide_end_request);

+/*
+ * Power Management state machine. This one is rather trivial for now,
+ * we should probably add more, like switching back to PIO on suspend
+ * to help some BIOSes, re-do the door locking on resume, etc...
+ */
+
+enum {
+	ide_pm_flush_cache	= ide_pm_state_start_suspend,
+	idedisk_pm_standby,
+
+	idedisk_pm_idle		= ide_pm_state_start_resume,
+	ide_pm_restore_dma,
+};
+
+static void ide_complete_power_step(ide_drive_t *drive, struct request *rq, u8 stat, u8 error)
+{
+	if (drive->media != ide_disk)
+		return;
+
+	switch (rq->pm->pm_step) {
+	case ide_pm_flush_cache:	/* Suspend step 1 (flush cache) complete */
+		if (rq->pm->pm_state == 4)
+			rq->pm->pm_step = ide_pm_state_completed;
+		else
+			rq->pm->pm_step = idedisk_pm_standby;
+		break;
+	case idedisk_pm_standby:	/* Suspend step 2 (standby) complete */
+		rq->pm->pm_step = ide_pm_state_completed;
+		break;
+	case idedisk_pm_idle:		/* Resume step 1 (idle) complete */
+		rq->pm->pm_step = ide_pm_restore_dma;
+		break;
+	}
+}
+
+static ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
+{
+	ide_task_t *args = rq->special;
+
+	memset(args, 0, sizeof(*args));
+
+	switch (rq->pm->pm_step) {
+	case ide_pm_flush_cache:	/* Suspend step 1 (flush cache) */
+		if (drive->media != ide_disk)
+			break;
+		/* Not supported? Switch to next step now. */
+		if (!drive->wcache || !ide_id_has_flush_cache(drive->id)) {
+			ide_complete_power_step(drive, rq, 0, 0);
+			return ide_stopped;
+		}
+		if (ide_id_has_flush_cache_ext(drive->id))
+			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
+		else
+			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
+		args->command_type = IDE_DRIVE_TASK_NO_DATA;
+		args->handler	   = &task_no_data_intr;
+		return do_rw_taskfile(drive, args);
+
+	case idedisk_pm_standby:	/* Suspend step 2 (standby) */
+		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_STANDBYNOW1;
+		args->command_type = IDE_DRIVE_TASK_NO_DATA;
+		args->handler	   = &task_no_data_intr;
+		return do_rw_taskfile(drive, args);
+
+	case idedisk_pm_idle:		/* Resume step 1 (idle) */
+		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_IDLEIMMEDIATE;
+		args->command_type = IDE_DRIVE_TASK_NO_DATA;
+		args->handler = task_no_data_intr;
+		return do_rw_taskfile(drive, args);
+
+	case ide_pm_restore_dma:	/* Resume step 2 (restore DMA) */
+		/*
+		 * Right now, all we do is call hwif->ide_dma_check(drive),
+		 * we could be smarter and check for current xfer_speed
+		 * in struct drive etc...
+		 */
+		if ((drive->id->capability & 1) == 0)
+			break;
+		if (drive->hwif->ide_dma_check == NULL)
+			break;
+		drive->hwif->ide_dma_check(drive);
+		break;
+	}
+	rq->pm->pm_step = ide_pm_state_completed;
+	return ide_stopped;
+}
+
 /**
  *	ide_complete_pm_request - end the current Power Management request
  *	@drive: target drive
@@ -408,7 +495,7 @@
 		printk("%s: complete_power_step(step: %d, stat: %x, err: %x)\n",
 			drive->name, rq->pm->pm_step, stat, err);
 #endif
-		DRIVER(drive)->complete_power_step(drive, rq, stat, err);
+		ide_complete_power_step(drive, rq, stat, err);
 		if (rq->pm->pm_step == ide_pm_state_completed)
 			ide_complete_pm_request(drive, rq);
 		return;
@@ -905,7 +992,7 @@
 			printk("%s: start_power_step(step: %d)\n",
 				drive->name, rq->pm->pm_step);
 #endif
-			startstop = DRIVER(drive)->start_power_step(drive, rq);
+			startstop = ide_start_power_step(drive, rq);
 			if (startstop == ide_stopped &&
 			    rq->pm->pm_step == ide_pm_state_completed)
 				ide_complete_pm_request(drive, rq);
diff -Nru a/drivers/ide/ide.c b/drivers/ide/ide.c
--- a/drivers/ide/ide.c	2005-01-21 23:53:31 +01:00
+++ b/drivers/ide/ide.c	2005-01-21 23:53:31 +01:00
@@ -2060,13 +2060,6 @@
 	return __ide_abort(drive, rq);
 }

-static ide_startstop_t default_start_power_step(ide_drive_t *drive,
-						struct request *rq)
-{
-	rq->pm->pm_step = ide_pm_state_completed;
-	return ide_stopped;
-}
-
 static void setup_driver_defaults (ide_driver_t *d)
 {
 	BUG_ON(d->attach == NULL || d->cleanup == NULL);
@@ -2078,8 +2071,6 @@
 	if (d->pre_reset == NULL)	d->pre_reset = default_pre_reset;
 	if (d->capacity == NULL)	d->capacity = default_capacity;
 	if (d->special == NULL)		d->special = default_special;
-	if (d->start_power_step == NULL)
-		d->start_power_step = default_start_power_step;
 }

 int ide_register_subdriver(ide_drive_t *drive, ide_driver_t *driver)
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	2005-01-21 23:53:31 +01:00
+++ b/include/linux/ide.h	2005-01-21 23:53:31 +01:00
@@ -1106,8 +1106,6 @@
 	int		(*attach)(ide_drive_t *);
 	void		(*ata_prebuilder)(ide_drive_t *);
 	void		(*atapi_prebuilder)(ide_drive_t *);
-	ide_startstop_t	(*start_power_step)(ide_drive_t *, struct request *);
-	void		(*complete_power_step)(ide_drive_t *, struct request *, u8, u8);
 	struct device_driver	gen_driver;
 	struct list_head drives;
 	struct list_head drivers;

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Pavel Machek]

Hi!

> Move PM code from ide-cd.c and ide-disk.c to IDE core so:
> * PM is supported for other ATAPI devices (floppy, tape)
> * PM is supported even if specific driver is not loaded

Why do you need to have state-machine? During suspend we are running
single-threaded, it should be okay to just do the calls directly.
				Pavel

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Bartlomiej Zolnierkiewicz]

On Sat, 22 Jan 2005 19:41:24 +0100, Pavel Machek <pavel@suse.cz> wrote:
> Hi!
> 
> > Move PM code from ide-cd.c and ide-disk.c to IDE core so:
> > * PM is supported for other ATAPI devices (floppy, tape)
> > * PM is supported even if specific driver is not loaded
> 
> Why do you need to have state-machine? During suspend we are running
> single-threaded, it should be okay to just do the calls directly.
>                                 Pavel

If we are running single-threaded I also see no reason for state-machine.
Ben?

Pavel, I assume that changes contained in the patch are OK with you?

Bartlomiej

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Pavel Machek]

Hi!

> > > Move PM code from ide-cd.c and ide-disk.c to IDE core so:
> > > * PM is supported for other ATAPI devices (floppy, tape)
> > > * PM is supported even if specific driver is not loaded
> > 
> > Why do you need to have state-machine? During suspend we are running
> > single-threaded, it should be okay to just do the calls directly.
> >                                 Pavel
> 
> If we are running single-threaded I also see no reason for state-machine.
> Ben?
> 
> Pavel, I assume that changes contained in the patch are OK with you?

I do not think I looked too closely but yes, they are probably ok.

								Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Benjamin Herrenschmidt]

On Wed, 2005-02-02 at 00:03 +0100, Bartlomiej Zolnierkiewicz wrote:
> On Sat, 22 Jan 2005 19:41:24 +0100, Pavel Machek <pavel@suse.cz> wrote:
> > Hi!
> > 
> > > Move PM code from ide-cd.c and ide-disk.c to IDE core so:
> > > * PM is supported for other ATAPI devices (floppy, tape)
> > > * PM is supported even if specific driver is not loaded
> > 
> > Why do you need to have state-machine? During suspend we are running
> > single-threaded, it should be okay to just do the calls directly.
> >                                 Pavel
> 
> If we are running single-threaded I also see no reason for state-machine.
> Ben?
> 
> Pavel, I assume that changes contained in the patch are OK with you?

We aren't always running single threaded. On PPC we are definitely
not :)

Also, we need the request to go down the queue for proper
synchronisation. It's all been discussed at lenght a while ago, this is
the best way to do it. Also, wakeup can be asynchronous this way.

Ben.

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Pavel Machek]

Hi!

> > > > Move PM code from ide-cd.c and ide-disk.c to IDE core so:
> > > > * PM is supported for other ATAPI devices (floppy, tape)
> > > > * PM is supported even if specific driver is not loaded
> > > 
> > > Why do you need to have state-machine? During suspend we are running
> > > single-threaded, it should be okay to just do the calls directly.
> > 
> > If we are running single-threaded I also see no reason for state-machine.
> > Ben?
> > 
> > Pavel, I assume that changes contained in the patch are OK with you?
> 
> We aren't always running single threaded. On PPC we are definitely
> not :)

Ok, Ben is right, I forgot about PPC.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Alan Cox]

On Maw, 2005-02-01 at 23:03, Bartlomiej Zolnierkiewicz wrote:
> On Sat, 22 Jan 2005 19:41:24 +0100, Pavel Machek <pavel@suse.cz> wrote:
> > Why do you need to have state-machine? During suspend we are running
> > single-threaded, it should be okay to just do the calls directly.
> >                                 Pavel
> 
> If we are running single-threaded I also see no reason for state-machine.
> Ben?

There may be outstanding I/O running at the time of the suspend. You
want to keep everything nicely ordered. The state machine suspend code
looks to me the right answer and is cleaner.

Re: [ide-dev 3/5] generic Power Management for IDE devices

[Bartlomiej Zolnierkiewicz]

On Thu, 03 Feb 2005 10:03:00 +0000, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Maw, 2005-02-01 at 23:03, Bartlomiej Zolnierkiewicz wrote:
> > On Sat, 22 Jan 2005 19:41:24 +0100, Pavel Machek <pavel@suse.cz> wrote:
> > > Why do you need to have state-machine? During suspend we are running
> > > single-threaded, it should be okay to just do the calls directly.
> > >                                 Pavel
> >
> > If we are running single-threaded I also see no reason for state-machine.
> > Ben?
> 
> There may be outstanding I/O running at the time of the suspend. You
> want to keep everything nicely ordered. The state machine suspend code
> looks to me the right answer and is cleaner.

Outstanding I/Os won't be a problem - suspend will be done as one
request.  Anyway, state machine is not going away.