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

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

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

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
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.
====================================================================

--- ide-dma.ori/drivers/ide/ide-dma.c	2006-11-30 05:57:37.000000000 +0800
+++ ide-dma/drivers/ide/ide-dma.c	2007-01-18 18:40:18.000000000 +0800
@@ -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.

Re: [PATCH] ide: Fix ATAPI DMA lost irq problem with CDB intr devices

[Bartlomiej Zolnierkiewicz]

Hi,

Albert Lee wrote:
> 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?

> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> 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.

Does the problem also happen on ICH4?

> ====================================================================
> 
> --- ide-dma.ori/drivers/ide/ide-dma.c   2006-11-30 05:57:37.000000000 +0800
> +++ ide-dma/drivers/ide/ide-dma.c       2007-01-18 18:40:18.000000000 +0800
> @@ -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.

Thanks,
Bart

Re: [PATCH] ide: Fix ATAPI DMA lost irq problem with CDB intr devices

[Albert Lee]

Hi Bart,
> 
> Albert Lee wrote:
> 
>>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...

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.

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

Re: [PATCH] ide: Fix ATAPI DMA lost irq problem with CDB intr devices

[Albert Lee]

Albert Lee wrote:
> Hi Bart,
> 
>>Albert Lee wrote:
>>
>>
>>>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...
> 
> 
> 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/linux-ide@vger.kernel.org/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

Re: [PATCH] ide: Fix ATAPI DMA lost irq problem with CDB intr devices

[Bartlomiej Zolnierkiewicz]

Hi,

On 1/22/07, Albert Lee <albertcc@tw.ibm.com> wrote:
> Albert Lee wrote:
> > Hi Bart,
> >
> >>Albert Lee wrote:
> >>
> >>
> >>>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...
> >
> >
> > 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/linux-ide@vger.kernel.org/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.

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

Please keep me in the loop.

Thanks,
Bart

[PATCH 0/2] ide: clear bmdma status in ide_intr for all commands

[Albert Lee]

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.

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

[Albert Lee]

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 <albertcc@tw.ibm.com>
---
diff -Nrup 00_ide_dma/drivers/ide/ide-cd.c 01_remove_from_ide_cd/drivers/ide/ide-cd.c
--- 00_ide_dma/drivers/ide/ide-cd.c	2007-01-23 11:47:42.000000000 +0800
+++ 01_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-24 11:00:03.000000000 +0800
@@ -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)
diff -Nrup 00_ide_dma/drivers/ide/pci/piix.c 01_remove_from_ide_cd/drivers/ide/pci/piix.c
--- 00_ide_dma/drivers/ide/pci/piix.c	2007-01-23 11:47:42.000000000 +0800
+++ 01_remove_from_ide_cd/drivers/ide/pci/piix.c	2007-01-24 11:00:03.000000000 +0800
@@ -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;
diff -Nrup 00_ide_dma/include/linux/ide.h 01_remove_from_ide_cd/include/linux/ide.h
--- 00_ide_dma/include/linux/ide.h	2007-01-23 11:47:48.000000000 +0800
+++ 01_remove_from_ide_cd/include/linux/ide.h	2007-01-24 11:00:03.000000000 +0800
@@ -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() */

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

[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 <albertcc@tw.ibm.com>
---
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
--- 01_remove_from_ide_cd/drivers/ide/ide-dma.c	2006-11-30 05:57:37.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-dma.c	2007-01-24 11:07:58.000000000 +0800
@@ -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)
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c
--- 01_remove_from_ide_cd/drivers/ide/ide-io.c	2006-11-30 05:57:37.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-io.c	2007-01-24 11:21:41.000000000 +0800
@@ -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)
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c
--- 01_remove_from_ide_cd/drivers/ide/ide.c	2007-01-23 11:47:42.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide.c	2007-01-24 11:00:48.000000000 +0800
@@ -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/include/linux/ide.h 02_add_to_ide_intr/include/linux/ide.h
--- 01_remove_from_ide_cd/include/linux/ide.h	2007-01-24 11:00:03.000000000 +0800
+++ 02_add_to_ide_intr/include/linux/ide.h	2007-01-24 11:00:48.000000000 +0800
@@ -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);

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

[Sergei Shtylyov]

Hello.

Albert Lee wrote:
> 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 <albertcc@tw.ibm.com>
> ---
> 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
> --- 01_remove_from_ide_cd/drivers/ide/ide-dma.c	2006-11-30 05:57:37.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-dma.c	2007-01-24 11:07:58.000000000 +0800
> @@ -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?

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

> +		dma_stat = hwif->INB(hwif->dma_status);
> +		/* Should we force the bit as well ? */

    Good idea.

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

> +	}
> +
> +	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
> --- 01_remove_from_ide_cd/drivers/ide/ide-io.c	2006-11-30 05:57:37.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-io.c	2007-01-24 11:21:41.000000000 +0800
> @@ -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. :-)

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

>  	if (drive->unmask)

MBR, Sergei

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

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

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
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.


diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c
--- 01_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-24 11:00:03.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-cd.c	2007-01-25 16:52:20.000000000 +0800
@@ -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
--- 01_remove_from_ide_cd/drivers/ide/ide-dma.c	2006-11-30 05:57:37.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-dma.c	2007-01-25 16:37:24.000000000 +0800
@@ -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)
diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-io.c 02_add_to_ide_intr/drivers/ide/ide-io.c
--- 01_remove_from_ide_cd/drivers/ide/ide-io.c	2006-11-30 05:57:37.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-io.c	2007-01-25 16:27:21.000000000 +0800
@@ -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
--- 01_remove_from_ide_cd/drivers/ide/ide.c	2007-01-23 11:47:42.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide.c	2007-01-24 11:00:48.000000000 +0800
@@ -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/include/linux/ide.h 02_add_to_ide_intr/include/linux/ide.h
--- 01_remove_from_ide_cd/include/linux/ide.h	2007-01-24 11:00:03.000000000 +0800
+++ 02_add_to_ide_intr/include/linux/ide.h	2007-01-25 16:10:42.000000000 +0800
@@ -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);

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

[Sergei Shtylyov]

Albert Lee wrote:
> 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 <albertcc@tw.ibm.com>
> ---
> 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...

> 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
> --- 01_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-24 11:00:03.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-cd.c	2007-01-25 16:52:20.000000000 +0800
> @@ -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,

    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

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

[Sergei Shtylyov]

Hello.

Sergei Shtylyov wrote:

>> diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c
>> --- 01_remove_from_ide_cd/drivers/ide/ide-cd.c    2007-01-24 11:00:03.000000000 +0800
>> +++ 02_add_to_ide_intr/drivers/ide/ide-cd.c    2007-01-25 16:52:20.000000000 +0800
>> @@ -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,

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

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

MBR, Sergei

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

[Albert Lee]

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.

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

[Albert Lee]

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 <albertcc@tw.ibm.com>
---

diff -Nrup 00_ide_dma/drivers/ide/ide-cd.c 01_remove_from_ide_cd/drivers/ide/ide-cd.c
--- 00_ide_dma/drivers/ide/ide-cd.c	2007-01-29 17:19:48.000000000 +0800
+++ 01_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-29 17:23:34.000000000 +0800
@@ -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)
diff -Nrup 00_ide_dma/drivers/ide/pci/piix.c 01_remove_from_ide_cd/drivers/ide/pci/piix.c
--- 00_ide_dma/drivers/ide/pci/piix.c	2007-01-29 17:19:48.000000000 +0800
+++ 01_remove_from_ide_cd/drivers/ide/pci/piix.c	2007-01-29 17:23:34.000000000 +0800
@@ -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;
diff -Nrup 00_ide_dma/include/linux/ide.h 01_remove_from_ide_cd/include/linux/ide.h
--- 00_ide_dma/include/linux/ide.h	2007-01-29 17:19:53.000000000 +0800
+++ 01_remove_from_ide_cd/include/linux/ide.h	2007-01-29 17:23:34.000000000 +0800
@@ -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() */

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

[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 <albertcc@tw.ibm.com>
---

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.

diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c
--- 01_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-29 17:23:34.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-cd.c	2007-01-29 17:23:58.000000000 +0800
@@ -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
--- 01_remove_from_ide_cd/drivers/ide/ide-dma.c	2006-11-30 05:57:37.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-dma.c	2007-01-29 17:23:58.000000000 +0800
@@ -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
--- 01_remove_from_ide_cd/drivers/ide/ide-io.c	2006-11-30 05:57:37.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide-io.c	2007-01-29 17:23:58.000000000 +0800
@@ -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
--- 01_remove_from_ide_cd/drivers/ide/ide.c	2007-01-29 17:19:48.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/ide.c	2007-01-29 17:23:58.000000000 +0800
@@ -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
--- 01_remove_from_ide_cd/drivers/ide/pci/piix.c	2007-01-29 17:23:34.000000000 +0800
+++ 02_add_to_ide_intr/drivers/ide/pci/piix.c	2007-01-29 17:23:58.000000000 +0800
@@ -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;
diff -Nrup 01_remove_from_ide_cd/include/linux/ide.h 02_add_to_ide_intr/include/linux/ide.h
--- 01_remove_from_ide_cd/include/linux/ide.h	2007-01-29 17:23:34.000000000 +0800
+++ 02_add_to_ide_intr/include/linux/ide.h	2007-01-29 17:23:58.000000000 +0800
@@ -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 */

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

[Sergei Shtylyov]

Hello.

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

> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---

> Calling hwif->ide_dma_clear_irq for all controllers looks risky.

    I'm not sure why it's so 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.

> diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide-cd.c 02_add_to_ide_intr/drivers/ide/ide-cd.c
> --- 01_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-29 17:23:34.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-cd.c	2007-01-29 17:23:58.000000000 +0800
> @@ -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
> --- 01_remove_from_ide_cd/drivers/ide/ide-dma.c	2006-11-30 05:57:37.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-dma.c	2007-01-29 17:23:58.000000000 +0800
> @@ -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
> --- 01_remove_from_ide_cd/drivers/ide/ide-io.c	2006-11-30 05:57:37.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-io.c	2007-01-29 17:23:58.000000000 +0800
> @@ -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
> --- 01_remove_from_ide_cd/drivers/ide/ide.c	2007-01-29 17:19:48.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide.c	2007-01-29 17:23:58.000000000 +0800
> @@ -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
> --- 01_remove_from_ide_cd/drivers/ide/pci/piix.c	2007-01-29 17:23:34.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/pci/piix.c	2007-01-29 17:23:58.000000000 +0800
> @@ -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?

>  	hwif->atapi_dma = 1;
>  	hwif->ultra_mask = 0x3f;
>  	hwif->mwdma_mask = 0x06;

MBR, Sergei

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

[Alan]

On Tue, 30 Jan 2007 11:02:03 +0800
Albert Lee <albertcc@tw.ibm.com> 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.

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

[Bartlomiej Zolnierkiewicz]

Hi,

Generally looks OK, thanks for working on it.

Some comments below.

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

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

> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> 
> 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

> 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
> --- 01_remove_from_ide_cd/drivers/ide/ide-cd.c  2007-01-29 17:23:34.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-cd.c     2007-01-29 17:23:58.000000000 +0800
> @@ -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
> --- 01_remove_from_ide_cd/drivers/ide/ide-dma.c 2006-11-30 05:57:37.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-dma.c    2007-01-29 17:23:58.000000000 +0800
> @@ -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);

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

> #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
> --- 01_remove_from_ide_cd/drivers/ide/ide-io.c  2006-11-30 05:57:37.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide-io.c     2007-01-29 17:23:58.000000000 +0800
> @@ -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);

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

If not please move the call outside the lock.

>        if (drive->unmask)
> diff -Nrup 01_remove_from_ide_cd/drivers/ide/ide.c 02_add_to_ide_intr/drivers/ide/ide.c
> --- 01_remove_from_ide_cd/drivers/ide/ide.c     2007-01-29 17:19:48.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/ide.c        2007-01-29 17:23:58.000000000 +0800
> @@ -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
> --- 01_remove_from_ide_cd/drivers/ide/pci/piix.c        2007-01-29 17:23:34.000000000 +0800
> +++ 02_add_to_ide_intr/drivers/ide/pci/piix.c   2007-01-29 17:23:58.000000000 +0800
> @@ -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

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

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

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

[Albert Lee]

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 <albertcc@tw.ibm.com>
---
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.

diff -Nrup 00_ide_dma/drivers/ide/ide-cd.c 01_add_to_ide_intr/drivers/ide/ide-cd.c
--- 00_ide_dma/drivers/ide/ide-cd.c	2007-01-29 17:19:48.000000000 +0800
+++ 01_add_to_ide_intr/drivers/ide/ide-cd.c	2007-01-31 11:01:03.000000000 +0800
@@ -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,
diff -Nrup 00_ide_dma/drivers/ide/ide-io.c 01_add_to_ide_intr/drivers/ide/ide-io.c
--- 00_ide_dma/drivers/ide/ide-io.c	2006-11-30 05:57:37.000000000 +0800
+++ 01_add_to_ide_intr/drivers/ide/ide-io.c	2007-01-31 12:19:09.000000000 +0800
@@ -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 */
diff -Nrup 00_ide_dma/drivers/ide/ide.c 01_add_to_ide_intr/drivers/ide/ide.c
--- 00_ide_dma/drivers/ide/ide.c	2007-01-29 17:19:48.000000000 +0800
+++ 01_add_to_ide_intr/drivers/ide/ide.c	2007-01-31 11:01:03.000000000 +0800
@@ -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 00_ide_dma/drivers/ide/pci/piix.c 01_add_to_ide_intr/drivers/ide/pci/piix.c
--- 00_ide_dma/drivers/ide/pci/piix.c	2007-01-29 17:19:48.000000000 +0800
+++ 01_add_to_ide_intr/drivers/ide/pci/piix.c	2007-01-31 13:38:42.000000000 +0800
@@ -411,17 +411,14 @@ fast_ata_pio:
 }
 
 /**
- *	init_chipset_piix	-	set up the PIIX chipset
- *	@dev: PCI device to set up
- *	@name: Name of the device
+ *	piix_is_ichx	-	check if ICHx
+ *	@dev: PCI device to check
  *
- *	Initialize the PCI device as required. For the PIIX this turns
- *	out to be nice and simple
+ *	returns 1 if ICHx, 0 otherwise.
  */
- 
-static unsigned int __devinit init_chipset_piix (struct pci_dev *dev, const char *name)
+static int piix_is_ichx(struct pci_dev *dev)
 {
-        switch(dev->device) {
+        switch (dev->device) {
 		case PCI_DEVICE_ID_INTEL_82801EB_1:
 		case PCI_DEVICE_ID_INTEL_82801AA_1:
 		case PCI_DEVICE_ID_INTEL_82801AB_1:
@@ -439,19 +436,51 @@ static unsigned int __devinit init_chips
 		case PCI_DEVICE_ID_INTEL_ICH7_21:
 		case PCI_DEVICE_ID_INTEL_ESB2_18:
 		case PCI_DEVICE_ID_INTEL_ICH8_6:
-		{
-			unsigned int extra = 0;
-			pci_read_config_dword(dev, 0x54, &extra);
-			pci_write_config_dword(dev, 0x54, extra|0x400);
-		}
-		default:
-			break;
+			return 1;
+	}
+	
+	return 0;
+}
+
+/**
+ *	init_chipset_piix	-	set up the PIIX chipset
+ *	@dev: PCI device to set up
+ *	@name: Name of the device
+ *
+ *	Initialize the PCI device as required. For the PIIX this turns
+ *	out to be nice and simple
+ */
+ 
+static unsigned int __devinit init_chipset_piix (struct pci_dev *dev, const char *name)
+{
+	if (piix_is_ichx(dev)) {
+		unsigned int extra = 0;
+		pci_read_config_dword(dev, 0x54, &extra);
+		pci_write_config_dword(dev, 0x54, extra|0x400);
 	}
 
 	return 0;
 }
 
 /**
+ *	piix_dma_clear_irq	-	clear BMDMA status
+ *	@drive: IDE drive to clear
+ *
+ *	Called from ide_intr() for PIO interrupts 
+ *	to clear BMDMA status as needed by ICHx
+ */
+static void piix_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);
+}
+
+/**
  *	init_hwif_piix		-	fill in the hwif for the PIIX
  *	@hwif: IDE interface
  *
@@ -487,6 +516,10 @@ static void __devinit init_hwif_piix(ide
 	if (!hwif->dma_base)
 		return;
 
+	/* ICHx need to clear the bmdma status for all interrupts */
+	if (piix_is_ichx(hwif->pci_dev))
+		hwif->ide_dma_clear_irq = &piix_dma_clear_irq;
+
 	hwif->atapi_dma = 1;
 	hwif->ultra_mask = 0x3f;
 	hwif->mwdma_mask = 0x06;
diff -Nrup 00_ide_dma/include/linux/ide.h 01_add_to_ide_intr/include/linux/ide.h
--- 00_ide_dma/include/linux/ide.h	2007-01-29 17:19:53.000000000 +0800
+++ 01_add_to_ide_intr/include/linux/ide.h	2007-01-31 11:10:27.000000000 +0800
@@ -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);

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

[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 <albertcc@tw.ibm.com>
---

diff -Nrup 01_add_to_ide_intr/drivers/ide/ide-cd.c 02_remove_from_ide_cd/drivers/ide/ide-cd.c
--- 01_add_to_ide_intr/drivers/ide/ide-cd.c	2007-01-31 11:01:03.000000000 +0800
+++ 02_remove_from_ide_cd/drivers/ide/ide-cd.c	2007-01-31 13:28:11.000000000 +0800
@@ -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)
diff -Nrup 01_add_to_ide_intr/drivers/ide/pci/piix.c 02_remove_from_ide_cd/drivers/ide/pci/piix.c
--- 01_add_to_ide_intr/drivers/ide/pci/piix.c	2007-01-31 13:38:42.000000000 +0800
+++ 02_remove_from_ide_cd/drivers/ide/pci/piix.c	2007-01-31 13:42:40.000000000 +0800
@@ -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;
diff -Nrup 01_add_to_ide_intr/include/linux/ide.h 02_remove_from_ide_cd/include/linux/ide.h
--- 01_add_to_ide_intr/include/linux/ide.h	2007-01-31 11:10:27.000000000 +0800
+++ 02_remove_from_ide_cd/include/linux/ide.h	2007-01-31 13:28:11.000000000 +0800
@@ -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() */

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

[Bartlomiej Zolnierkiewicz]

Albert Lee wrote:
> 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

Re: [PATCH] ide: Fix ATAPI DMA lost irq problem with CDB intr devices

[Alan]

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