PATCH: Fixes for ide-disk.c

PATCH Nigel Cunningham 6 participants 20 messages Apr 5, 2003 – Apr 12, 2003

Share & tools

Nigel Cunningham

author

2003-04-05 07:22 UTC #0 lore raw

Hi.

In working on swsusp, I've found the following patches were needed. The first fragment is trivial, as you'll see.
The second and third handle the fact that the suspend & resume functions can be called multiple times. Pavel asked me to pass it on immediately. The fourth handles the fact that writeback caches seem to be implemented in two ways. People using swsusp under 2.4 found that everything worked fine if they rebooted after writing the image, but powering down at the end of writing the image caused corruption. I got the additional check from the source for hdparm, which only does the new check to determine if a drive has a writeback cache.

Regards,

Nigel

drivers/ide/ide-disk.c +6 -5

@@ -1515,7 +1515,7 @@
 {
 	ide_drive_t *drive = dev->driver_data;
 
-	printk("Suspending device %p\n", dev->driver_data);
+	printk(KERN_INFO "Suspending device %p\n", dev->driver_data);
 
 	/* I hope that every freeze operation from the upper levels have
 	 * already been done...

@@ -1527,7 +1527,7 @@
 	/* set the drive to standby */
 	printk(KERN_INFO "suspending: %s ", drive->name);
 	do_idedisk_standby(drive);
-	drive->blocked = 1;
+	drive->blocked++;
 
 	BUG_ON (HWGROUP(drive)->handler);
 	return 0;

@@ -1539,8 +1539,8 @@
 
 	if (level != RESUME_RESTORE_STATE)
 		return 0;
-	BUG_ON(!drive->blocked);
-	drive->blocked = 0;
+	if (drive->blocked)
+		drive->blocked--;
 	return 0;
 }

@@ -1804,7 +1804,8 @@
 	if ((!drive->head || drive->head > 16) && !drive->select.b.lba) {
 		printk(KERN_ERR "%s: INVALID GEOMETRY: %d PHYSICAL HEADS?\n",
 			drive->name, drive->head);
-		if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
+		if (((drive->id->cfs_enable_2 & 0x3000) && drive->wcache) ||
+		    ((drive->id->command_set_1 & 0x20) && drive->id->cfs_enable_1 & 0x20))
 			if (do_idedisk_flushcache(drive))
 				printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
 					drive->name);

John Bradford

2003-04-05 09:14 UTC #1 lore raw

Quoted 5 lines

People using swsusp under 2.4 found that everything worked fine if they rebooted after writing the image, but powering down at the end of writing the image caused corruption. I got the additional check from the source for hdparm, which only does the new check to determine if a drive has a writeback cache.

Did we ever establish what the best way to ensure that the write cache is flushed, is? An explicit cache flush and spin down are both necessary, but I had problems with drives spinning back up when we did the spindown first.

John.

Andre Hedrick

2003-04-05 11:04 UTC #2 lore raw

Are you issuing "standby" or "suspend" ?

The shutdown issues a "standby" and not a "suspend", this is why you get the spinup on a flush-cache.

Quoted 10 lines · John Bradford

Quoted 5 lines
People using swsusp under 2.4 found that everything worked fine if they rebooted after writing the image, but powering down at the end of writing the image caused corruption. I got the additional check from the source for hdparm, which only does the new check to determine if a drive has a writeback cache.
Did we ever establish what the best way to ensure that the write cache is flushed, is? An explicit cache flush and spin down are both necessary, but I had problems with drives spinning back up when we did the spindown first.

Quoted 7 lines

John.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andre Hedrick
LAD Storage Consulting Group

John Bradford

2003-04-05 12:56 UTC #3 lore raw

Quoted 4 lines

Are you issuing "standby" or "suspend" ?
The shutdown issues a "standby" and not a "suspend", this is why you get the spinup on a flush-cache.

I noticed this happening a few months ago when I was testing 2.4.20 on a couple of similar 486 laptops. About 25% of the time, the disk would spin down, then up again, after the flushing IDE devices message.

I think that the argument against doing the flush first, then the standby, was that some drives would abort, or wait on the flush, but I never saw a conclusive answer to it.

Am I the only person who observed the spin down, and immediate spin up behavior?

John.

Alan Cox

2003-04-05 17:33 UTC #4 lore raw

In reply to Nigel Cunningham

drivers/ide/ide-disk.c line 1531

Quoted 7 lines · Nigel Cunningham

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

@@ -1527,7 +1527,7 @@
 	/* set the drive to standby */
 	printk(KERN_INFO "suspending: %s ", drive->name);
 	do_idedisk_standby(drive);
-	drive->blocked = 1;
+	drive->blocked++;

Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.

Alan

Nigel Cunningham

author

2003-04-05 19:16 UTC #5 lore raw

Hi.

Quoted 2 lines · Alan Cox

Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.

Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?

Regards,

Nigel

Signature · 6 lines

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Andre Hedrick

2003-04-05 20:51 UTC #6 lore raw

Why?

Drive->blocked N times is still blocked.

I refuse to play tracking games to figure out whose Drive->blocked is whose!

Cheers,

Quoted 6 lines · Nigel Cunningham

Hi.
Quoted 2 lines · Alan Cox
Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.

Quoted 22 lines

Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?

Regards,

Nigel

Signature · 12 lines

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Andre Hedrick
LAD Storage Consulting Group

Alan Cox

2003-04-06 15:50 UTC #7 lore raw

In reply to Nigel Cunningham

Quoted 9 lines · Nigel Cunningham

Hi.
Quoted 2 lines · Alan Cox
Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.
Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?

Blocked is a binary power management described state, its not a lock. What are you actually trying to do ?

Nigel Cunningham

author

2003-04-06 20:51 UTC #8 lore raw

Ok. I just figured that if there are two (say) calls to suspend a driver, there should be two calls to resume it before it actually is resumed. Does the spec say anything about how multiple suspends & resumes should be handled?

Regards,

Nigel

Quoted 2 lines

Blocked is a binary power management described state, its not a lock. What are you actually trying to do ?

Signature · 6 lines

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Alan Cox

2003-04-06 21:49 UTC #9 lore raw

Quoted 4 lines · Nigel Cunningham

Ok. I just figured that if there are two (say) calls to suspend a driver, there should be two calls to resume it before it actually is resumed. Does the spec say anything about how multiple suspends & resumes should be handled?

The IDE layer assumes the power management layer doesn't generate bogus repeated suspend requests. If it does then lots of drivers are broken and the core code needs fixing.

If its because there are two different notifiers trying to park the IDE disk that might make more sense.

However getting this wrong loses data so I will accept no changes to this code until someone precisely explains what is going on so we can be sure the fix itself is safe.

Nigel Cunningham

author

2003-04-06 21:03 UTC #10 lore raw

In reply to Alan Cox

Hi again.

I've just reread your message. Isn't it used as a lock at the moment? IIRC, you get a BUG if you try to use the driver while its blocked. Perhaps that's where I'm getting confused.

Regards,

Nigel

Quoted 2 lines · Alan Cox

Blocked is a binary power management described state, its not a lock. What are you actually trying to do ?

Alan Cox

2003-04-06 21:47 UTC #11 lore raw

Quoted 5 lines · Nigel Cunningham

Hi again.
I've just reread your message. Isn't it used as a lock at the moment? IIRC, you get a BUG if you try to use the driver while its blocked. Perhaps that's where I'm getting confused.

There are multiple "blocked" fields to deal with. drive->blocked indicates we can't feed it commands because it is blocked due to power management.

idedisk_suspend sets the drive->blocked field so that we panic if anything is sent to the disk after we turned it off (since it would be a corrupter).

idedisk_resume is called on the resume path and marks the drive as safe to use.

So if you hit that bug, you did I/O after shutting the drive down, which is not allowed.

Nigel Cunningham

author

2003-04-06 21:50 UTC #12 lore raw

Quoted 8 lines · Alan Cox

There are multiple "blocked" fields to deal with. drive->blocked indicates we can't feed it commands because it is blocked due to power management.
idedisk_suspend sets the drive->blocked field so that we panic if anything is sent to the disk after we turned it off (since it would be a corrupter).

Quoted 5 lines

idedisk_resume is called on the resume path and marks the drive as safe to use.
So if you hit that bug, you did I/O after shutting the drive down, which is not allowed.

Okay. So the right behaviour is to ignore multiple idedisk_suspend calls and ignore multiple idedisk_resume calls?

Regards,

Nigel

Signature · 6 lines

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Pavel Machek

2003-04-10 22:09 UTC #13 lore raw

In reply to Alan Cox

Hi!

Quoted 6 lines

Quoted 3 lines
Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?
Blocked is a binary power management described state, its not a lock. What are you actually trying to do ?

He's trying to fix swsusp. Just now it likes
to BUG() for some people.

Signature · 2 lines

				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

Alan Cox

2003-04-10 22:42 UTC #14 lore raw

Quoted 11 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 3 lines
Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?
Blocked is a binary power management described state, its not a lock. What are you actually trying to do ?
He's trying to fix swsusp. Just now it likes
to BUG() for some people.

I meant at a slightly lower and more detailed level

Nigel Cunningham

author

2003-04-10 23:28 UTC #15 lore raw

Hi.

As far as ide code goes, I'm trying to:

1) Get rid of messages that don't need to appear during suspend (so the 'nice display' looks nice (hence printk changes)
2) Make sure the data actually does make it to disk before we poweroff (hence write back cache interest)
3) Make sure ide_suspend/unsuspend do the right thing (however that's defined).

I hadn't looked at the code yet to see why it's called twice - too busy with other things. I did wonder if

        device_suspend(4, SUSPEND_NOTIFY);
        device_suspend(4, SUSPEND_SAVE_STATE);
        device_suspend(4, SUSPEND_DISABLE);

might do it, but it's an untested theory.

Regards,

Nigel

Quoted 14 lines · Alan Cox

Quoted 11 lines · Pavel Machek
Hi!
Quoted 6 lines
Quoted 3 lines
Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?
Blocked is a binary power management described state, its not a lock. What are you actually trying to do ?
He's trying to fix swsusp. Just now it likes
to BUG() for some people.
I meant at a slightly lower and more detailed level

Signature · 6 lines

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Pavel Machek

2003-04-12 20:55 UTC #16 lore raw

Hi!

Quoted 8 lines

I hadn't looked at the code yet to see why it's called twice - too busy with other things. I did wonder if
        device_suspend(4, SUSPEND_NOTIFY);
        device_suspend(4, SUSPEND_SAVE_STATE);
        device_suspend(4, SUSPEND_DISABLE);
might do it, but it's an untested theory.

ide should only care about one of those
levels.

Signature · 2 lines

				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

Pavel Machek

2003-04-10 22:11 UTC #17 lore raw

In reply to Nigel Cunningham

Hi!

Quoted 6 lines

Quoted 2 lines
Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.
Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?

Simplest fix seems to be make sure it only reacts to one "suspending level" (suspend_save_state) and that really should not be nested.
(If that nests, fix caller)

Signature · 2 lines

				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

Andre Hedrick

2003-04-11 03:19 UTC #18 lore raw

Hi!

<rant>
Well you authored the mess^H^H^H^Hwork for the suspend, and it fails the binary logic of on or off. You failed to abort the caller for attempting to set multiple blocks. Fix the kernel because the caller has to be assumed dumb and brain dead ... it is user space.
</rant>

Andre Hedrick
LAD Storage Consulting Group

Quoted 23 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 2 lines
Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.
Okay. We need a different solution then, but the problem remains - the driver can get multiple, nexted calls to block and unblock. Can it become a counting lock?
Simplest fix seems to be make sure it only reacts to one "suspending level" (suspend_save_state) and that really should not be nested.
(If that nests, fix caller)
Signature · 8 lines
				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Alan Cox

2003-04-05 17:33 UTC #19 lore raw

In reply to Nigel Cunningham

drivers/ide/ide-disk.c line 1531

Quoted 7 lines · Nigel Cunningham

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

@@ -1527,7 +1527,7 @@
 	/* set the drive to standby */
 	printk(KERN_INFO "suspending: %s ", drive->name);
 	do_idedisk_standby(drive);
-	drive->blocked = 1;
+	drive->blocked++;

Drive->blocked is a single bit field. Its not a counting lock. Either we are blocked or not.

Alan