Kill obsolete and unused suspend/resume code from IDE

PATCH Pavel Machek 3 participants 8 messages Nov 12, 2002 – Nov 13, 2002 1 message not in the archive

lore

Share & tools

lore

Pavel Machek

author

2002-11-12 17:46 UTC #0 lore raw

Hi!

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

drivers/ide/ide-disk.c +1 ~~-28~~

@@ -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,

drivers/ide/ide.c +2 ~~-25~~

@@ -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;
 }

include/linux/ide.h +0 -3

@@ -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);

Signature · 2 lines

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

Alan Cox

2002-11-12 17:55 UTC #1 lore raw

Quoted 2 lines

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

drivers/ide/ide-disk.c line 1699

Quoted 4 lines

drivers/ide/ide-disk.c +1 -0

+	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.

Pavel Machek

author

2002-11-13 13:35 UTC #2 lore raw

Hi!

Quoted 4 lines

Quoted 2 lines
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.

Quoted 6 lines

drivers/ide/ide-disk.c line 1699
Quoted 4 lines
drivers/ide/ide-disk.c +1 -0
+	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).

Quoted 2 lines

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

Signature · 2 lines

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

Alan Cox

2002-11-13 14:00 UTC #3 lore raw

Quoted 5 lines · Pavel Machek

Quoted 1 line
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

Quoted 9 lines

As of obsolete... It is doing power managment outside of sysfs framework... Which is not how it should be done.
Quoted 6 lines
drivers/ide/ide-disk.c line 1699
Quoted 4 lines
drivers/ide/ide-disk.c +1 -0
+	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

Quoted 2 lines

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

Pavel Machek

author

2002-11-13 14:10 UTC #4 lore raw

Hi!

Quoted 17 lines

Quoted 9 lines
As of obsolete... It is doing power managment outside of sysfs framework... Which is not how it should be done.
Quoted 6 lines
drivers/ide/ide-disk.c line 1699
Quoted 4 lines
drivers/ide/ide-disk.c +1 -0
+	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
Quoted 2 lines
Linus actually asked me for the patch, and discussion about it was public, then he silently droped it. This was "only" retransmit.

Quoted 7 lines

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

Signature · 2 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Machek

author

2002-11-13 13:39 UTC #6 lore raw

Hi!

Quoted 5 lines

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.

Review

Quoted 12 lines

[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
]
(quoted diff) +0 -0
 	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

Signature · 2 lines

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

Pavel Machek

author

2002-11-13 14:07 UTC #7 lore raw

In reply to a message not in the archive

Hi!

Quoted 5 lines

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

Signature · 2 lines

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

Alan Cox

2002-11-13 14:18 UTC #8 lore raw

Quoted 7 lines · Pavel Machek

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?

Quoted 5 lines

...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