[PATCH] sata_mv: fix broken DSM/TRIM support

[PATCH] sata_mv: fix broken DSM/TRIM support

[Mark Lord]

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

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 <mlord@pobox.com>
---

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

--- linux/drivers/ata/sata_mv.c.orig	2010-08-02 13:30:51.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2010-08-19 20:57:26.785033240 -0400
@@ -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;


[-- Attachment #2: 01_sata_mv_dsm_trim_fix.patch --]
[-- Type: text/x-patch, Size: 2449 bytes --]

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 <mlord@pobox.com>

--- linux/drivers/ata/sata_mv.c.orig	2010-08-02 13:30:51.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2010-08-19 20:57:26.785033240 -0400
@@ -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;

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

[Mark Lord]

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

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 <mlord@pobox.com>
---

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

--- 2.6.35.2/drivers/ata/sata_mv.c	2010-08-01 18:11:14.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2010-08-19 21:34:46.139766319 -0400
@@ -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;


[-- Attachment #2: 01_sata_mv_dsm_trim_fix.patch --]
[-- Type: text/x-patch, Size: 2978 bytes --]

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 <mlord@pobox.com>
---

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

--- 2.6.35.2/drivers/ata/sata_mv.c	2010-08-01 18:11:14.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2010-08-19 21:34:46.139766319 -0400
@@ -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;

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

[Mark Lord]

On 10-08-19 09:40 PM, Mark Lord wrote:
> 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.

???

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

[Mark Lord]

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

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 <mlord@pobox.com>

--- 2.6.35.3-rc1/drivers/ata/libata-sff.c	2010-08-01 18:11:14.000000000 -0400
+++ linux/drivers/ata/libata-sff.c	2010-08-20 10:06:36.531914889 -0400
@@ -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);


[-- Attachment #2: 02_libata_sff_remove_bug.patch --]
[-- Type: text/x-patch, Size: 746 bytes --]

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 <mlord@pobox.com>

--- 2.6.35.3-rc1/drivers/ata/libata-sff.c	2010-08-01 18:11:14.000000000 -0400
+++ linux/drivers/ata/libata-sff.c	2010-08-20 10:06:36.531914889 -0400
@@ -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);

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

[Tejun Heo]

On 08/20/2010 04:13 PM, Mark Lord wrote:
> 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 <mlord@pobox.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Tejun Heo]

On 08/20/2010 03:40 AM, Mark Lord wrote:
> 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 <mlord@pobox.com>
> ---
> 
> 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).
> 
> --- 2.6.35.2/drivers/ata/sata_mv.c	2010-08-01 18:11:14.000000000 -0400
> +++ linux/drivers/ata/sata_mv.c	2010-08-19 21:34:46.139766319 -0400
> @@ -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);
> +	}
> +}

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

Thanks.

-- 
tejun

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

[Mark Lord]

On Mon, 2010-08-23 at 10:22 +0200, Tejun Heo wrote:
> On 08/20/2010 03:40 AM, Mark Lord wrote:
> > 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 <mlord@pobox.com>
..


> 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

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

[Tejun Heo]

On 08/23/2010 03:41 PM, Mark Lord wrote:
>> 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 <tj@kernel.org>

Thanks.

-- 
tejun

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

[Mark Lord]

On 10-08-23 09:47 AM, Tejun Heo wrote:
> On 08/23/2010 03:41 PM, Mark Lord wrote:
>>> 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<tj@kernel.org>
..

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

Cheers

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

[Tejun Heo]

On 08/23/2010 03:54 PM, Mark Lord wrote:
> On 10-08-23 09:47 AM, Tejun Heo wrote:
>> On 08/23/2010 03:41 PM, Mark Lord wrote:
>>>> 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<tj@kernel.org>
> ..
> 
> 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: stable@kernel.org below SOB's.  Greg will be
automatically notified when these patches hit mainline.

Thanks.

-- 
tejun

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

[Mark Lord]

>On 08/20/2010 03:40 AM, Mark Lord wrote:
>> 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 <mlord@pobox.com>
>> ---
>> 
>> 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).
>> 
>> --- 2.6.35.2/drivers/ata/sata_mv.c    2010-08-01 18:11:14.000000000 -0400
>> +++ linux/drivers/ata/sata_mv.c       2010-08-19 21:34:46.139766319 -0400
>> @@ -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);
>> +     }
>> +}
>
>Don't we need this for other BMDMA commands too?  If so, maybe it's
>better to make it two patches?
>-- 
>tejun

mlord wrote:
> 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