Kill obsolete and unused suspend/resume code from IDE

Kill obsolete and unused suspend/resume code from IDE

[Pavel Machek]

Hi!

This code in ide is obsolete and unused. I have followup patch to
integrate IDE into sysfs. Please apply,
								Pavel

--- clean/drivers/ide/ide-disk.c	2002-11-01 00:37:14.000000000 +0100
+++ linux-swsusp/drivers/ide/ide-disk.c	2002-11-06 14:33:35.000000000 +0100
@@ -1412,24 +1412,6 @@
 	return call_idedisk_standby(drive, 0);
 }
 
-static int call_idedisk_suspend (ide_drive_t *drive, int arg)
-{
-	ide_task_t args;
-	u8 suspend = (arg) ? WIN_SLEEPNOW2 : WIN_SLEEPNOW1;
-	memset(&args, 0, sizeof(ide_task_t));
-	args.tfRegister[IDE_COMMAND_OFFSET]	= suspend;
-	args.command_type			= ide_cmd_type_parser(&args);
-	return ide_raw_taskfile(drive, &args, NULL);
-}
-
-static int do_idedisk_suspend (ide_drive_t *drive)
-{
-	if (drive->suspend_reset)
-		return 1;
-
-	return call_idedisk_suspend(drive, 0);
-}
-
 #if 0
 static int call_idedisk_checkpower (ide_drive_t *drive, int arg)
 {
@@ -1456,13 +1438,6 @@
 }
 #endif
 
-static int do_idedisk_resume (ide_drive_t *drive)
-{
-	if (!drive->suspend_reset)
-		return 1;
-	return 0;
-}
-
 static int do_idedisk_flushcache (ide_drive_t *drive)
 {
 	ide_task_t args;
@@ -1671,6 +1693,7 @@
 {
 	struct gendisk *g = drive->disk;
 
+	do_idedisk_standby(drive);
 	if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
 		if (do_idedisk_flushcache(drive))
 			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
@@ -1696,9 +1719,6 @@
 	.supports_dma		= 1,
 	.supports_dsc_overlap	= 0,
 	.cleanup		= idedisk_cleanup,
-	.standby		= do_idedisk_standby,
-	.suspend		= do_idedisk_suspend,
-	.resume			= do_idedisk_resume,
 	.flushcache		= do_idedisk_flushcache,
 	.do_request		= do_rw_disk,
 	.sense			= idedisk_dump_status,
--- clean/drivers/ide/ide.c	2002-11-07 16:39:48.000000000 +0100
+++ linux-swsusp/drivers/ide/ide.c	2002-11-07 17:19:42.000000000 +0100
@@ -3110,21 +3110,6 @@
 #endif
 }
 
-static int default_standby (ide_drive_t *drive)
-{
-	return 0;
-}
-
-static int default_suspend (ide_drive_t *drive)
-{
-	return 0;
-}
-
-static int default_resume (ide_drive_t *drive)
-{
-	return 0;
-}
-
 static int default_flushcache (ide_drive_t *drive)
 {
 	return 0;
@@ -3181,9 +3166,6 @@
 {
 	ide_driver_t *d = drive->driver;
 
-	if (d->standby == NULL)		d->standby = default_standby;
-	if (d->suspend == NULL)		d->suspend = default_suspend;
-	if (d->resume == NULL)		d->resume = default_resume;
 	if (d->flushcache == NULL)	d->flushcache = default_flushcache;
 	if (d->do_request == NULL)	d->do_request = default_do_request;
 	if (d->end_request == NULL)	d->end_request = default_end_request;
@@ -3261,13 +3243,8 @@
 	ide_drive_t * drive = container_of(dev,ide_drive_t,gendev);
 	ide_driver_t * driver = drive->driver;
 
-	if (driver) {
-		if (driver->standby)
-			driver->standby(drive);
-		if (driver->cleanup)
-			driver->cleanup(drive);
-	}
-	
+	if (driver && driver->cleanup)
+		driver->cleanup(drive);
 	return 0;
 }
 
--- clean/include/linux/ide.h	2002-11-01 00:37:40.000000000 +0100
+++ linux-swsusp/include/linux/ide.h	2002-11-06 14:07:05.000000000 +0100
@@ -1180,9 +1180,6 @@
 	unsigned supports_dma		: 1;
 	unsigned supports_dsc_overlap	: 1;
 	int		(*cleanup)(ide_drive_t *);
-	int		(*standby)(ide_drive_t *);
-	int		(*suspend)(ide_drive_t *);
-	int		(*resume)(ide_drive_t *);
 	int		(*flushcache)(ide_drive_t *);
 	ide_startstop_t	(*do_request)(ide_drive_t *, struct request *, sector_t);
 	int		(*end_request)(ide_drive_t *, int, int);

-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?

Re: Kill obsolete and unused suspend/resume code from IDE

[Alan Cox]

> This code in ide is obsolete and unused. I have followup patch to
> integrate IDE into sysfs. Please apply,

The code is not obsolete, it is not unused

> +	do_idedisk_standby(drive);
>  	if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
>  		if (do_idedisk_flushcache(drive))
>  			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",

What locking rules are you using here ?

Linus please reject this patch. Its just getting in the way of actually
fixing the IDE code properly.

Re: Kill obsolete and unused suspend/resume code from IDE

[Pavel Machek]

Hi!

> > This code in ide is obsolete and unused. I have followup patch to
> > integrate IDE into sysfs. Please apply,
> 
> The code is not obsolete, it is not unused.

Can you point out what uses it? I certainly could not find it and
killing standby/etc produced no compilation errors.

As of obsolete... It is doing power managment outside of sysfs
framework... Which is not how it should be done.

> > +	do_idedisk_standby(drive);
> >  	if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
> >  		if (do_idedisk_flushcache(drive))
> >  			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
> 
> What locking rules are you using here ?

I did not change the code, it works exactly as it did before (ide disk
is never ATAPI device). If it is broken then sorry (but it was broken
before).

> Linus please reject this patch. Its just getting in the way of actually
> fixing the IDE code properly.

Linus actually asked me for the patch, and discussion about it was
public, then he silently droped it. This was "only" retransmit.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Kill obsolete and unused suspend/resume code from IDE

[Pavel Machek]

Hi!

> With the shutdown/cleanup split so the locking works out you might
> actually be able to do what you want (although I dont think you can get
> all the locking logic right yet because some of it is still hosed in the
> ide core). Also take a glance at the SC1200 driver with regards to the
> sysfs based power management handling.

I'll take a look.

> [Linus you can apply this if you want - it fixes
> 	Serverworks /proc
> 	Adds SC1200
> 	Short term fix for simplex DMA devices
> 	Fixes PCMCIA ide eject
> 	Splits IDE I/O from the registration code
> 	Makes the argument names saner
> ]
> 
>  	int		(*standby)(ide_drive_t *);
>  	int		(*suspend)(ide_drive_t *);
>  	int		(*resume)(ide_drive_t *);

Can you show me who calls these 3 callbacks?

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Kill obsolete and unused suspend/resume code from IDE

[Pavel Machek]

Hi!

> With the shutdown/cleanup split so the locking works out you might
> actually be able to do what you want (although I dont think you can get
> all the locking logic right yet because some of it is still hosed in the
> ide core). Also take a glance at the SC1200 driver with regards to the
> sysfs based power management handling.

SC1200 power managment looks sane. Questions:

Don't you need to wait for current operation to complete before
calling sc1200_spindown_drive?

I noticed you are not playing any tricks with inserting requests to
queues... This is basically what I was doing, so I like it.

Is sc1200_spindown_drive really sc1200 specific? Is there some problem
with doing suspend at ide.c layer, and resume (which is controller
specific) in each driver?

...Ugh. You are doing it as pci device, not as generic device. I
believe you should use sysfs directly.

Anyway I can live with this, and its way better than whats in there
just now.
								Pavel

-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Kill obsolete and unused suspend/resume code from IDE

[Pavel Machek]

Hi!

> > As of obsolete... It is doing power managment outside of sysfs
> > framework... Which is not how it should be done.
> > 
> > > > +	do_idedisk_standby(drive);
> > > >  	if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
> > > >  		if (do_idedisk_flushcache(drive))
> > > >  			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
> > > 
> > > What locking rules are you using here ?
> 
> flushcache takes the ide lock which is already held at that point. The
> code splits the unregister/standby path in two so that the unregister
> doesnt deadlock
> 
> > Linus actually asked me for the patch, and discussion about it was
> > public, then he silently droped it. This was "only" retransmit.
> 
> Ok.
> 
> I think your flushcache thing is safe in the new code, and I'm more than
> happy for the PM changes against the new code to use sysfs to go into
> the tree. 2.5.47-ac2 has pretty current ide - there isnt anything going
> to get moved around dramatically again just yet (I wamt to move the
> disk, tape, floppy,... drivers into a subdir later but not yet)

So, if I do basically same patch, but against 2.5.47-ac2 and submit it
to you, it might actually be acceptable? If so its good news.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Kill obsolete and unused suspend/resume code from IDE

[Alan Cox]

On Wed, 2002-11-13 at 13:42, Pavel Machek wrote:
> > The code is not obsolete, it is not unused.
> 
> Can you point out what uses it? I certainly could not find it and
> killing standby/etc produced no compilation errors.
>
You were right - actually the code that uses it isnt committed into the
tree. Its also not going to be now you are doing sysfs

> As of obsolete... It is doing power managment outside of sysfs
> framework... Which is not how it should be done.
> 
> > > +	do_idedisk_standby(drive);
> > >  	if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
> > >  		if (do_idedisk_flushcache(drive))
> > >  			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
> > 
> > What locking rules are you using here ?

flushcache takes the ide lock which is already held at that point. The
code splits the unregister/standby path in two so that the unregister
doesnt deadlock

> Linus actually asked me for the patch, and discussion about it was
> public, then he silently droped it. This was "only" retransmit.

Ok.

I think your flushcache thing is safe in the new code, and I'm more than
happy for the PM changes against the new code to use sysfs to go into
the tree. 2.5.47-ac2 has pretty current ide - there isnt anything going
to get moved around dramatically again just yet (I wamt to move the
disk, tape, floppy,... drivers into a subdir later but not yet)

Alan

Re: Kill obsolete and unused suspend/resume code from IDE

[Alan Cox]

On Wed, 2002-11-13 at 14:13, Pavel Machek wrote:
> I noticed you are not playing any tricks with inserting requests to
> queues... This is basically what I was doing, so I like it.
> 
> Is sc1200_spindown_drive really sc1200 specific? Is there some problem
> with doing suspend at ide.c layer, and resume (which is controller
> specific) in each driver?
> 
> ...Ugh. You are doing it as pci device, not as generic device. I
> believe you should use sysfs directly.
> 
> Anyway I can live with this, and its way better than whats in there
> just now.

If you think doing it in sysfs is the right way, then move it to sysfs.
The drive spindown is going to end up on the end of the comamnd queue
anyway. Right now we don't have any controller specific power
management, but for some controllers there is power management that we
can do when sleeping (pci D3, turning off buffers etc)

Alan