[BK PATCHES] libata fixes

[BK PATCHES] libata fixes

[Jeff Garzik]

Linus, please do a

	bk pull bk://gkernel.bkbits.net/libata-2.5

This will update the following files:

 drivers/scsi/libata.h       |    2 +-
 drivers/scsi/sata_promise.c |   18 ++++++++++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

through these ChangeSets:

<jgarzik@redhat.com> (03/11/07 1.1417)
   [libata] fix Promise PCI posting bugs

<jgarzik@redhat.com> (03/11/06 1.1416)
   [libata] bump libata version

<jgarzik@redhat.com> (03/11/06 1.1415)
   [libata] fix ugly Promise interrupt masking bug

Re: [BK PATCHES] libata fixes

[Sergey Vlasov]

(Not about this particular libata update)

static void __init quirk_intel_ide_combined(struct pci_dev *pdev)
...
	{ PCI_FIXUP_FINAL,      PCI_VENDOR_ID_INTEL,    PCI_ANY_ID,
	  quirk_intel_ide_combined },

Won't this oops if some Intel device would be hotplugged?  A similar
problem with quirk_via_bridge was just fixed.

Re: [BK PATCHES] libata fixes

[Jeff Garzik]

Sergey Vlasov wrote:
> (Not about this particular libata update)
> 
> static void __init quirk_intel_ide_combined(struct pci_dev *pdev)
> ...
> 	{ PCI_FIXUP_FINAL,      PCI_VENDOR_ID_INTEL,    PCI_ANY_ID,
> 	  quirk_intel_ide_combined },
> 
> Won't this oops if some Intel device would be hotplugged?  A similar
> problem with quirk_via_bridge was just fixed.


We should probably start marking entries as "no hotplug" because many of 
the quirks have no need to be run after initial boot.

	Jeff

Success with Promise FastTrak S150 TX4 (Re: [BK PATCHES] libata fixes)

[Oliver M. Bolzer]

On Sat, Nov 08, 2003 at 12:26:21PM -0500, Jeff Garzik <jgarzik@pobox.com> wrote...
 
> <jgarzik@redhat.com> (03/11/06 1.1415)
>    [libata] fix ugly Promise interrupt masking bug

This solved the last outstanding problem with the 4th drive and the
driver seems to find all drives and properly boot off them, at least
in a situation where no RAID-functionalty of the card is used. Great
Work.

# A first quick run of bonnie++ seems to show 2.6.0-test9+libata several
# %s slower then 2.4.22+ft3xx, but that might be related to differences
# between 2.4 and 2.6.

-- 
	Oliver M. Bolzer
	oliver@gol.com

GPG (PGP) Fingerprint = 621B 52F6 2AC1 36DB 8761  018F 8786 87AD EF50 D1FF

Re: Success with Promise FastTrak S150 TX4 (Re: [BK PATCHES] libata fixes)

[Jeff Garzik]

Oliver M. Bolzer wrote:
> On Sat, Nov 08, 2003 at 12:26:21PM -0500, Jeff Garzik <jgarzik@pobox.com> wrote...
>  
> 
>><jgarzik@redhat.com> (03/11/06 1.1415)
>>   [libata] fix ugly Promise interrupt masking bug
> 
> 
> This solved the last outstanding problem with the 4th drive and the
> driver seems to find all drives and properly boot off them, at least
> in a situation where no RAID-functionalty of the card is used. Great
> Work.

Thanks for testing.


> # A first quick run of bonnie++ seems to show 2.6.0-test9+libata several
> # %s slower then 2.4.22+ft3xx, but that might be related to differences
> # between 2.4 and 2.6.

One possibility is that queueing is not yet enabled in my sata_promise 
driver.  Several of the SATA drivers support having multiple commands 
outstanding per driver (tagged command queueing), but I need to do a bit 
more work before I can enable queueing in the core.

	Jeff

Re: Success with Promise FastTrak S150 TX4 (Re: [BK PATCHES] libata fixes)

[Oliver M. Bolzer]

On Mon, Nov 10, 2003 at 11:26:16AM -0500, Jeff Garzik <jgarzik@pobox.com> wrote...
 
> ># A first quick run of bonnie++ seems to show 2.6.0-test9+libata several
> ># %s slower then 2.4.22+ft3xx, but that might be related to differences
> ># between 2.4 and 2.6.
> 
> One possibility is that queueing is not yet enabled in my sata_promise 
> driver.  Several of the SATA drivers support having multiple commands 
> outstanding per driver (tagged command queueing), but I need to do a bit 
> more work before I can enable queueing in the core.

I disabled /dev/mdX Software-RAID5 on two of the boxes that have the
hardware (got 8 of them) and did some preliminary benchmarking using
bonnie++ 1.02b. The hardware is P4-2.4GHz, 1GB RAM (highmem-enabled) wich
4 Maxtor SATA-drives with 200GBs each. The partitions I ran the tests
on are 186GB large.

2.4.22 + Promise ft3xx
silo4:/dev/sda3

Version 1.02b       ------Sequential Output------ --Sequential Input- --Random-
                    -Per Chr- --Block-- -Rewrite- -Per Chr- --Block-- --Seeks--
Machine        Size K/sec %CP K/sec %CP K/sec %CP K/sec %CP K/sec %CP  /sec %CP
silo4.rz.informa 2G 27330  95 62522  19 22619   6 29056  98 56311   6 159.7   0
                    ------Sequential Create------ --------Random Create--------
                    -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
              files  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
                 16  3007  98 +++++ +++ +++++ +++  3087  99 +++++ +++  9173 100
silo4,2G,27330,95,62522,19,22619,6,29056,98,56311,6,159.7,0,16,3007,98,+++++,+++,+++++,+++,3087,99,+++++,+++,9173,100

==========
2.4.22-bk48 + 2.4.22-bk48-libata1
silo5:/dev/sda3

Version 1.02b       ------Sequential Output------ --Sequential Input- --Random-
                    -Per Chr- --Block-- -Rewrite- -Per Chr- --Block-- --Seeks--
Machine        Size K/sec %CP K/sec %CP K/sec %CP K/sec %CP K/sec %CP  /sec %CP
silo5.rz.informa 2G 28068  96 61698  18 23174   5 24372  77 56009   6 175.1   0
                    ------Sequential Create------ --------Random Create--------
                    -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
              files  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
                 16  2986  97 +++++ +++ +++++ +++  3136  99 +++++ +++  9867  98
silo5,2G,28068,96,61698,18,23174,5,24372,77,56009,6,175.1,0,16,2986,97,+++++,+++,+++++,+++,3136,99,+++++,+++,9867,98


The performance differences are not that big that they would really
matter. One is better at one direction while the other is better at the
other direction.

I am REALLY happy that a free-as-in-freedom driver exist for the hardware
that has a much more certain future thant a
looks-like-GPL-on-the-first-look-but-one--.o-has-no-source driver. And
the libata code was really nice to read and follow, even though I knew
almost nothing about disk driver. THANK YOU!

-- 
	Oliver M. Bolzer
	oliver@gol.com

GPG (PGP) Fingerprint = 621B 52F6 2AC1 36DB 8761  018F 8786 87AD EF50 D1FF

Re: Success with Promise FastTrak S150 TX4 (Re: [BK PATCHES] libata fixes)

[Jeff Garzik]

Oliver M. Bolzer wrote:

> I disabled /dev/mdX Software-RAID5 on two of the boxes that have the
> hardware (got 8 of them) and did some preliminary benchmarking using
> bonnie++ 1.02b. The hardware is P4-2.4GHz, 1GB RAM (highmem-enabled) wich
> 4 Maxtor SATA-drives with 200GBs each. The partitions I ran the tests
> on are 186GB large.
[...]

> The performance differences are not that big that they would really
> matter. One is better at one direction while the other is better at the
> other direction.

Well, I'll still be looking to increase performance in libata, even such ;)

But stability before performance :)


> I am REALLY happy that a free-as-in-freedom driver exist for the hardware
> that has a much more certain future thant a
> looks-like-GPL-on-the-first-look-but-one--.o-has-no-source driver. And
> the libata code was really nice to read and follow, even though I knew
> almost nothing about disk driver. THANK YOU!

Thank Promise, too.  They are actively supporting my efforts on this 
driver with hardware and docs.  Promise has also provided Arjan (author 
of "pdcraid") with documentation on their vendor-specific RAID format, 
and they are making other efforts to better support open source, and be 
more friendly with the open source community.

	Jeff

Re: Success with Promise FastTrak S150 TX4 (Re: [BK PATCHES] libata fixes)

[Kevin P. Fleming]

Jeff Garzik wrote:

> Thank Promise, too.  They are actively supporting my efforts on this 
> driver with hardware and docs.  Promise has also provided Arjan (author 
> of "pdcraid") with documentation on their vendor-specific RAID format, 
> and they are making other efforts to better support open source, and be 
> more friendly with the open source community.

And it's working, I would not have purchased a SATA150 TX4 for my new 
server if libata did not have support for it. I know, it's small 
potatoes, they didn't make much money off that sale, but everything 
helps. The only other 4-port (non RAID) SATA card I have seen was from 
SIIG, but it has disappeared from their web site and distributors, and 
libata's support for the Sil3114 on it isn't ready yet anyway :-)

Re: [BK PATCHES] libata fixes

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

Linus Torvalds wrote:
> 
> On Fri, 5 Dec 2003, Jeff Garzik wrote:
> 
>>Linus, please do a
>>
>>	bk pull bk://gkernel.bkbits.net/libata-2.5
>>
>>This will update the following files:
>>
>> drivers/scsi/libata-core.c  |   17 ++---
>> drivers/scsi/sata_promise.c |  128 +++++++++++++++++++++++++-------------------
> 
> 
> Right now, I'm accepting one-liners that I think are "obvious" and also
> "very important" (ie fixes for oopses that anybody can trigger, rather
> than for example updates to one particular driver). So it sounds like I
> might accept _one_ of these:
> 
> 
>><jgarzik@redhat.com> (03/12/05 1.1498)
>>   [libata] fix use-after-free
>>
>>   Fixes oops some were seeing on module unload.
>>
>>   Caught by Jon Burgess.
> 
> 
> If this is basically an obvious one-liner ("move a kfree")?
> 
> Andrew is still off, and he can make a decision independently, but right
> now I'm not going to apply anything bigger.


Yep, split out and attached...

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1081 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1497  -> 1.1498 
#	drivers/scsi/libata-core.c	1.8     -> 1.9    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/12/05	jgarzik@redhat.com	1.1498
# [libata] fix use-after-free
# 
# Fixes oops some were seeing on module unload.
# 
# Caught by Jon Burgess.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c	Fri Dec  5 13:46:54 2003
+++ b/drivers/scsi/libata-core.c	Fri Dec  5 13:46:54 2003
@@ -3224,8 +3224,6 @@
 		scsi_host_put(ap->host); /* FIXME: check return val */
 	}
 
-	kfree(host_set);
-
 	pci_release_regions(pdev);
 
 	for (i = 0; i < host_set->n_ports; i++) {
@@ -3242,6 +3240,7 @@
 		}
 	}
 
+	kfree(host_set);
 	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 }

Re: [BK PATCHES] libata fixes

[Linus Torvalds]

On Fri, 5 Dec 2003, Jeff Garzik wrote:
>
> Linus, please do a
>
> 	bk pull bk://gkernel.bkbits.net/libata-2.5
>
> This will update the following files:
>
>  drivers/scsi/libata-core.c  |   17 ++---
>  drivers/scsi/sata_promise.c |  128 +++++++++++++++++++++++++-------------------

Right now, I'm accepting one-liners that I think are "obvious" and also
"very important" (ie fixes for oopses that anybody can trigger, rather
than for example updates to one particular driver). So it sounds like I
might accept _one_ of these:

> <jgarzik@redhat.com> (03/12/05 1.1498)
>    [libata] fix use-after-free
>
>    Fixes oops some were seeing on module unload.
>
>    Caught by Jon Burgess.

If this is basically an obvious one-liner ("move a kfree")?

Andrew is still off, and he can make a decision independently, but right
now I'm not going to apply anything bigger.

		Linus

[BK PATCHES] libata fixes

[Jeff Garzik]

Linus, please do a

	bk pull bk://gkernel.bkbits.net/libata-2.5

This will update the following files:

 drivers/scsi/libata-core.c  |   17 ++---
 drivers/scsi/sata_promise.c |  128 +++++++++++++++++++++++++-------------------
 2 files changed, 81 insertions(+), 64 deletions(-)

through these ChangeSets:

<jgarzik@redhat.com> (03/12/05 1.1498)
   [libata] fix use-after-free
   
   Fixes oops some were seeing on module unload.
   
   Caught by Jon Burgess.

<grundler@parisc-linux.org> (03/11/30 1.1489.3.2)
   [libata] use sg_dma_xxx macros
   
   Fixes build on some platforms, fixes issues on others.

<jgarzik@redhat.com> (03/11/26 1.1489.3.1)
   [libata] Fix PDC20621: we only have one Host DMA engine, not one per port
   
   Whoops.  So, we need to queue HDMA transactions internally.

diff -Nru a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c	Fri Dec  5 13:14:13 2003
+++ b/drivers/scsi/libata-core.c	Fri Dec  5 13:14:13 2003
@@ -1635,8 +1635,8 @@
 	if (cmd->use_sg)
 		pci_unmap_sg(ap->host_set->pdev, sg, qc->n_elem, dir);
 	else
-		pci_unmap_single(ap->host_set->pdev, sg[0].dma_address,
-				 sg[0].length, dir);
+		pci_unmap_single(ap->host_set->pdev, sg_dma_address(&sg[0]),
+				 sg_dma_len(&sg[0]), dir);
 
 	qc->flags &= ~ATA_QCFLAG_SG;
 	qc->sg = NULL;
@@ -1659,8 +1659,8 @@
 	assert(qc->n_elem > 0);
 
 	for (i = 0; i < qc->n_elem; i++) {
-		ap->prd[i].addr = cpu_to_le32(sg[i].dma_address);
-		ap->prd[i].flags_len = cpu_to_le32(sg[i].length);
+		ap->prd[i].addr = cpu_to_le32(sg_dma_address(&sg[i]));
+		ap->prd[i].flags_len = cpu_to_le32(sg_dma_len(&sg[i]));
 		VPRINTK("PRD[%u] = (0x%X, 0x%X)\n",
 			i, le32_to_cpu(ap->prd[i].addr), le32_to_cpu(ap->prd[i].flags_len));
 	}
@@ -1691,12 +1691,12 @@
 
 	sg->page = virt_to_page(cmd->request_buffer);
 	sg->offset = (unsigned long) cmd->request_buffer & ~PAGE_MASK;
-	sg->length = cmd->request_bufflen;
+	sg_dma_len(sg) = cmd->request_bufflen;
 
 	if (!have_sg)
 		return 0;
 
-	sg->dma_address = pci_map_single(ap->host_set->pdev,
+	sg_dma_address(sg) = pci_map_single(ap->host_set->pdev,
 					 cmd->request_buffer,
 					 cmd->request_bufflen, dir);
 
@@ -1917,7 +1917,7 @@
 	qc->cursg_ofs++;
 
 	if (cmd->use_sg)
-		if ((qc->cursg_ofs * ATA_SECT_SIZE) == sg[qc->cursg].length) {
+		if ((qc->cursg_ofs * ATA_SECT_SIZE) == sg_dma_len(&sg[qc->cursg])) {
 			qc->cursg++;
 			qc->cursg_ofs = 0;
 		}
@@ -3224,8 +3224,6 @@
 		scsi_host_put(ap->host); /* FIXME: check return val */
 	}
 
-	kfree(host_set);
-
 	pci_release_regions(pdev);
 
 	for (i = 0; i < host_set->n_ports; i++) {
@@ -3242,6 +3240,7 @@
 		}
 	}
 
+	kfree(host_set);
 	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 }
diff -Nru a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
--- a/drivers/scsi/sata_promise.c	Fri Dec  5 13:14:13 2003
+++ b/drivers/scsi/sata_promise.c	Fri Dec  5 13:14:13 2003
@@ -34,10 +34,8 @@
 #include <linux/libata.h>
 #include <asm/io.h>
 
-#undef DIRECT_HDMA
-
 #define DRV_NAME	"sata_promise"
-#define DRV_VERSION	"0.86"
+#define DRV_VERSION	"0.87"
 
 
 enum {
@@ -82,6 +80,9 @@
 
 	PDC_FLAG_20621		= (1 << 30), /* we have a 20621 */
 	PDC_HDMA_RESET		= (1 << 11), /* HDMA reset */
+
+	PDC_MAX_HDMA		= 32,
+	PDC_HDMA_Q_MASK		= (PDC_MAX_HDMA - 1),
 };
 
 
@@ -89,6 +90,15 @@
 	u8			dimm_buf[(ATA_PRD_SZ * ATA_MAX_PRD) + 512];
 	u8			*pkt;
 	dma_addr_t		pkt_dma;
+
+	unsigned int		doing_hdma;
+	unsigned int		hdma_prod;
+	unsigned int		hdma_cons;
+	struct {
+		struct ata_queued_cmd *qc;
+		unsigned int	seq;
+		unsigned long	pkt_ofs;
+	} hdma[32];
 };
 
 
@@ -256,6 +266,7 @@
 		rc = -ENOMEM;
 		goto err_out;
 	}
+	memset(pp, 0, sizeof(*pp));
 
 	pp->pkt = pci_alloc_consistent(pdev, 128, &pp->pkt_dma);
 	if (!pp->pkt) {
@@ -605,8 +616,8 @@
 	last = qc->n_elem;
 	idx = 0;
 	for (i = 0; i < last; i++) {
-		buf[idx++] = cpu_to_le32(sg[i].dma_address);
-		buf[idx++] = cpu_to_le32(sg[i].length);
+		buf[idx++] = cpu_to_le32(sg_dma_address(&sg[i]));
+		buf[idx++] = cpu_to_le32(sg_dma_len(&sg[i]));
 		total_len += sg[i].length;
 	}
 	buf[idx - 1] |= cpu_to_le32(ATA_PRD_EOT);
@@ -643,42 +654,60 @@
 	VPRINTK("ata pkt buf ofs %u, prd size %u, mmio copied\n", i, sgt_len);
 }
 
-#ifdef DIRECT_HDMA
-static void pdc20621_push_hdma(struct ata_queued_cmd *qc)
+static void __pdc20621_push_hdma(struct ata_queued_cmd *qc,
+				 unsigned int seq,
+				 u32 pkt_ofs)
 {
 	struct ata_port *ap = qc->ap;
 	struct ata_host_set *host_set = ap->host_set;
-	unsigned int port_no = ap->port_no;
 	void *mmio = host_set->mmio_base;
-	unsigned int rw = (qc->flags & ATA_QCFLAG_WRITE);
-	u32 tmp;
-
-	unsigned int host_sg = PDC_20621_DIMM_BASE +
-			       (PDC_DIMM_WINDOW_STEP * port_no) +
-			       PDC_DIMM_HOST_PRD;
-	unsigned int dimm_sg = PDC_20621_DIMM_BASE +
-			       (PDC_DIMM_WINDOW_STEP * port_no) +
-			       PDC_DIMM_HPKT_PRD;
 
 	/* hard-code chip #0 */
 	mmio += PDC_CHIP0_OFS;
 
-	tmp = readl(mmio + PDC_HDMA_CTLSTAT) & 0xffffff00;
-	tmp |= port_no + 1 + 4;		/* seq. ID */
-	if (!rw)
-		tmp |= (1 << 6);	/* hdma data direction */
-	writel(tmp, mmio + PDC_HDMA_CTLSTAT); /* note: stops DMA, if active */
-	readl(mmio + PDC_HDMA_CTLSTAT);	/* flush */
-
-	writel(host_sg, mmio + 0x108);
-	writel(dimm_sg, mmio + 0x10C);
-	writel(0, mmio + 0x128);
-
-	tmp |= (1 << 7);
-	writel(tmp, mmio + PDC_HDMA_CTLSTAT);
-	readl(mmio + PDC_HDMA_CTLSTAT);	/* flush */
+	writel(0x00000001, mmio + PDC_20621_SEQCTL + (seq * 4));
+	readl(mmio + PDC_20621_SEQCTL + (seq * 4));	/* flush */
+
+	writel(pkt_ofs, mmio + PDC_HDMA_PKT_SUBMIT);
+	readl(mmio + PDC_HDMA_PKT_SUBMIT);	/* flush */
+}
+
+static void pdc20621_push_hdma(struct ata_queued_cmd *qc,
+				unsigned int seq,
+				u32 pkt_ofs)
+{
+	struct ata_port *ap = qc->ap;
+	struct pdc_port_priv *pp = ap->private_data;
+	unsigned int idx = pp->hdma_prod & PDC_HDMA_Q_MASK;
+
+	if (!pp->doing_hdma) {
+		__pdc20621_push_hdma(qc, seq, pkt_ofs);
+		pp->doing_hdma = 1;
+		return;
+	}
+
+	pp->hdma[idx].qc = qc;
+	pp->hdma[idx].seq = seq;
+	pp->hdma[idx].pkt_ofs = pkt_ofs;
+	pp->hdma_prod++;
+}
+
+static void pdc20621_pop_hdma(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct pdc_port_priv *pp = ap->private_data;
+	unsigned int idx = pp->hdma_cons & PDC_HDMA_Q_MASK;
+
+	/* if nothing on queue, we're done */
+	if (pp->hdma_prod == pp->hdma_cons) {
+		pp->doing_hdma = 0;
+		return;
+	}
+
+	__pdc20621_push_hdma(pp->hdma[idx].qc, pp->hdma[idx].seq,
+			     pp->hdma[idx].pkt_ofs);
+	pp->hdma_cons++;
 }
-#endif
 
 #ifdef ATA_VERBOSE_DEBUG
 static void pdc20621_dump_hdma(struct ata_queued_cmd *qc)
@@ -724,23 +753,17 @@
 
 	wmb();			/* flush PRD, pkt writes */
 
-	writel(0x00000001, mmio + PDC_20621_SEQCTL + (seq * 4));
-	readl(mmio + PDC_20621_SEQCTL + (seq * 4));	/* flush */
-
 	if (doing_hdma) {
 		pdc20621_dump_hdma(qc);
-#ifdef DIRECT_HDMA
-		pdc20621_push_hdma(qc);
-#else
-		writel(port_ofs + PDC_DIMM_HOST_PKT,
-		       mmio + PDC_HDMA_PKT_SUBMIT);
-		readl(mmio + PDC_HDMA_PKT_SUBMIT);	/* flush */
-#endif
-		VPRINTK("submitted ofs 0x%x (%u), seq %u\n",
-		port_ofs + PDC_DIMM_HOST_PKT,
-		port_ofs + PDC_DIMM_HOST_PKT,
-		seq);
+		pdc20621_push_hdma(qc, seq, port_ofs + PDC_DIMM_HOST_PKT);
+		VPRINTK("queued ofs 0x%x (%u), seq %u\n",
+			port_ofs + PDC_DIMM_HOST_PKT,
+			port_ofs + PDC_DIMM_HOST_PKT,
+			seq);
 	} else {
+		writel(0x00000001, mmio + PDC_20621_SEQCTL + (seq * 4));
+		readl(mmio + PDC_20621_SEQCTL + (seq * 4));	/* flush */
+
 		writel(port_ofs + PDC_DIMM_ATA_PKT,
 		       (void *) ap->ioaddr.cmd_addr + PDC_PKT_SUBMIT);
 		readl((void *) ap->ioaddr.cmd_addr + PDC_PKT_SUBMIT);
@@ -771,6 +794,7 @@
 			VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			pdc_dma_complete(ap, qc);
+			pdc20621_pop_hdma(qc);
 		}
 
 		/* step one - exec ATA command */
@@ -781,15 +805,8 @@
 
 			/* submit hdma pkt */
 			pdc20621_dump_hdma(qc);
-			writel(0x00000001, mmio + PDC_20621_SEQCTL + (seq * 4));
-			readl(mmio + PDC_20621_SEQCTL + (seq * 4));
-#ifdef DIRECT_HDMA
-			pdc20621_push_hdma(qc);
-#else
-			writel(port_ofs + PDC_DIMM_HOST_PKT,
-			       mmio + PDC_HDMA_PKT_SUBMIT);
-			readl(mmio + PDC_HDMA_PKT_SUBMIT);
-#endif
+			pdc20621_push_hdma(qc, seq,
+					   port_ofs + PDC_DIMM_HOST_PKT);
 		}
 		handled = 1;
 		break;
@@ -814,6 +831,7 @@
 			VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			pdc_dma_complete(ap, qc);
+			pdc20621_pop_hdma(qc);
 		}
 		handled = 1;
 		break;