Skip to contentSkip to conversation

ide-dma blacklist behaviour broken

PATCH In review Jens Axboe 5 participants 12 messages Oct 5, 2004 – Oct 10, 2004

lore
Share & tools
lore

Jens Axboe

author
#0 lore raw

PATCH 1/1 ide-dma blacklist behaviour broken

Hi,

The blacklist stuff is broken. When set_using_dma() calls into ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This of course succeeds, returning success to the caller of ide_dma_check(). Not so good... It then uncondtionally calls ide_dma_on(), which turns on dma for the drive.

This moves the check to ide_dma_on() so we also catch the buggy ->ide_dma_check() defined by various chipset drivers.

ide/ide-dma.c renamed from ide/ide-dma.c~ +9 -4
@@ -354,11 +355,13 @@
 	struct hd_driveid *id = drive->id;
 	ide_hwif_t *hwif = HWIF(drive);
 
-	if ((id->capability & 1) && hwif->autodma) {
-		/* Consult the list of known "bad" drives */
-		if (__ide_dma_bad_drive(drive))
-			return __ide_dma_off(drive);
+	/* Consult the list of known "bad" drives */
+	if (__ide_dma_bad_drive(drive)) {
+		__ide_dma_off(drive);
+		return 1;
+	}
 
+	if ((id->capability & 1) && hwif->autodma) {
 		/*
 		 * Enable DMA on any drive that has
 		 * UltraDMA (mode 0/1/2/3/4/5/6) enabled

@@ -512,6 +515,9 @@
  
 int __ide_dma_on (ide_drive_t *drive)
 {
+	if (__ide_dma_bad_drive(drive))
+		return 1;
+
 	drive->using_dma = 1;
 	ide_toggle_bounce(drive, 1);
Signature · 1 line
Jens Axboe
#1 lore raw
Quoted 10 lines · Jens Axboe

Hi,

The blacklist stuff is broken. When set_using_dma() calls into ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This of course succeeds, returning success to the caller of ide_dma_check(). Not so good... It then uncondtionally calls ide_dma_on(), which turns on dma for the drive.

This moves the check to ide_dma_on() so we also catch the buggy ->ide_dma_check() defined by various chipset drivers.

Is this a bug introduced in the 2.6.9ish IDE changes or has it been there for a longer time?

author
#2 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · Jens Axboe

Hi,

The blacklist stuff is broken. When set_using_dma() calls into ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This of course succeeds, returning success to the caller of ide_dma_check(). Not so good... It then uncondtionally calls ide_dma_on(), which turns on dma for the drive.

Quoted 5 lines
Quoted 2 lines

This moves the check to ide_dma_on() so we also catch the buggy ->ide_dma_check() defined by various chipset drivers.

Is this a bug introduced in the 2.6.9ish IDE changes or has it been there for a longer time?

I didn't check, someone just reported today. But looking at eg 2.6.5, it seems to have the same bug. So it's likely very old.

Signature · 1 line
Jens Axboe
#9 lore raw
In reply to Christoph Hellwig
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · Jens Axboe

Hi,

The blacklist stuff is broken. When set_using_dma() calls into ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This of course succeeds, returning success to the caller of ide_dma_check(). Not so good... It then uncondtionally calls ide_dma_on(), which turns on dma for the drive.

Quoted 5 lines
Quoted 2 lines

This moves the check to ide_dma_on() so we also catch the buggy ->ide_dma_check() defined by various chipset drivers.

Is this a bug introduced in the 2.6.9ish IDE changes or has it been there for a longer time?

Looks like it is also in 2.4.27.

Juri

#3 lore raw
In reply to Jens Axboe
Quoted 2 lines · Jens Axboe

I didn't check, someone just reported today. But looking at eg 2.6.5, it seems to have the same bug. So it's likely very old.

We should actually probably nuke most of the IDE blacklist, much of the CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.

author
#4 lore raw
Quoted 6 lines · Alan Cox
Quoted 2 lines · Jens Axboe

I didn't check, someone just reported today. But looking at eg 2.6.5, it seems to have the same bug. So it's likely very old.

We should actually probably nuke most of the IDE blacklist, much of the CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.

Hmm? When have we ever done that?

Signature · 1 line
Jens Axboe
#5 lore raw
Quoted 4 lines · Jens Axboe
Quoted 2 lines

We should actually probably nuke most of the IDE blacklist, much of the CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.

Hmm? When have we ever done that?

2.0, 2.2, 2.4 to about 2.4.18 or so (Khalid Aziz eventually pinned it down and fixed it).

author
#6 lore raw
Quoted 8 lines · Alan Cox
Quoted 4 lines · Jens Axboe
Quoted 2 lines

We should actually probably nuke most of the IDE blacklist, much of the CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.

Hmm? When have we ever done that?

2.0, 2.2, 2.4 to about 2.4.18 or so (Khalid Aziz eventually pinned it down and fixed it).

Ah, I think you are misreading it. It wasn't the DMA'ing of the atapi cdb, that was always pio'ed to the drive. But DMA for the command itself was turned on before the cdb had been transferred.

Signature · 1 line
Jens Axboe
#7 lore raw
Quoted 3 lines · Jens Axboe

Ah, I think you are misreading it. It wasn't the DMA'ing of the atapi cdb, that was always pio'ed to the drive. But DMA for the command itself was turned on before the cdb had been transferred.

Yep. I may have the detail wrong, its a long time ago. That fixed several CD's that were on the blacklist and most of the others may well never have been tested.

author
#8 lore raw
Quoted 8 lines · Alan Cox
Quoted 3 lines · Jens Axboe

Ah, I think you are misreading it. It wasn't the DMA'ing of the atapi cdb, that was always pio'ed to the drive. But DMA for the command itself was turned on before the cdb had been transferred.

Yep. I may have the detail wrong, its a long time ago. That fixed several CD's that were on the blacklist and most of the others may well never have been tested.

Yeah, it was a very nasty bug. So do you suggest we try and scrap the atapi drives from the blacklist?

Signature · 1 line
Jens Axboe
#10 lore raw
In reply to Jens Axboe

[ linux-ide MIA ]

Quoted 7 lines · Jens Axboe

Hi,

The blacklist stuff is broken. When set_using_dma() calls into ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This of course succeeds, returning success to the caller of ide_dma_check(). Not so good... It then uncondtionally calls ide_dma_on(), which turns on dma for the drive.

- s/ide_dma_check/->ide_dma_check/
- s/ide_dma_off/__ide_dma_off/

Quoted 2 lines

This moves the check to ide_dma_on() so we also catch the buggy ->ide_dma_check() defined by various chipset drivers.

Yep, good catch.

Quoted 10 lines
ide/ide-dma.c renamed from ide/ide-dma.c~ +0 -4
@@ -354,11 +355,13 @@
 	struct hd_driveid *id = drive->id;
 	ide_hwif_t *hwif = HWIF(drive);
 
-	if ((id->capability & 1) && hwif->autodma) {
-		/* Consult the list of known "bad" drives */
-		if (__ide_dma_bad_drive(drive))
-			return __ide_dma_off(drive);

ide/ide-dma.c line 367

Quoted 10 lines
ide/ide-dma.c renamed from ide/ide-dma.c~ +6 -0
+	/* Consult the list of known "bad" drives */
+	if (__ide_dma_bad_drive(drive)) {
+		__ide_dma_off(drive);
+		return 1;
+	}
 
+	if ((id->capability & 1) && hwif->autodma) {
 		/*
 		 * Enable DMA on any drive that has
 		 * UltraDMA (mode 0/1/2/3/4/5/6) enabled

Is __ide_dma_bad_drive() check still needed?
Doesn't __ide_dma_on() fix handle this now?

Quoted 10 lines
ide/ide-dma.c renamed from ide/ide-dma.c~ +3 -0
@@ -512,6 +515,9 @@
 
 int __ide_dma_on (ide_drive_t *drive)
 {
+	if (__ide_dma_bad_drive(drive))
+		return 1;
+
 	drive->using_dma = 1;
 	ide_toggle_bounce(drive, 1);
author
#11 lore raw
Quoted 16 lines · Bartlomiej Zolnierkiewicz
Quoted 16 lines
ide/ide-dma.c renamed from ide/ide-dma.c~ +5 -4
@@ -354,11 +355,13 @@
 	struct hd_driveid *id = drive->id;
 	ide_hwif_t *hwif = HWIF(drive);
 
-	if ((id->capability & 1) && hwif->autodma) {
-		/* Consult the list of known "bad" drives */
-		if (__ide_dma_bad_drive(drive))
-			return __ide_dma_off(drive);
+	/* Consult the list of known "bad" drives */
+	if (__ide_dma_bad_drive(drive)) {
+		__ide_dma_off(drive);
+		return 1;
+	}
Quoted 7 lines

ide/ide-dma.c line 367

Quoted 4 lines
ide/ide-dma.c renamed from ide/ide-dma.c~ +1 -0
+	if ((id->capability & 1) && hwif->autodma) {
 		/*
 		 * Enable DMA on any drive that has
 		 * UltraDMA (mode 0/1/2/3/4/5/6) enabled

Is __ide_dma_bad_drive() check still needed?
Doesn't __ide_dma_on() fix handle this now?

Yeah, we can solely rely on that if we so wish.

Signature · 1 line
Jens Axboe