sata_mv: fix broken DSM/TRIM support

PATCH New Mark Lord 2 participants 11 messages Aug 20, 2010 – Aug 23, 2010 libata

Share & tools

Mark Lord

author

2010-08-20 01:19 UTC #0 lore raw

PATCH 1/1 sata_mv: fix broken DSM/TRIM support

Fix DSM/TRIM commands in sata_mv.
These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.

And since the chips don't have proper BM DMA status, we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".

Signed-off-by Mark Lord <[email protected]>

---

(my apologies for broken mailers -- patch also attached in case inline is b0rked).

drivers/ata/sata_mv.c renamed from drivers/ata/sata_mv.c.orig +30 -7

@@ -1886,19 +1886,25 @@
  *	LOCKING:
  *	Inherited from caller.
  */
-static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+static void mv_bmdma_stop_ap(struct ata_port *ap)
 {
-	struct ata_port *ap = qc->ap;
 	void __iomem *port_mmio = mv_ap_base(ap);
 	u32 cmd;
 
 	/* clear start/stop bit */
 	cmd = readl(port_mmio + BMDMA_CMD);
-	cmd &= ~ATA_DMA_START;
-	writelfl(cmd, port_mmio + BMDMA_CMD);
+	if (cmd & ATA_DMA_START) {
+		cmd &= ~ATA_DMA_START;
+		writelfl(cmd, port_mmio + BMDMA_CMD);
+
+		/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
+		ata_sff_dma_pause(ap);
+	}
+}
 
-	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_dma_pause(ap);
+static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+{
+	mv_bmdma_stop_ap(qc->ap);
 }
 
 /**

@@ -1922,8 +1928,21 @@
 	reg = readl(port_mmio + BMDMA_STATUS);
 	if (reg & ATA_DMA_ACTIVE)
 		status = ATA_DMA_ACTIVE;
-	else
+	else if (reg & ATA_DMA_ERR)
 		status = (reg & ATA_DMA_ERR) | ATA_DMA_INTR;
+	else {
+		/*
+		 * Just because DMA_ACTIVE is 0 (DMA completed),
+		 * this does _not_ mean the device is "done".
+		 * So we should not yet be signalling ATA_DMA_INTR
+		 * in some cases.  Eg. DSM/TRIM, and perhaps others.
+		 */
+		mv_bmdma_stop_ap(ap);
+		if (ioread8(ap->ioaddr.altstatus_addr) & ATA_BUSY)
+			status = 0;
+		else
+			status = ATA_DMA_INTR;
+	}
 	return status;
 }

@@ -2082,6 +2101,8 @@
 	if ((tf->protocol != ATA_PROT_DMA) &&
 	    (tf->protocol != ATA_PROT_NCQ))
 		return;
+	if (tf->command == ATA_CMD_DSM)
+		return;  /* use bmdma for this */
 
 	/* Fill in Gen IIE command request block */
 	if (!(tf->flags & ATA_TFLAG_WRITE))

@@ -2277,6 +2298,8 @@
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
+		if (qc->tf.command == ATA_CMD_DSM)
+			break;  /* use bmdma for this */
 	case ATA_PROT_NCQ:
 		mv_start_edma(ap, port_mmio, pp, qc->tf.protocol);
 		pp->req_idx = (pp->req_idx + 1) & MV_MAX_Q_DEPTH_MASK;

Fix DSM/TRIM commands in sata_mv.
These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/sata_mv.c renamed from drivers/ata/sata_mv.c.orig +30 -7

@@ -1886,19 +1886,25 @@
  *	LOCKING:
  *	Inherited from caller.
  */
-static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+static void mv_bmdma_stop_ap(struct ata_port *ap)
 {
-	struct ata_port *ap = qc->ap;
 	void __iomem *port_mmio = mv_ap_base(ap);
 	u32 cmd;
 
 	/* clear start/stop bit */
 	cmd = readl(port_mmio + BMDMA_CMD);
-	cmd &= ~ATA_DMA_START;
-	writelfl(cmd, port_mmio + BMDMA_CMD);
+	if (cmd & ATA_DMA_START) {
+		cmd &= ~ATA_DMA_START;
+		writelfl(cmd, port_mmio + BMDMA_CMD);
+
+		/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
+		ata_sff_dma_pause(ap);
+	}
+}
 
-	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_dma_pause(ap);
+static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+{
+	mv_bmdma_stop_ap(qc->ap);
 }
 
 /**

@@ -1922,8 +1928,21 @@
 	reg = readl(port_mmio + BMDMA_STATUS);
 	if (reg & ATA_DMA_ACTIVE)
 		status = ATA_DMA_ACTIVE;
-	else
+	else if (reg & ATA_DMA_ERR)
 		status = (reg & ATA_DMA_ERR) | ATA_DMA_INTR;
+	else {
+		/*
+		 * Just because DMA_ACTIVE is 0 (DMA completed),
+		 * this does _not_ mean the device is "done".
+		 * So we should not yet be signalling ATA_DMA_INTR
+		 * in some cases.  Eg. DSM/TRIM, and perhaps others.
+		 */
+		mv_bmdma_stop_ap(ap);
+		if (ioread8(ap->ioaddr.altstatus_addr) & ATA_BUSY)
+			status = 0;
+		else
+			status = ATA_DMA_INTR;
+	}
 	return status;
 }

@@ -2082,6 +2101,8 @@
 	if ((tf->protocol != ATA_PROT_DMA) &&
 	    (tf->protocol != ATA_PROT_NCQ))
 		return;
+	if (tf->command == ATA_CMD_DSM)
+		return;  /* use bmdma for this */
 
 	/* Fill in Gen IIE command request block */
 	if (!(tf->flags & ATA_TFLAG_WRITE))

@@ -2277,6 +2298,8 @@
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
+		if (qc->tf.command == ATA_CMD_DSM)
+			break;  /* use bmdma for this */
 	case ATA_PROT_NCQ:
 		mv_start_edma(ap, port_mmio, pp, qc->tf.protocol);
 		pp->req_idx = (pp->req_idx + 1) & MV_MAX_Q_DEPTH_MASK;

Mark Lord

author

2010-08-20 01:40 UTC #1 lore raw

PATCH 1/1 sata_mv: fix broken DSM/TRIM support (v2)

Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.

Since the chips don't have proper BM DMA status,
we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".

GEN_I chips don't have BM DMA, so no TRIM for them.

Signed-off-by Mark Lord <[email protected]>

---

Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. (my apologies for broken mailers -- patch also attached in case inline is b0rked).

drivers/ata/sata_mv.c +37 -7

@@ -1898,19 +1898,25 @@
  *	LOCKING:
  *	Inherited from caller.
  */
-static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+static void mv_bmdma_stop_ap(struct ata_port *ap)
 {
-	struct ata_port *ap = qc->ap;
 	void __iomem *port_mmio = mv_ap_base(ap);
 	u32 cmd;
 
 	/* clear start/stop bit */
 	cmd = readl(port_mmio + BMDMA_CMD);
-	cmd &= ~ATA_DMA_START;
-	writelfl(cmd, port_mmio + BMDMA_CMD);
+	if (cmd & ATA_DMA_START) {
+		cmd &= ~ATA_DMA_START;
+		writelfl(cmd, port_mmio + BMDMA_CMD);
 
-	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_dma_pause(ap);
+		/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
+		ata_sff_dma_pause(ap);
+	}
+}
+
+static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+{
+	mv_bmdma_stop_ap(qc->ap);
 }
 
 /**

@@ -1934,8 +1940,21 @@
 	reg = readl(port_mmio + BMDMA_STATUS);
 	if (reg & ATA_DMA_ACTIVE)
 		status = ATA_DMA_ACTIVE;
-	else
+	else if (reg & ATA_DMA_ERR)
 		status = (reg & ATA_DMA_ERR) | ATA_DMA_INTR;
+	else {
+		/*
+		 * Just because DMA_ACTIVE is 0 (DMA completed),
+		 * this does _not_ mean the device is "done".
+		 * So we should not yet be signalling ATA_DMA_INTR
+		 * in some cases.  Eg. DSM/TRIM, and perhaps others.
+		 */
+		mv_bmdma_stop_ap(ap);
+		if (ioread8(ap->ioaddr.altstatus_addr) & ATA_BUSY)
+			status = 0;
+		else
+			status = ATA_DMA_INTR;
+	}
 	return status;
 }

@@ -1995,6 +2014,9 @@
 
 	switch (tf->protocol) {
 	case ATA_PROT_DMA:
+		if (tf->command == ATA_CMD_DSM)
+			return;
+		/* fall-thru */
 	case ATA_PROT_NCQ:
 		break;	/* continue below */
 	case ATA_PROT_PIO:

@@ -2094,6 +2116,8 @@
 	if ((tf->protocol != ATA_PROT_DMA) &&
 	    (tf->protocol != ATA_PROT_NCQ))
 		return;
+	if (tf->command == ATA_CMD_DSM)
+		return;  /* use bmdma for this */
 
 	/* Fill in Gen IIE command request block */
 	if (!(tf->flags & ATA_TFLAG_WRITE))

@@ -2289,6 +2313,12 @@
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
+		if (qc->tf.command == ATA_CMD_DSM) {
+			if (!ap->ops->bmdma_setup)  /* no bmdma on GEN_I */
+				return AC_ERR_OTHER;
+			break;  /* use bmdma for this */
+		}
+		/* fall thru */
 	case ATA_PROT_NCQ:
 		mv_start_edma(ap, port_mmio, pp, qc->tf.protocol);
 		pp->req_idx = (pp->req_idx + 1) & MV_MAX_Q_DEPTH_MASK;

Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.

Since the chips don't have proper BM DMA status,
we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".

GEN_I chips don't have BM DMA, so no TRIM for them.

Signed-off-by Mark Lord <[email protected]>

---

Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. (my apologies for broken mailers -- patch also attached in case inline is b0rked).

drivers/ata/sata_mv.c +37 -7

82 lines not on this page · raw message

Mark Lord

author

2010-08-20 13:56 UTC #2 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

Quoted 8 lines · Mark Lord

Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.
Since the chips don't have proper BM DMA status,
we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".

Speaking of which.. it appears that BM DMA support for sata_mv is intentionally broken by 2.6.35 kernels, with the addition of this code in libata-sff.c:

unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)

{
        struct ata_port *ap = qc->ap;

        /* see ata_dma_blacklisted() */
        BUG_ON((ap->flags & ATA_FLAG_PIO_POLLING) &&
               qc->tf.protocol == ATAPI_PROT_DMA);

Is that BUG_ON even necessary??
It really should be looking only at tf.flags, not qp->flags there, as it actually does redundantly further on:

   WARN_ON_ONCE(qc->tf.flags & ATA_TFLAG_POLLING);

sata_mv has ATA_FLAG_PIO_POLLING set for all ports, but doesn't set ATA_TFLAG_POLLING when doing BM DMA.

???

Mark Lord

author

2010-08-20 14:13 UTC #3 lore raw

PATCH 1/1 libata-sff: remove harmful BUG_ON from ata_bmdma_qc_issue

Remove harmful BUG_ON() from ata_bmdma_qc_issue(), as it casts too wide of a net and breaks sata_mv. It also crashes the kernel while doing the BUG_ON().

There's already a WARN_ON_ONCE() further down to catch the case of POLLING for a BMDMA operation.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-sff.c +0 -4

@@ -2735,10 +2735,6 @@
 {
 	struct ata_port *ap = qc->ap;
 
-	/* see ata_dma_blacklisted() */
-	BUG_ON((ap->flags & ATA_FLAG_PIO_POLLING) &&
-	       qc->tf.protocol == ATAPI_PROT_DMA);
-
 	/* defer PIO handling to sff_qc_issue */
 	if (!ata_is_dma(qc->tf.protocol))
 		return ata_sff_qc_issue(qc);

Remove harmful BUG_ON() from ata_bmdma_qc_issue(), as it casts too wide of a net and breaks sata_mv. It also crashes the kernel while doing the BUG_ON().

There's already a WARN_ON_ONCE() further down to catch the case of POLLING for a BMDMA operation.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-sff.c +0 -4

@@ -2735,10 +2735,6 @@
 {
 	struct ata_port *ap = qc->ap;
 
-	/* see ata_dma_blacklisted() */
-	BUG_ON((ap->flags & ATA_FLAG_PIO_POLLING) &&
-	       qc->tf.protocol == ATAPI_PROT_DMA);
-
 	/* defer PIO handling to sff_qc_issue */
 	if (!ata_is_dma(qc->tf.protocol))
 		return ata_sff_qc_issue(qc);

Tejun Heo

Acked-by on v1 1/1

2010-08-23 08:22 UTC #4 lore raw

Re: [PATCH] libata-sff: remove harmful BUG_ON from ata_bmdma_qc_issue

Quoted 8 lines · Mark Lord

Remove harmful BUG_ON() from ata_bmdma_qc_issue(), as it casts too wide of a net and breaks sata_mv. It also crashes the kernel while doing the BUG_ON().
There's already a WARN_ON_ONCE() further down to catch the case of POLLING for a BMDMA operation.
Signed-off-by Mark Lord <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Tejun Heo

2010-08-23 08:27 UTC #5 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

In reply to Mark Lord

Quoted 34 lines · Mark Lord

Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.
Since the chips don't have proper BM DMA status,
we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".
GEN_I chips don't have BM DMA, so no TRIM for them.
Signed-off-by Mark Lord <[email protected]>
---
Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. (my apologies for broken mailers -- patch also attached in case inline is b0rked).
drivers/ata/sata_mv.c +1 -4
@@ -1898,19 +1898,25 @@
  *	LOCKING:
  *	Inherited from caller.
  */
-static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+static void mv_bmdma_stop_ap(struct ata_port *ap)
 {
-	struct ata_port *ap = qc->ap;
 	void __iomem *port_mmio = mv_ap_base(ap);
 	u32 cmd;
 
 	/* clear start/stop bit */
 	cmd = readl(port_mmio + BMDMA_CMD);
-	cmd &= ~ATA_DMA_START;
-	writelfl(cmd, port_mmio + BMDMA_CMD);

drivers/ata/sata_mv.c line 1915

Quoted 10 lines

drivers/ata/sata_mv.c +7 -2

+	if (cmd & ATA_DMA_START) {
+		cmd &= ~ATA_DMA_START;
+		writelfl(cmd, port_mmio + BMDMA_CMD);
 
-	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_dma_pause(ap);
+		/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
+		ata_sff_dma_pause(ap);
+	}
+}

Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches?

Thanks.

Signature · 1 line

tejun

Mark Lord

author

2010-08-23 13:41 UTC #6 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

Quoted 13 lines · Tejun Heo

Quoted 12 lines · Mark Lord
Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.
Since the chips don't have proper BM DMA status,
we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".
GEN_I chips don't have BM DMA, so no TRIM for them.
Signed-off-by Mark Lord <[email protected]>

Quoted 2 lines

Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches?

TRIM is the first/only command which actually breaks without this. It is possible that other commands may benefit as well on some DVD-RW drives, but I have not encountered those yet.

So for now, I've left it as a single patch, with all of the (small) pieces together for TRIM.

Cheers

Tejun Heo

Acked-by

2010-08-23 13:46 UTC #7 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

Quoted 11 lines · Mark Lord

Quoted 2 lines
Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches?
TRIM is the first/only command which actually breaks without this. It is possible that other commands may benefit as well on some DVD-RW drives, but I have not encountered those yet.
So for now, I've left it as a single patch, with all of the (small) pieces together for TRIM.

I see. Looks good to me then.

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Mark Lord

author

2010-08-23 13:54 UTC #8 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

Quoted 10 lines · Tejun Heo

Quoted 9 lines · Mark Lord
Quoted 2 lines
Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches?
TRIM is the first/only command which actually breaks without this. It is possible that other commands may benefit as well on some DVD-RW drives, but I have not encountered those yet.

Quoted 6 lines

Quoted 2 lines
So for now, I've left it as a single patch, with all of the (small) pieces together for TRIM.
I see. Looks good to me then.
Acked-by Tejun Heo<[email protected]>

Peachy. I do imagine that both of these patches
might also be prime candidates for -stable,
if/when they ever make it upstream.

Cheers

Tejun Heo

2010-08-23 13:58 UTC #9 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

Quoted 14 lines · Mark Lord

Quoted 13 lines · Tejun Heo
Quoted 11 lines · Mark Lord
Quoted 2 lines
Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches?
TRIM is the first/only command which actually breaks without this. It is possible that other commands may benefit as well on some DVD-RW drives, but I have not encountered those yet.
So for now, I've left it as a single patch, with all of the (small) pieces together for TRIM.

Quoted 8 lines

Quoted 3 lines
I see. Looks good to me then.
Acked-by Tejun Heo<[email protected]>
..
Peachy. I do imagine that both of these patches
might also be prime candidates for -stable,
if/when they ever make it upstream.

Just add Cc: [email protected] below SOB's. Greg will be automatically notified when these patches hit mainline.

Thanks.

Signature · 1 line

tejun

Mark Lord

author

2010-08-23 13:50 UTC #10 lore raw

Re: [PATCH] sata_mv: fix broken DSM/TRIM support (v2)

In reply to Tejun Heo

Quoted 50 lines · Mark Lord

Quoted 34 lines
Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue.
Since the chips don't have proper BM DMA status,
we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete".
GEN_I chips don't have BM DMA, so no TRIM for them.
Signed-off-by Mark Lord <[email protected]>
---
Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. (my apologies for broken mailers -- patch also attached in case inline is b0rked).
drivers/ata/sata_mv.c 2010-08-19 21:34:46.139766319 -0400 renamed from drivers/ata/sata_mv.c 2010-08-01 18:11:14.000000000 -0400 +1 -4
@@ -1898,19 +1898,25 @@
  *   LOCKING:
  *   Inherited from caller.
  */
-static void mv_bmdma_stop(struct ata_queued_cmd *qc)
+static void mv_bmdma_stop_ap(struct ata_port *ap)
 {
-     struct ata_port *ap = qc->ap;
      void __iomem *port_mmio = mv_ap_base(ap);
      u32 cmd;
 
      /* clear start/stop bit */
      cmd = readl(port_mmio + BMDMA_CMD);
-     cmd &= ~ATA_DMA_START;
-     writelfl(cmd, port_mmio + BMDMA_CMD);
drivers/ata/sata_mv.c 2010-08-19 21:34:46.139766319 -0400 line 1915
Quoted 10 lines
drivers/ata/sata_mv.c 2010-08-19 21:34:46.139766319 -0400 renamed from drivers/ata/sata_mv.c 2010-08-01 18:11:14.000000000 -0400 +7 -2
+     if (cmd & ATA_DMA_START) {
+             cmd &= ~ATA_DMA_START;
+             writelfl(cmd, port_mmio + BMDMA_CMD);
 
-     /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-     ata_sff_dma_pause(ap);
+             /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
+             ata_sff_dma_pause(ap);
+     }
+}
Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches?
Signature · 1 line
tejun

Quoted 7 lines · mlord

TRIM is the first/only command which actually breaks without this. It is possible that other commands may benefit as well on some DVD-RW drives, but I have not encountered those yet.
So for now, I've left it as a single patch,
with all of the (small) pieces together for TRIM.

Oh, waitasec.. you were commenting on just that one scrap of code above. Here, there's no change to normal operation. It still gets called, and still runs. The change above just removes unnecessary register writes if/when the code gets called multiple times, which can now happen from the status polling further down.

Tested and working for TRIM and ATAPI (the only two users) on both the PCI (6081) and PCIe (7042) variants, and also confirmed by Nils on his Kirkwood SOC variant.

Cheers