Skip to contentSkip to conversation

ide: Fix ATAPI DMA lost irq problem with CDB intr devices

PATCH In review Albert Lee 4 participants 23 messages Jan 18, 2007 – Feb 2, 2007 3 messages not in the archive libata

lore
Share & tools
lore

Albert Lee

author
#0 lore raw

PATCH 1/1 ide: Fix ATAPI DMA lost irq problem with CDB intr devices

Problem: IDE ATAPI DMA lost irq with CDB intr devices on Intel ICHx machines.

This patch clears the INTR and ERROR bits of DMA status before starting BMDMA to fix the problem.

Signed-off-by Albert Lee <[email protected]>

---
ATAPI DMA irq lost when testing Intel ICHx with CDB-intr CD-ROM drives:

Jan 18 18:08:48 p4ht-s kernel: ide-cd: cmd 0x28 timed out
Jan 18 18:08:48 p4ht-s kernel: hdc: DMA interrupt recovery
Jan 18 18:08:48 p4ht-s kernel: hdc: lost interrupt
Jan 18 18:08:48 p4ht-s kernel: hdc: status timeout: status=0xd0 { Busy }
Jan 18 18:08:48 p4ht-s kernel: ide: failed opcode was: unknown
Jan 18 18:08:48 p4ht-s kernel: hdc: DMA disabled
Jan 18 18:08:48 p4ht-s kernel: hdc: drive not ready for command
Jan 18 18:08:48 p4ht-s kernel: hdc: ATAPI reset complete

The problem is reported by Adam Hawks.
It seems the the INTR bit was set to 1 by the CDB-intr before the DMA is started. ICHx doesn't like it and causes the DMA irq lost later.

The patch is against 2.6.20-rc5. Tested ok on ICH4.
====================================================================

drivers/ide/ide-dma.c +6 -0
@@ -591,6 +591,12 @@ void ide_dma_start(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	u8 dma_cmd		= hwif->INB(hwif->dma_command);
+	u8 dma_stat		= hwif->INB(hwif->dma_status);
+
+	/* The intr bit of dma_status might has been set by the CDB interrupt.
+	 * Clear the INTR & ERROR bits here to prevent DMA irq lost later.
+	 */
+	hwif->OUTB(dma_stat|6, hwif->dma_status);
 
 	/* Note that this is done *after* the cmd has
 	 * been issued to the drive, as per the BM-IDE spec.

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

#2 lore raw

Hi,

Quoted 5 lines · Albert Lee

Problem: IDE ATAPI DMA lost irq with CDB intr devices on Intel ICHx machines.

This patch clears the INTR and ERROR bits of DMA status before starting BMDMA to fix the problem.

[PATCH] ide-cd: Handle strange interrupt on the Intel ESB2
http://lkml.org/lkml/2006/12/4/201

The above patch was already merged so it seems that the list of PCI IDs requiring ->atapi_irq_bogon needs to be expanded...

Adam, which ICH version is it?

Quoted 13 lines
Signed-off-by Albert Lee <[email protected]>

---
ATAPI DMA irq lost when testing Intel ICHx with CDB-intr CD-ROM drives:

Jan 18 18:08:48 p4ht-s kernel: ide-cd: cmd 0x28 timed out
Jan 18 18:08:48 p4ht-s kernel: hdc: DMA interrupt recovery
Jan 18 18:08:48 p4ht-s kernel: hdc: lost interrupt
Jan 18 18:08:48 p4ht-s kernel: hdc: status timeout: status=0xd0 { Busy }
Jan 18 18:08:48 p4ht-s kernel: ide: failed opcode was: unknown
Jan 18 18:08:48 p4ht-s kernel: hdc: DMA disabled
Jan 18 18:08:48 p4ht-s kernel: hdc: drive not ready for command
Jan 18 18:08:48 p4ht-s kernel: hdc: ATAPI reset complete
Quoted 5 lines

The problem is reported by Adam Hawks.
It seems the the INTR bit was set to 1 by the CDB-intr before the DMA is started. ICHx doesn't like it and causes the DMA irq lost later.

The patch is against 2.6.20-rc5. Tested ok on ICH4.

Does the problem also happen on ICH4?

Quoted 7 lines

====================================================================

drivers/ide/ide-dma.c 2007-01-18 18:40:18.000000000 +0800 renamed from drivers/ide/ide-dma.c 2006-11-30 05:57:37.000000000 +0800 +0 -0
@@ -591,6 +591,12 @@ void ide_dma_start(ide_drive_t *drive)
 {
       ide_hwif_t *hwif        = HWIF(drive);

drivers/ide/ide-dma.c 2007-01-18 18:40:18.000000000 +0800 void ide_dma_start(ide_drive_t *drive) · line 602

Quoted 10 lines
drivers/ide/ide-dma.c 2007-01-18 18:40:18.000000000 +0800 renamed from drivers/ide/ide-dma.c 2006-11-30 05:57:37.000000000 +0800 +6 -0
       u8 dma_cmd              = hwif->INB(hwif->dma_command);
+       u8 dma_stat             = hwif->INB(hwif->dma_status);
+
+       /* The intr bit of dma_status might has been set by the CDB interrupt.
+        * Clear the INTR & ERROR bits here to prevent DMA irq lost later.
+        */
+       hwif->OUTB(dma_stat|6, hwif->dma_status);
 
       /* Note that this is done *after* the cmd has
        * been issued to the drive, as per the BM-IDE spec.

Thanks,
Bart

author
#3 lore raw

Hi Bart,

Quoted 10 lines
Quoted 5 lines · Albert Lee

Problem: IDE ATAPI DMA lost irq with CDB intr devices on Intel ICHx machines.

This patch clears the INTR and ERROR bits of DMA status before starting BMDMA to fix the problem.

Quoted 5 lines

[PATCH] ide-cd: Handle strange interrupt on the Intel ESB2
http://lkml.org/lkml/2006/12/4/201

The above patch was already merged so it seems that the list of PCI IDs requiring ->atapi_irq_bogon needs to be expanded...

Instead of adding PCI IDs to the blacklist, could we just unconditionally clear the dma_status:INTR in ide_dma_start()?

Reason: Currently the INTR bit is cleared in ide_dma_setup(). Later the PIO data transfer is used to send ATAPI CDB. The BMDMA is started last. The bmdma INTR bit might be changed either by spurious irq (as seen by Alan/Stratus) or by CDB-intr CD-ROM drives (as seen by Adam) etc. during the time frame before bmdma is actually started.
It looks safer if we can clear INTR right before dma is started.

Quoted 7 lines

Adam, which ICH version is it?

<snip>
Does the problem also happen on ICH4?

Adam's box is an ICH (8086:2411) and mine ICH4 (8086:24cb). Yes, this problem also happen on ICH4. I suspect that all Intel ICHx has such behavior (i.e. DMA irq lost if INTR is already 1).

BTW, this problem does not reproduce on libata since libata always clears bmdma INTR in ata_host_intr().

Thanks,

Albert

author
#4 lore raw
Quoted 15 lines · Albert Lee

Hi Bart,

Quoted 13 lines
Quoted 5 lines · Albert Lee

Problem: IDE ATAPI DMA lost irq with CDB intr devices on Intel ICHx machines.

This patch clears the INTR and ERROR bits of DMA status before starting BMDMA to fix the problem.

[PATCH] ide-cd: Handle strange interrupt on the Intel ESB2
http://lkml.org/lkml/2006/12/4/201

Quoted 7 lines
Quoted 2 lines

The above patch was already merged so it seems that the list of PCI IDs requiring ->atapi_irq_bogon needs to be expanded...

Instead of adding PCI IDs to the blacklist, could we just unconditionally clear the dma_status:INTR in ide_dma_start()?

Sorry I did not read through the ICH7 irq storm problem well.

It seems clearing dma_status:INTR in ide_dma_start() is only good for
1. Spurious interrupt problem (http://lkml.org/lkml/2006/12/4/201) and
2. This CDB intr device + ICHx irq lost problem

    (http://www.mail-archive.com/[email protected]/msg02934.html)

But it does not fix
3. ICH7 native mode irq storm problem (http://lkml.org/lkml/2006/11/15/94).

Maybe we should clearing the INTR in the interrupt handler, no matter it is DMA/PIO/ATA/ATAPI, just like what libata does.

Will check ide_intr() to see if we can clear the bmdma_status:INTR there. --
albert

#5 lore raw

Hi,

Quoted 29 lines · Albert Lee
Quoted 15 lines · Albert Lee

Hi Bart,

Quoted 13 lines
Quoted 5 lines · Albert Lee

Problem: IDE ATAPI DMA lost irq with CDB intr devices on Intel ICHx machines.

This patch clears the INTR and ERROR bits of DMA status before starting BMDMA to fix the problem.

[PATCH] ide-cd: Handle strange interrupt on the Intel ESB2
http://lkml.org/lkml/2006/12/4/201

Quoted 7 lines
Quoted 2 lines

The above patch was already merged so it seems that the list of PCI IDs requiring ->atapi_irq_bogon needs to be expanded...

Instead of adding PCI IDs to the blacklist, could we just unconditionally clear the dma_status:INTR in ide_dma_start()?

Sorry I did not read through the ICH7 irq storm problem well.

It seems clearing dma_status:INTR in ide_dma_start() is only good for
1. Spurious interrupt problem (http://lkml.org/lkml/2006/12/4/201) and
2. This CDB intr device + ICHx irq lost problem

Quoted 6 lines
    (http://www.mail-archive.com/[email protected]/msg02934.html)

But it does not fix
3. ICH7 native mode irq storm problem (http://lkml.org/lkml/2006/11/15/94).

Maybe we should clearing the INTR in the interrupt handler, no matter it is DMA/PIO/ATA/ATAPI, just like what libata does.

Seems like a generic and clean way to fix the issue. Keeping IDE code similar to libata is an additional bonus.

Quoted 1 line

Will check ide_intr() to see if we can clear the bmdma_status:INTR there.

Please keep me in the loop.

Thanks,
Bart

author
#6 lore raw

cover ide: clear bmdma status in ide_intr for all commands

patch 1/2: remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.

patch 2/2: do the clearing in ide_intr() and add a new hwif->ide_dma_clear_irq such that lldd could override it.

author
#7 lore raw

PATCH 1/2 ide: remove clearing bmdma status from cdrom_decode_status()

patch 1/2:
Remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.
(http://lkml.org/lkml/2006/12/4/201 and http://lkml.org/lkml/2006/11/15/94)

Signed-off-by Albert Lee <[email protected]>

---

drivers/ide/ide-cd.c +0 -7
diff -Nrup 00_ide_dma/drivers/ide/ide-cd.c 01_remove_from_ide_cd/drivers/ide/ide-cd.c
@@ -687,15 +687,8 @@ static void ide_dump_status_no_sense(ide
 static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
 {
 	struct request *rq = HWGROUP(drive)->rq;
-	ide_hwif_t *hwif = HWIF(drive);
 	int stat, err, sense_key;
 	
-	/* We may have bogus DMA interrupts in PIO state here */
-	if (HWIF(drive)->dma_status && hwif->atapi_irq_bogon) {
-		stat = hwif->INB(hwif->dma_status);
-		/* Should we force the bit as well ? */
-		hwif->OUTB(stat, hwif->dma_status);
-	}
 	/* Check for errors. */
 	stat = HWIF(drive)->INB(IDE_STATUS_REG);
 	if (stat_ret)
drivers/ide/pci/piix.c +0 -4
diff -Nrup 00_ide_dma/drivers/ide/pci/piix.c 01_remove_from_ide_cd/drivers/ide/pci/piix.c
@@ -473,10 +473,6 @@ static void __devinit init_hwif_piix(ide
 		/* This is a painful system best to let it self tune for now */
 		return;
 	}
-	/* ESB2 appears to generate spurious DMA interrupts in PIO mode
-	   when in native mode */
-	if (hwif->pci_dev->device == PCI_DEVICE_ID_INTEL_ESB2_18)
-		hwif->atapi_irq_bogon = 1;
 
 	hwif->autodma = 0;
 	hwif->tuneproc = &piix_tune_drive;
include/linux/ide.h +0 -1
diff -Nrup 00_ide_dma/include/linux/ide.h 01_remove_from_ide_cd/include/linux/ide.h
@@ -796,7 +796,6 @@ typedef struct hwif_s {
 	unsigned	sg_mapped  : 1;	/* sg_table and sg_nents are ready */
 	unsigned	no_io_32bit : 1; /* 1 = can not do 32-bit IO ops */
 	unsigned	err_stops_fifo : 1; /* 1=data FIFO is cleared by an error */
-	unsigned	atapi_irq_bogon : 1; /* Generates spurious DMA interrupts in PIO mode */
 
 	struct device	gendev;
 	struct completion gendev_rel_comp; /* To deal with device release() */
author
#8 lore raw

PATCH 2/2 ide: clear bmdma status in ide_intr()

In reply to Albert Lee

patch 2/2:
Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq such that LLDD could override it.

Signed-off-by Albert Lee <[email protected]>

---
Tested ok on ICH4 and pdc20275. Not sure if this would have bad effect for other adapters.

Patch against 2.6.20-rc5, for your review, thanks.

drivers/ide/ide-dma.c +18 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-dma.c 02_add_to_ide_intr/drivers/ide/ide-dma.c
@@ -650,6 +650,22 @@ static int __ide_dma_test_irq(ide_drive_
 			drive->name, __FUNCTION__);
 	return 0;
 }
+
+/* returns 1 on error, 0 otherwise */
+int __ide_dma_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	u8 dma_stat;
+
+	/* clear the INTR & ERROR bits */
+	if (hwif->dma_status) {
+		dma_stat = hwif->INB(hwif->dma_status);
+		/* Should we force the bit as well ? */
+		hwif->OUTB(dma_stat, hwif->dma_status);
+	}
+
+	return 0;
+}
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
 
 int __ide_dma_bad_drive (ide_drive_t *drive)

@@ -928,6 +944,8 @@ void ide_setup_dma (ide_hwif_t *hwif, un
 		hwif->ide_dma_end = &__ide_dma_end;
 	if (!hwif->ide_dma_test_irq)
 		hwif->ide_dma_test_irq = &__ide_dma_test_irq;
+	if (!hwif->ide_dma_clear_irq)
+		hwif->ide_dma_clear_irq = &__ide_dma_clear_irq;
 	if (!hwif->ide_dma_timeout)
 		hwif->ide_dma_timeout = &__ide_dma_timeout;
 	if (!hwif->ide_dma_lostirq)
drivers/ide/ide-io.c +12 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c
@@ -1644,6 +1644,18 @@ irqreturn_t ide_intr (int irq, void *dev
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
+
+	/* Some controllers might set DMA INTR no matter DMA or PIO;
+	 * bmdma status might need to be cleared even for
+	 * PIO interrupts to prevent spurious irq or irq lost.
+	 */
+	if (hwif->ide_dma_clear_irq && !(hwif->dma))
+		/* ide_dma_end() needs bmdma status for error checking.
+		 * So, skip clearing bmdma status here and leave it
+		 * to ide_dma_end() if this is dma interrupt.
+		 */
+		hwif->ide_dma_clear_irq(drive);
+
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)
drivers/ide/ide.c +1 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c
@@ -503,6 +503,7 @@ static void ide_hwif_restore(ide_hwif_t 
 	hwif->ide_dma_on		= tmp_hwif->ide_dma_on;
 	hwif->ide_dma_off_quietly	= tmp_hwif->ide_dma_off_quietly;
 	hwif->ide_dma_test_irq		= tmp_hwif->ide_dma_test_irq;
+	hwif->ide_dma_clear_irq		= tmp_hwif->ide_dma_clear_irq;
 	hwif->ide_dma_host_on		= tmp_hwif->ide_dma_host_on;
 	hwif->ide_dma_host_off		= tmp_hwif->ide_dma_host_off;
 	hwif->ide_dma_lostirq		= tmp_hwif->ide_dma_lostirq;
include/linux/ide.h +1 -0
diff -Nrup 01_remove_from_ide_cd/include/linux/ide.h 02_add_to_ide_intr/include/linux/ide.h
@@ -727,6 +727,7 @@ typedef struct hwif_s {
 	int (*ide_dma_on)(ide_drive_t *drive);
 	int (*ide_dma_off_quietly)(ide_drive_t *drive);
 	int (*ide_dma_test_irq)(ide_drive_t *drive);
+	int (*ide_dma_clear_irq)(ide_drive_t *drive);
 	int (*ide_dma_host_on)(ide_drive_t *drive);
 	int (*ide_dma_host_off)(ide_drive_t *drive);
 	int (*ide_dma_lostirq)(ide_drive_t *drive);
#9 lore raw

Re: [PATCH 2/2] ide: clear bmdma status in ide_intr()

Hello.

Quoted 13 lines · Albert Lee

patch 2/2:
Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq such that LLDD could override it.

Signed-off-by Albert Lee <[email protected]>

---
Tested ok on ICH4 and pdc20275. Not sure if this would have bad effect for other adapters.

Patch against 2.6.20-rc5, for your review, thanks.

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-dma.c 02_add_to_ide_intr/drivers/ide/ide-dma.c

drivers/ide/ide-dma.c static int __ide_dma_test_irq(ide_drive_ · line 654

Quoted 5 lines
drivers/ide/ide-dma.c +2 -0
@@ -650,6 +650,22 @@ static int __ide_dma_test_irq(ide_drive_
 			drive->name, __FUNCTION__);
 	return 0;
 }
+
+/* returns 1 on error, 0 otherwise */

Why? Do you check the result?

drivers/ide/ide-dma.c static int __ide_dma_test_irq(ide_drive_ · line 661

Quoted 7 lines
drivers/ide/ide-dma.c +7 -0
+int __ide_dma_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	u8 dma_stat;
+
+	/* clear the INTR & ERROR bits */
+	if (hwif->dma_status) {

hwif->dma_status should always be set, else ide-dma.c just won't work. The
check seems superfluous.

drivers/ide/ide-dma.c static int __ide_dma_test_irq(ide_drive_ · line 663

Quoted 2 lines
drivers/ide/ide-dma.c +2 -0
+		dma_stat = hwif->INB(hwif->dma_status);
+		/* Should we force the bit as well ? */

Good idea.

drivers/ide/ide-dma.c static int __ide_dma_test_irq(ide_drive_ · line 664

Quoted 1 line
drivers/ide/ide-dma.c +1 -0
+		hwif->OUTB(dma_stat, hwif->dma_status);

I'm afraid this would break __ide_dma_end() function which tests the error
conditions and returns failure if (dma_stat & 7) != 4...

Quoted 11 lines
drivers/ide/ide-dma.c +4 -0
+	}
+
+	return 0;
+}
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
 
 int __ide_dma_bad_drive (ide_drive_t *drive)

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c

drivers/ide/ide-io.c irqreturn_t ide_intr (int irq, void *dev · line 1651

Quoted 8 lines
drivers/ide/ide-io.c +5 -0
@@ -1644,6 +1644,18 @@ irqreturn_t ide_intr (int irq, void *dev
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
+
+	/* Some controllers might set DMA INTR no matter DMA or PIO;
+	 * bmdma status might need to be cleared even for
+	 * PIO interrupts to prevent spurious irq or irq lost.
+	 */

Either "being lost" or "loss". Sorry for grammar nitpicking. :-)

drivers/ide/ide-io.c irqreturn_t ide_intr (int irq, void *dev · line 1659

Quoted 8 lines
drivers/ide/ide-io.c +7 -0
+	if (hwif->ide_dma_clear_irq && !(hwif->dma))
+		/* ide_dma_end() needs bmdma status for error checking.
+		 * So, skip clearing bmdma status here and leave it
+		 * to ide_dma_end() if this is dma interrupt.
+		 */
+		hwif->ide_dma_clear_irq(drive);
+
 	spin_unlock(&ide_lock);

Ah, you're only calling this when hwif->dma == 0... Well, that's probably
fine. Although, once introduced, this method asks for more use...

drivers/ide/ide-io.c irqreturn_t ide_intr (int irq, void *dev · line 1661

Quoted 1 line
drivers/ide/ide-io.c +0 -0
 	if (drive->unmask)

MBR, Sergei

author
#10 lore raw

PATCH 2/2 ide: clear bmdma status in ide_intr() (revised)

patch 2/2 (revised):
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq such that LLDD can override it.
- Fix drive->waiting_for_dma to work with CDB-intr devices.

Signed-off-by Albert Lee <[email protected]>

---
hwif->dma is not reliable: ide_intr() races with dma_start(). Sometimes we got hwif->dma == 0 in ide_intr() even though it is actually a DMA interrupt. So, drive->waiting_for_dma is used instead.

Also revised per Sergei's comments and let ide_dma_clear_irq return "void". The "if (hwif->dma_status)" check in __ide_dma_clear_irq() is kept as is since I think it would be safer for the old ISA/VESA IDE devices that has no BMDMA registers.

For your review, thanks.

drivers/ide/ide-cd.c +8 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c
@@ -923,6 +923,10 @@ static ide_startstop_t cdrom_start_packe
 		HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
  
 	if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+		/* waiting for CDB interrupt, not DMA yet. */
+		if (info->dma)
+			drive->waiting_for_dma = 0;
+
 		/* packet command */
 		ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
 		return ide_started;

@@ -965,6 +969,10 @@ static ide_startstop_t cdrom_transfer_pa
 		/* Check for errors. */
 		if (cdrom_decode_status(drive, DRQ_STAT, NULL))
 			return ide_stopped;
+
+		/* Ok, next interrupt will be dma interrupt. */
+		if (info->dma)
+			drive->waiting_for_dma = 1;
 	} else {
 		/* Otherwise, we must wait for DRQ to get set. */
 		if (ide_wait_stat(&startstop, drive, DRQ_STAT,
drivers/ide/ide-dma.c +15 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-dma.c 02_add_to_ide_intr/drivers/ide/ide-dma.c
@@ -650,6 +650,19 @@ static int __ide_dma_test_irq(ide_drive_
 			drive->name, __FUNCTION__);
 	return 0;
 }
+
+static void __ide_dma_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	u8 dma_stat;
+
+	/* clear the INTR & ERROR bits */
+	if (hwif->dma_status) {
+		dma_stat = hwif->INB(hwif->dma_status);
+		/* Should we force the bit as well ? */
+		hwif->OUTB(dma_stat, hwif->dma_status);
+	}
+}
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
 
 int __ide_dma_bad_drive (ide_drive_t *drive)

@@ -928,6 +941,8 @@ void ide_setup_dma (ide_hwif_t *hwif, un
 		hwif->ide_dma_end = &__ide_dma_end;
 	if (!hwif->ide_dma_test_irq)
 		hwif->ide_dma_test_irq = &__ide_dma_test_irq;
+	if (!hwif->ide_dma_clear_irq)
+		hwif->ide_dma_clear_irq = &__ide_dma_clear_irq;
 	if (!hwif->ide_dma_timeout)
 		hwif->ide_dma_timeout = &__ide_dma_timeout;
 	if (!hwif->ide_dma_lostirq)
drivers/ide/ide-io.c +12 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c
@@ -1644,6 +1644,18 @@ irqreturn_t ide_intr (int irq, void *dev
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
+
+	/* Some controllers might set DMA INTR no matter DMA or PIO;
+	 * bmdma status might need to be cleared even for
+	 * PIO interrupts to prevent spurious/lost irq.
+	 */
+	if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
+		/* ide_dma_end() needs bmdma status for error checking.
+		 * So, skip clearing bmdma status here and leave it
+		 * to ide_dma_end() if this is dma interrupt.
+		 */
+		hwif->ide_dma_clear_irq(drive);
+
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)
drivers/ide/ide.c +1 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c
@@ -503,6 +503,7 @@ static void ide_hwif_restore(ide_hwif_t 
 	hwif->ide_dma_on		= tmp_hwif->ide_dma_on;
 	hwif->ide_dma_off_quietly	= tmp_hwif->ide_dma_off_quietly;
 	hwif->ide_dma_test_irq		= tmp_hwif->ide_dma_test_irq;
+	hwif->ide_dma_clear_irq		= tmp_hwif->ide_dma_clear_irq;
 	hwif->ide_dma_host_on		= tmp_hwif->ide_dma_host_on;
 	hwif->ide_dma_host_off		= tmp_hwif->ide_dma_host_off;
 	hwif->ide_dma_lostirq		= tmp_hwif->ide_dma_lostirq;
include/linux/ide.h +1 -0
diff -Nrup 01_remove_from_ide_cd/include/linux/ide.h 02_add_to_ide_intr/include/linux/ide.h
@@ -727,6 +727,7 @@ typedef struct hwif_s {
 	int (*ide_dma_on)(ide_drive_t *drive);
 	int (*ide_dma_off_quietly)(ide_drive_t *drive);
 	int (*ide_dma_test_irq)(ide_drive_t *drive);
+	void (*ide_dma_clear_irq)(ide_drive_t *drive);
 	int (*ide_dma_host_on)(ide_drive_t *drive);
 	int (*ide_dma_host_off)(ide_drive_t *drive);
 	int (*ide_dma_lostirq)(ide_drive_t *drive);
#11 lore raw

Re: [PATCH 2/2] ide: clear bmdma status in ide_intr() (revised)

Quoted 4 lines · Albert Lee

patch 2/2 (revised):
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq such that LLDD can override it.
- Fix drive->waiting_for_dma to work with CDB-intr devices.

Quoted 8 lines
Signed-off-by Albert Lee <[email protected]>

---
hwif->dma is not reliable: ide_intr() races with dma_start(). Sometimes we got hwif->dma == 0 in ide_intr() even though it is actually a DMA interrupt. So, drive->waiting_for_dma is used instead.

Also revised per Sergei's comments and let ide_dma_clear_irq return "void". The "if (hwif->dma_status)" check in __ide_dma_clear_irq() is kept as is since I think it would be safer for the old ISA/VESA IDE devices that has no BMDMA registers.

ISA/VESA drivers shouldn't have this method defined at all since they're
PIO only. But well, it won't hurt...

Quoted 17 lines

For your review, thanks.

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c

drivers/ide/ide-cd.c +4 -0
@@ -923,6 +923,10 @@ static ide_startstop_t cdrom_start_packe
 		HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
 
 	if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+		/* waiting for CDB interrupt, not DMA yet. */
+		if (info->dma)
+			drive->waiting_for_dma = 0;
+
 		/* packet command */
 		ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
 		return ide_started;

drivers/ide/ide-cd.c static ide_startstop_t cdrom_transfer_pa · line 978

Quoted 10 lines
drivers/ide/ide-cd.c +4 -0
@@ -965,6 +969,10 @@ static ide_startstop_t cdrom_transfer_pa
 		/* Check for errors. */
 		if (cdrom_decode_status(drive, DRQ_STAT, NULL))
 			return ide_stopped;
+
+		/* Ok, next interrupt will be dma interrupt. */
+		if (info->dma)
+			drive->waiting_for_dma = 1;
 	} else {
 		/* Otherwise, we must wait for DRQ to get set. */
 		if (ide_wait_stat(&startstop, drive, DRQ_STAT,

Erm... shouldn't we set drive->waiting_for_dma in hwif->dma_start() then?
Why it's set in hwif->dma_setup() at all I wonder?

MBR, Sergei

#12 lore raw

Re: [PATCH 2/2] ide: clear bmdma status in ide_intr() (revised)

Hello.

Quoted 26 lines · Sergei Shtylyov
Quoted 14 lines

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c

drivers/ide/ide-cd.c 2007-01-25 16:52:20.000000000 +0800 renamed from drivers/ide/ide-cd.c 2007-01-24 11:00:03.000000000 +0800 +4 -0
@@ -923,6 +923,10 @@ static ide_startstop_t cdrom_start_packe
         HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
       if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+        /* waiting for CDB interrupt, not DMA yet. */
+        if (info->dma)
+            drive->waiting_for_dma = 0;
+
         /* packet command */
         ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
         return ide_started;

drivers/ide/ide-cd.c 2007-01-25 16:52:20.000000000 +0800 static ide_startstop_t cdrom_transfer_pa · line 978

Quoted 10 lines
drivers/ide/ide-cd.c 2007-01-25 16:52:20.000000000 +0800 renamed from drivers/ide/ide-cd.c 2007-01-24 11:00:03.000000000 +0800 +4 -0
@@ -965,6 +969,10 @@ static ide_startstop_t cdrom_transfer_pa
         /* Check for errors. */
         if (cdrom_decode_status(drive, DRQ_STAT, NULL))
             return ide_stopped;
+
+        /* Ok, next interrupt will be dma interrupt. */
+        if (info->dma)
+            drive->waiting_for_dma = 1;
     } else {
         /* Otherwise, we must wait for DRQ to get set. */
         if (ide_wait_stat(&startstop, drive, DRQ_STAT,

   Erm... shouldn't we set drive->waiting_for_dma in hwif->dma_start()
Quoted 1 line

then? Why it's set in hwif->dma_setup() at all I wonder?

Well, it'll become raicy as well then... Isn't it ugly. :-/

MBR, Sergei

author
#13 lore raw

cover ide: clear bmdma status in ide_intr for Intel controllers (revised #3)

patch 1/2: remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.

patch 2/2: clear bmdma status in ide_intr() and add a new hwif->ide_dma_clear_irq for piix.c to use.

author
#14 lore raw

PATCH 1/2 ide: remove clearing bmdma status from cdrom_decode_status() (rev #3)

patch 1/2:
Remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.
(http://lkml.org/lkml/2006/12/4/201 and http://lkml.org/lkml/2006/11/15/94)

Signed-off-by Albert Lee <[email protected]>

---

drivers/ide/ide-cd.c +0 -7
diff -Nrup 00_ide_dma/drivers/ide/ide-cd.c 01_remove_from_ide_cd/drivers/ide/ide-cd.c
@@ -687,15 +687,8 @@ static void ide_dump_status_no_sense(ide
 static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
 {
 	struct request *rq = HWGROUP(drive)->rq;
-	ide_hwif_t *hwif = HWIF(drive);
 	int stat, err, sense_key;
 	
-	/* We may have bogus DMA interrupts in PIO state here */
-	if (HWIF(drive)->dma_status && hwif->atapi_irq_bogon) {
-		stat = hwif->INB(hwif->dma_status);
-		/* Should we force the bit as well ? */
-		hwif->OUTB(stat, hwif->dma_status);
-	}
 	/* Check for errors. */
 	stat = HWIF(drive)->INB(IDE_STATUS_REG);
 	if (stat_ret)
drivers/ide/pci/piix.c +0 -4
diff -Nrup 00_ide_dma/drivers/ide/pci/piix.c 01_remove_from_ide_cd/drivers/ide/pci/piix.c
@@ -473,10 +473,6 @@ static void __devinit init_hwif_piix(ide
 		/* This is a painful system best to let it self tune for now */
 		return;
 	}
-	/* ESB2 appears to generate spurious DMA interrupts in PIO mode
-	   when in native mode */
-	if (hwif->pci_dev->device == PCI_DEVICE_ID_INTEL_ESB2_18)
-		hwif->atapi_irq_bogon = 1;
 
 	hwif->autodma = 0;
 	hwif->tuneproc = &piix_tune_drive;
include/linux/ide.h +0 -1
diff -Nrup 00_ide_dma/include/linux/ide.h 01_remove_from_ide_cd/include/linux/ide.h
@@ -796,7 +796,6 @@ typedef struct hwif_s {
 	unsigned	sg_mapped  : 1;	/* sg_table and sg_nents are ready */
 	unsigned	no_io_32bit : 1; /* 1 = can not do 32-bit IO ops */
 	unsigned	err_stops_fifo : 1; /* 1=data FIFO is cleared by an error */
-	unsigned	atapi_irq_bogon : 1; /* Generates spurious DMA interrupts in PIO mode */
 
 	struct device	gendev;
 	struct completion gendev_rel_comp; /* To deal with device release() */
author
#15 lore raw

PATCH 2/2 ide: clear bmdma status in ide_intr() for Intel controllers (revised #3)

In reply to Albert Lee

patch 2/2 (revised):
- Fix drive->waiting_for_dma to work with CDB-intr devices.
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq for Intel controllers.

Signed-off-by Albert Lee <[email protected]>

---

Calling hwif->ide_dma_clear_irq for all controllers looks risky. Revised to do it only for the Intel controllers.
(If any other adapters need it, we may add it later.)

Patch against 2.6.20-rc6. Tested ok on my ICH4 and pdc20275 adapters. Please review/apply, thanks.

drivers/ide/ide-cd.c +8 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c
@@ -923,6 +923,10 @@ static ide_startstop_t cdrom_start_packe
 		HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
  
 	if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+		/* waiting for CDB interrupt, not DMA yet. */
+		if (info->dma)
+			drive->waiting_for_dma = 0;
+
 		/* packet command */
 		ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
 		return ide_started;

@@ -965,6 +969,10 @@ static ide_startstop_t cdrom_transfer_pa
 		/* Check for errors. */
 		if (cdrom_decode_status(drive, DRQ_STAT, NULL))
 			return ide_stopped;
+
+		/* Ok, next interrupt will be DMA interrupt. */
+		if (info->dma)
+			drive->waiting_for_dma = 1;
 	} else {
 		/* Otherwise, we must wait for DRQ to get set. */
 		if (ide_wait_stat(&startstop, drive, DRQ_STAT,
drivers/ide/ide-dma.c +14 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-dma.c 02_add_to_ide_intr/drivers/ide/ide-dma.c
@@ -650,6 +650,20 @@ static int __ide_dma_test_irq(ide_drive_
 			drive->name, __FUNCTION__);
 	return 0;
 }
+
+void __ide_dma_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	u8 dma_stat;
+
+	/* clear the INTR & ERROR bits */
+	dma_stat = hwif->INB(hwif->dma_status);
+	/* Should we force the bit as well ? */
+	hwif->OUTB(dma_stat, hwif->dma_status);
+}
+
+EXPORT_SYMBOL_GPL(__ide_dma_clear_irq);
+
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
 
 int __ide_dma_bad_drive (ide_drive_t *drive)
drivers/ide/ide-io.c +12 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c
@@ -1644,6 +1644,18 @@ irqreturn_t ide_intr (int irq, void *dev
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
+
+	/* Some controllers might set DMA INTR no matter DMA or PIO;
+	 * bmdma status might need to be cleared even for
+	 * PIO interrupts to prevent spurious/lost irq.
+	 */
+	if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
+		/* ide_dma_end() needs bmdma status for error checking.
+		 * So, skip clearing bmdma status here and leave it
+		 * to ide_dma_end() if this is dma interrupt.
+		 */
+		hwif->ide_dma_clear_irq(drive);
+
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)
drivers/ide/ide.c +1 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c
@@ -503,6 +503,7 @@ static void ide_hwif_restore(ide_hwif_t 
 	hwif->ide_dma_on		= tmp_hwif->ide_dma_on;
 	hwif->ide_dma_off_quietly	= tmp_hwif->ide_dma_off_quietly;
 	hwif->ide_dma_test_irq		= tmp_hwif->ide_dma_test_irq;
+	hwif->ide_dma_clear_irq		= tmp_hwif->ide_dma_clear_irq;
 	hwif->ide_dma_host_on		= tmp_hwif->ide_dma_host_on;
 	hwif->ide_dma_host_off		= tmp_hwif->ide_dma_host_off;
 	hwif->ide_dma_lostirq		= tmp_hwif->ide_dma_lostirq;
drivers/ide/pci/piix.c +1 -0
diff -Nrup 01_remove_from_ide_cd/drivers/ide/pci/piix.c 02_add_to_ide_intr/drivers/ide/pci/piix.c
@@ -483,6 +483,7 @@ static void __devinit init_hwif_piix(ide
 	if (!hwif->dma_base)
 		return;
 
+	hwif->ide_dma_clear_irq = &__ide_dma_clear_irq;
 	hwif->atapi_dma = 1;
 	hwif->ultra_mask = 0x3f;
 	hwif->mwdma_mask = 0x06;
include/linux/ide.h +2 -0
diff -Nrup 01_remove_from_ide_cd/include/linux/ide.h 02_add_to_ide_intr/include/linux/ide.h
@@ -727,6 +727,7 @@ typedef struct hwif_s {
 	int (*ide_dma_on)(ide_drive_t *drive);
 	int (*ide_dma_off_quietly)(ide_drive_t *drive);
 	int (*ide_dma_test_irq)(ide_drive_t *drive);
+	void (*ide_dma_clear_irq)(ide_drive_t *drive);
 	int (*ide_dma_host_on)(ide_drive_t *drive);
 	int (*ide_dma_host_off)(ide_drive_t *drive);
 	int (*ide_dma_lostirq)(ide_drive_t *drive);

@@ -1283,6 +1284,7 @@ extern int __ide_dma_check(ide_drive_t *
 extern int ide_dma_setup(ide_drive_t *);
 extern void ide_dma_start(ide_drive_t *);
 extern int __ide_dma_end(ide_drive_t *);
+extern void __ide_dma_clear_irq(ide_drive_t *);
 extern int __ide_dma_lostirq(ide_drive_t *);
 extern int __ide_dma_timeout(ide_drive_t *);
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
#16 lore raw

Re: [PATCH 2/2] ide: clear bmdma status in ide_intr() for Intel controllers (revised #3)

Hello.

Quoted 8 lines · Albert Lee

patch 2/2 (revised):
- Fix drive->waiting_for_dma to work with CDB-intr devices.
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq for Intel controllers.

Signed-off-by Albert Lee <[email protected]>

---

Calling hwif->ide_dma_clear_irq for all controllers looks risky.

I'm not sure why it's so risky...

Quoted 92 lines

Revised to do it only for the Intel controllers. (If any other adapters need it, we may add it later.)

Patch against 2.6.20-rc6. Tested ok on my ICH4 and pdc20275 adapters. Please review/apply, thanks.

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c

drivers/ide/ide-cd.c +8 -0
@@ -923,6 +923,10 @@ static ide_startstop_t cdrom_start_packe
 		HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
 
 	if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+		/* waiting for CDB interrupt, not DMA yet. */
+		if (info->dma)
+			drive->waiting_for_dma = 0;
+
 		/* packet command */
 		ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
 		return ide_started;

@@ -965,6 +969,10 @@ static ide_startstop_t cdrom_transfer_pa
 		/* Check for errors. */
 		if (cdrom_decode_status(drive, DRQ_STAT, NULL))
 			return ide_stopped;
+
+		/* Ok, next interrupt will be DMA interrupt. */
+		if (info->dma)
+			drive->waiting_for_dma = 1;
 	} else {
 		/* Otherwise, we must wait for DRQ to get set. */
 		if (ide_wait_stat(&startstop, drive, DRQ_STAT,

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-dma.c 02_add_to_ide_intr/drivers/ide/ide-dma.c

drivers/ide/ide-dma.c +14 -0
@@ -650,6 +650,20 @@ static int __ide_dma_test_irq(ide_drive_
 			drive->name, __FUNCTION__);
 	return 0;
 }
+
+void __ide_dma_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	u8 dma_stat;
+
+	/* clear the INTR & ERROR bits */
+	dma_stat = hwif->INB(hwif->dma_status);
+	/* Should we force the bit as well ? */
+	hwif->OUTB(dma_stat, hwif->dma_status);
+}
+
+EXPORT_SYMBOL_GPL(__ide_dma_clear_irq);
+
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
 
 int __ide_dma_bad_drive (ide_drive_t *drive)

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c

drivers/ide/ide-io.c +12 -0
@@ -1644,6 +1644,18 @@ irqreturn_t ide_intr (int irq, void *dev
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
+
+	/* Some controllers might set DMA INTR no matter DMA or PIO;
+	 * bmdma status might need to be cleared even for
+	 * PIO interrupts to prevent spurious/lost irq.
+	 */
+	if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
+		/* ide_dma_end() needs bmdma status for error checking.
+		 * So, skip clearing bmdma status here and leave it
+		 * to ide_dma_end() if this is dma interrupt.
+		 */
+		hwif->ide_dma_clear_irq(drive);
+
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c

drivers/ide/ide.c +1 -0
@@ -503,6 +503,7 @@ static void ide_hwif_restore(ide_hwif_t 
 	hwif->ide_dma_on		= tmp_hwif->ide_dma_on;
 	hwif->ide_dma_off_quietly	= tmp_hwif->ide_dma_off_quietly;
 	hwif->ide_dma_test_irq		= tmp_hwif->ide_dma_test_irq;
+	hwif->ide_dma_clear_irq		= tmp_hwif->ide_dma_clear_irq;
 	hwif->ide_dma_host_on		= tmp_hwif->ide_dma_host_on;
 	hwif->ide_dma_host_off		= tmp_hwif->ide_dma_host_off;
 	hwif->ide_dma_lostirq		= tmp_hwif->ide_dma_lostirq;

diff -Nrup 01_remove_from_ide_cd/drivers/ide/pci/piix.c 02_add_to_ide_intr/drivers/ide/pci/piix.c

drivers/ide/pci/piix.c static void __devinit init_hwif_piix(ide · line 486

Quoted 4 lines
drivers/ide/pci/piix.c +1 -0
@@ -483,6 +483,7 @@ static void __devinit init_hwif_piix(ide
 	if (!hwif->dma_base)
 		return;
 
+	hwif->ide_dma_clear_irq = &__ide_dma_clear_irq;

If this handler is only called by single driver, why it's not defined locally?

drivers/ide/pci/piix.c static void __devinit init_hwif_piix(ide · line 489

Quoted 3 lines
drivers/ide/pci/piix.c +0 -0
 	hwif->atapi_dma = 1;
 	hwif->ultra_mask = 0x3f;
 	hwif->mwdma_mask = 0x06;

MBR, Sergei

#17 lore raw

Re: [PATCH 2/2] ide: clear bmdma status in ide_intr() for Intel controllers (revised #3)

In reply to Albert Lee
Quoted 3 lines · On Tue, 30 Jan 2007 11:02:03 +0800 Albert Lee <[email protected]> wrote:

patch 2/2 (revised):
- Fix drive->waiting_for_dma to work with CDB-intr devices.
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq for Intel controllers.

This should probably only be done for the ICH and later controllers with SATA, as the older ones appear to work fine, as do any not in native mode.

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

#19 lore raw

Re: [PATCH 2/2] ide: clear bmdma status in ide_intr() for Intel controllers (revised #3)

Hi,

Generally looks OK, thanks for working on it.

Some comments below.

Quoted 5 lines · Albert Lee

patch 2/2 (revised):
- Fix drive->waiting_for_dma to work with CDB-intr devices.
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq for Intel controllers.

Please make it patch 1/2 or just merge two patches together.

We should always have the kernel in the "working" state (so introduction of the new fix first, removal of the old one next).

Quoted 6 lines
Signed-off-by Albert Lee <[email protected]>

---

Calling hwif->ide_dma_clear_irq for all controllers looks risky. Revised to do it only for the Intel controllers.
(If any other adapters need it, we may add it later.)

OK

Quoted 38 lines

Patch against 2.6.20-rc6. Tested ok on my ICH4 and pdc20275 adapters. Please review/apply, thanks.

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c

drivers/ide/ide-cd.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-cd.c 2007-01-29 17:23:34.000000000 +0800 +8 -0
@@ -923,6 +923,10 @@ static ide_startstop_t cdrom_start_packe
               HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
 
       if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+               /* waiting for CDB interrupt, not DMA yet. */
+               if (info->dma)
+                       drive->waiting_for_dma = 0;
+
               /* packet command */
               ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
               return ide_started;

@@ -965,6 +969,10 @@ static ide_startstop_t cdrom_transfer_pa
               /* Check for errors. */
               if (cdrom_decode_status(drive, DRQ_STAT, NULL))
                       return ide_stopped;
+
+               /* Ok, next interrupt will be DMA interrupt. */
+               if (info->dma)
+                       drive->waiting_for_dma = 1;
       } else {
               /* Otherwise, we must wait for DRQ to get set. */
               if (ide_wait_stat(&startstop, drive, DRQ_STAT,

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-dma.c 02_add_to_ide_intr/drivers/ide/ide-dma.c

drivers/ide/ide-dma.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-dma.c 2006-11-30 05:57:37.000000000 +0800 +3 -0
@@ -650,6 +650,20 @@ static int __ide_dma_test_irq(ide_drive_
                       drive->name, __FUNCTION__);
       return 0;
 }
+
+void __ide_dma_clear_irq(ide_drive_t *drive)
+{

drivers/ide/ide-dma.c 2007-01-29 17:23:58.000000000 +0800 static int __ide_dma_test_irq(ide_drive_ · line 665

Quoted 10 lines
drivers/ide/ide-dma.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-dma.c 2006-11-30 05:57:37.000000000 +0800 +10 -0
+       ide_hwif_t *hwif = HWIF(drive);
+       u8 dma_stat;
+
+       /* clear the INTR & ERROR bits */
+       dma_stat = hwif->INB(hwif->dma_status);
+       /* Should we force the bit as well ? */
+       hwif->OUTB(dma_stat, hwif->dma_status);
+}
+
+EXPORT_SYMBOL_GPL(__ide_dma_clear_irq);

As noted by Sergei, this belongs to piix.c
(at least for now, we can change it when needed).

Quoted 13 lines
drivers/ide/ide-dma.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-dma.c 2006-11-30 05:57:37.000000000 +0800 +0 -0
 #endif /* CONFIG_BLK_DEV_IDEDMA_PCI */
 
 int __ide_dma_bad_drive (ide_drive_t *drive)

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c

drivers/ide/ide-io.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-io.c 2006-11-30 05:57:37.000000000 +0800 +3 -0
@@ -1644,6 +1644,18 @@ irqreturn_t ide_intr (int irq, void *dev
       }
       hwgroup->handler = NULL;
       del_timer(&hwgroup->timer);
+
+       /* Some controllers might set DMA INTR no matter DMA or PIO;
+        * bmdma status might need to be cleared even for

drivers/ide/ide-io.c 2007-01-29 17:23:58.000000000 +0800 irqreturn_t ide_intr (int irq, void *dev · line 1659

Quoted 10 lines
drivers/ide/ide-io.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-io.c 2006-11-30 05:57:37.000000000 +0800 +9 -0
+        * PIO interrupts to prevent spurious/lost irq.
+        */
+       if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
+               /* ide_dma_end() needs bmdma status for error checking.
+                * So, skip clearing bmdma status here and leave it
+                * to ide_dma_end() if this is dma interrupt.
+                */
+               hwif->ide_dma_clear_irq(drive);
+
       spin_unlock(&ide_lock);

Is there a reason to protect ->ide_dma_clear_irq call by the ide_lock?

If not please move the call outside the lock.

Quoted 16 lines
drivers/ide/ide-io.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide-io.c 2006-11-30 05:57:37.000000000 +0800 +0 -0
       if (drive->unmask)

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c

drivers/ide/ide.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/ide.c 2007-01-29 17:19:48.000000000 +0800 +1 -0
@@ -503,6 +503,7 @@ static void ide_hwif_restore(ide_hwif_t
       hwif->ide_dma_on                = tmp_hwif->ide_dma_on;
       hwif->ide_dma_off_quietly       = tmp_hwif->ide_dma_off_quietly;
       hwif->ide_dma_test_irq          = tmp_hwif->ide_dma_test_irq;
+       hwif->ide_dma_clear_irq         = tmp_hwif->ide_dma_clear_irq;
       hwif->ide_dma_host_on           = tmp_hwif->ide_dma_host_on;
       hwif->ide_dma_host_off          = tmp_hwif->ide_dma_host_off;
       hwif->ide_dma_lostirq           = tmp_hwif->ide_dma_lostirq;

diff -Nrup 01_remove_from_ide_cd/drivers/ide/pci/piix.c 02_add_to_ide_intr/drivers/ide/pci/piix.c

drivers/ide/pci/piix.c 2007-01-29 17:23:58.000000000 +0800 static void __devinit init_hwif_piix(ide · line 486

Quoted 4 lines
drivers/ide/pci/piix.c 2007-01-29 17:23:58.000000000 +0800 renamed from drivers/ide/pci/piix.c 2007-01-29 17:23:34.000000000 +0800 +1 -0
@@ -483,6 +483,7 @@ static void __devinit init_hwif_piix(ide
       if (!hwif->dma_base)
               return;
 
+       hwif->ide_dma_clear_irq = &__ide_dma_clear_irq;

As noted by Alan, this should be probably done only for newer chipsets. I think that we can safely do it for all ICHx chipsets.

It should be possible to split out ICHx PCI ID filter from init_chipset_piix() (as this function does the tuning only on ICHx chipsets) to some helper function and use it later in init_hwif_piix().

With the above corrections I'll happily add the patch to my tree.

Thanks,
Bart

author
#20 lore raw

cover ide: clear bmdma status in ide_intr for Intel controllers (revised #4)

patch 1/2: clear bmdma status in ide_intr() and add a new hwif->ide_dma_clear_irq for piix.c to use. patch 2/2: remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.

author
#21 lore raw

PATCH 1/2 ide: clear bmdma status in ide_intr() for ICHx controllers (revised #4)

patch 1/2 (revised):
- Fix drive->waiting_for_dma to work with CDB-intr devices.
- Do the dma status clearing in ide_intr() and add a new hwif->ide_dma_clear_irq for Intel ICHx controllers.

Signed-off-by Albert Lee <[email protected]>

---
Revised per Alan, Sergei and Bart's advice.

Patch against 2.6.20-rc6. Tested ok on my ICH4 and pdc20275 adapters. Please review/apply, thanks.

drivers/ide/ide-cd.c +8 -0
diff -Nrup 00_ide_dma/drivers/ide/ide-cd.c 01_add_to_ide_intr/drivers/ide/ide-cd.c
@@ -930,6 +930,10 @@ static ide_startstop_t cdrom_start_packe
 		HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
  
 	if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
+		/* waiting for CDB interrupt, not DMA yet. */
+		if (info->dma)
+			drive->waiting_for_dma = 0;
+
 		/* packet command */
 		ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
 		return ide_started;

@@ -972,6 +976,10 @@ static ide_startstop_t cdrom_transfer_pa
 		/* Check for errors. */
 		if (cdrom_decode_status(drive, DRQ_STAT, NULL))
 			return ide_stopped;
+
+		/* Ok, next interrupt will be DMA interrupt. */
+		if (info->dma)
+			drive->waiting_for_dma = 1;
 	} else {
 		/* Otherwise, we must wait for DRQ to get set. */
 		if (ide_wait_stat(&startstop, drive, DRQ_STAT,
drivers/ide/ide-io.c +11 -0
diff -Nrup 00_ide_dma/drivers/ide/ide-io.c 01_add_to_ide_intr/drivers/ide/ide-io.c
@@ -1646,6 +1646,17 @@ irqreturn_t ide_intr (int irq, void *dev
 	del_timer(&hwgroup->timer);
 	spin_unlock(&ide_lock);
 
+	/* Some controllers might set DMA INTR no matter DMA or PIO;
+	 * bmdma status might need to be cleared even for
+	 * PIO interrupts to prevent spurious/lost irq.
+	 */
+	if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
+		/* ide_dma_end() needs bmdma status for error checking.
+		 * So, skip clearing bmdma status here and leave it
+		 * to ide_dma_end() if this is dma interrupt.
+		 */
+		hwif->ide_dma_clear_irq(drive);
+
 	if (drive->unmask)
 		local_irq_enable_in_hardirq();
 	/* service this interrupt, may set handler for next interrupt */
drivers/ide/ide.c +1 -0
diff -Nrup 00_ide_dma/drivers/ide/ide.c 01_add_to_ide_intr/drivers/ide/ide.c
@@ -503,6 +503,7 @@ static void ide_hwif_restore(ide_hwif_t 
 	hwif->ide_dma_on		= tmp_hwif->ide_dma_on;
 	hwif->ide_dma_off_quietly	= tmp_hwif->ide_dma_off_quietly;
 	hwif->ide_dma_test_irq		= tmp_hwif->ide_dma_test_irq;
+	hwif->ide_dma_clear_irq		= tmp_hwif->ide_dma_clear_irq;
 	hwif->ide_dma_host_on		= tmp_hwif->ide_dma_host_on;
 	hwif->ide_dma_host_off		= tmp_hwif->ide_dma_host_off;
 	hwif->ide_dma_lostirq		= tmp_hwif->ide_dma_lostirq;
drivers/ide/pci/piix.c +48 -15

90 lines not on this page · raw message

include/linux/ide.h +1 -0
diff -Nrup 00_ide_dma/include/linux/ide.h 01_add_to_ide_intr/include/linux/ide.h
@@ -727,6 +727,7 @@ typedef struct hwif_s {
 	int (*ide_dma_on)(ide_drive_t *drive);
 	int (*ide_dma_off_quietly)(ide_drive_t *drive);
 	int (*ide_dma_test_irq)(ide_drive_t *drive);
+	void (*ide_dma_clear_irq)(ide_drive_t *drive);
 	int (*ide_dma_host_on)(ide_drive_t *drive);
 	int (*ide_dma_host_off)(ide_drive_t *drive);
 	int (*ide_dma_lostirq)(ide_drive_t *drive);
author
#22 lore raw

PATCH 2/2 ide: remove clearing bmdma status from cdrom_decode_status() (rev #4)

In reply to Albert Lee

patch 2/2:
Remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.
(http://lkml.org/lkml/2006/12/4/201 and http://lkml.org/lkml/2006/11/15/94)

Signed-off-by Albert Lee <[email protected]>

---

drivers/ide/ide-cd.c +0 -7
diff -Nrup 01_add_to_ide_intr/drivers/ide/ide-cd.c 02_remove_from_ide_cd/drivers/ide/ide-cd.c
@@ -687,15 +687,8 @@ static void ide_dump_status_no_sense(ide
 static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
 {
 	struct request *rq = HWGROUP(drive)->rq;
-	ide_hwif_t *hwif = HWIF(drive);
 	int stat, err, sense_key;
 	
-	/* We may have bogus DMA interrupts in PIO state here */
-	if (HWIF(drive)->dma_status && hwif->atapi_irq_bogon) {
-		stat = hwif->INB(hwif->dma_status);
-		/* Should we force the bit as well ? */
-		hwif->OUTB(stat, hwif->dma_status);
-	}
 	/* Check for errors. */
 	stat = HWIF(drive)->INB(IDE_STATUS_REG);
 	if (stat_ret)
drivers/ide/pci/piix.c +0 -4
diff -Nrup 01_add_to_ide_intr/drivers/ide/pci/piix.c 02_remove_from_ide_cd/drivers/ide/pci/piix.c
@@ -502,10 +502,6 @@ static void __devinit init_hwif_piix(ide
 		/* This is a painful system best to let it self tune for now */
 		return;
 	}
-	/* ESB2 appears to generate spurious DMA interrupts in PIO mode
-	   when in native mode */
-	if (hwif->pci_dev->device == PCI_DEVICE_ID_INTEL_ESB2_18)
-		hwif->atapi_irq_bogon = 1;
 
 	hwif->autodma = 0;
 	hwif->tuneproc = &piix_tune_drive;
include/linux/ide.h +0 -1
diff -Nrup 01_add_to_ide_intr/include/linux/ide.h 02_remove_from_ide_cd/include/linux/ide.h
@@ -797,7 +797,6 @@ typedef struct hwif_s {
 	unsigned	sg_mapped  : 1;	/* sg_table and sg_nents are ready */
 	unsigned	no_io_32bit : 1; /* 1 = can not do 32-bit IO ops */
 	unsigned	err_stops_fifo : 1; /* 1=data FIFO is cleared by an error */
-	unsigned	atapi_irq_bogon : 1; /* Generates spurious DMA interrupts in PIO mode */
 
 	struct device	gendev;
 	struct completion gendev_rel_comp; /* To deal with device release() */

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

Applied
#24 lore raw

Re: [PATCH 0/2] ide: clear bmdma status in ide_intr for Intel controllers (revised #4)

Quoted 2 lines · Albert Lee

patch 1/2: clear bmdma status in ide_intr() and add a new hwif->ide_dma_clear_irq for piix.c to use. patch 2/2: remove clearing bmdma status from cdrom_decode_status() since ATA devices might need it as well.

applied both patches, thanks

#25 lore raw
In reply to Albert Lee
Quoted 2 lines

Instead of adding PCI IDs to the blacklist, could we just unconditionally clear the dma_status:INTR in ide_dma_start()?

Possibly. I did the patch to be as safe as possible close to an actual release. If someone wants to do the general legacy IDE layer testing with assorted controllers/drives then it would probably be the best approach.