Re: [PATCH] sata_nv: don't use legacy DMA in ADMA mode (v2)

[Tejun Heo]

Tejun Heo wrote:
> Robert Hancock wrote:
>> We need to run any DMA command with result taskfile requested in ADMA mode
>> when the port is in ADMA mode, otherwise it may try to use the legacy DMA 
>> engine
>> in ADMA mode which is not allowed. Enforce this with BUG_ON() since data
>> corruption could potentially result if this happened. Also WARN_ON() if we 
>> try
>> and send result taskfile commands while NCQ commands are still active, since 
>> the
>> hardware doesn't allow this.
>>
>> Signed-off-by: Robert Hancock <[EMAIL PROTECTED]>
>>
>> @@ -1425,9 +1427,17 @@
>> +/* We can't handle result taskfile with NCQ commands active, since
>> +   retrieving the taskfile switches us out of ADMA mode and would abort
>> +   existing commands. */
>> +WARN_ON((qc->flags & ATA_QCFLAG_RESULT_TF) &&
>> +(qc->ap->qc_allocated & ~(1 << qc->tag)));
> 
> I owe an apology here.  ap->qc_allocated & ~(1 << qc->tag) test isn't
> correct.  Sorry.  qc deferring happens after qc is allocated so the
> condition can trigger (although it should be rare) even when nothing is
> going wrong, so I guess it should be WARN_ON((qc->flags &
> ATA_QCFLAG_RESULT_TF) && link->sactive).

Or, just make the assumption clear by not allowing NCQ w/ RESULT_TF at all.

if (unlikely(qc->tf.protocol == ATA_PROT_NCQ &&
 (qc->flags & ATA_QCFLAG_RESULT_TF)) {
ata_dev_printk(qc->dev, KERN_ERR,
   "NCQ w/ RESULT_TF not allowed\n");
return AC_ERR_SYSTEM;
}

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sata_nv: don't use legacy DMA in ADMA mode (v2)

[Tejun Heo]

Robert Hancock wrote:
> We need to run any DMA command with result taskfile requested in ADMA mode
> when the port is in ADMA mode, otherwise it may try to use the legacy DMA 
> engine
> in ADMA mode which is not allowed. Enforce this with BUG_ON() since data
> corruption could potentially result if this happened. Also WARN_ON() if we try
> and send result taskfile commands while NCQ commands are still active, since 
> the
> hardware doesn't allow this.
> 
> Signed-off-by: Robert Hancock <[EMAIL PROTECTED]>
> 
> @@ -1425,9 +1427,17 @@
> + /* We can't handle result taskfile with NCQ commands active, since
> +retrieving the taskfile switches us out of ADMA mode and would abort
> +existing commands. */
> + WARN_ON((qc->flags & ATA_QCFLAG_RESULT_TF) &&
> + (qc->ap->qc_allocated & ~(1 << qc->tag)));

I owe an apology here.  ap->qc_allocated & ~(1 << qc->tag) test isn't
correct.  Sorry.  qc deferring happens after qc is allocated so the
condition can trigger (although it should be rare) even when nothing is
going wrong, so I guess it should be WARN_ON((qc->flags &
ATA_QCFLAG_RESULT_TF) && link->sactive).

Sorry. :-)

-- 
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sata_nv: don't use legacy DMA in ADMA mode (v2)

[Tejun Heo]

Robert Hancock wrote:
 We need to run any DMA command with result taskfile requested in ADMA mode
 when the port is in ADMA mode, otherwise it may try to use the legacy DMA 
 engine
 in ADMA mode which is not allowed. Enforce this with BUG_ON() since data
 corruption could potentially result if this happened. Also WARN_ON() if we try
 and send result taskfile commands while NCQ commands are still active, since 
 the
 hardware doesn't allow this.
 
 Signed-off-by: Robert Hancock [EMAIL PROTECTED]
 
 @@ -1425,9 +1427,17 @@
 + /* We can't handle result taskfile with NCQ commands active, since
 +retrieving the taskfile switches us out of ADMA mode and would abort
 +existing commands. */
 + WARN_ON((qc-flags  ATA_QCFLAG_RESULT_TF) 
 + (qc-ap-qc_allocated  ~(1  qc-tag)));

I owe an apology here.  ap-qc_allocated  ~(1  qc-tag) test isn't
correct.  Sorry.  qc deferring happens after qc is allocated so the
condition can trigger (although it should be rare) even when nothing is
going wrong, so I guess it should be WARN_ON((qc-flags 
ATA_QCFLAG_RESULT_TF)  link-sactive).

Sorry. :-)

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sata_nv: don't use legacy DMA in ADMA mode (v2)

[Tejun Heo]

Tejun Heo wrote:
 Robert Hancock wrote:
 We need to run any DMA command with result taskfile requested in ADMA mode
 when the port is in ADMA mode, otherwise it may try to use the legacy DMA 
 engine
 in ADMA mode which is not allowed. Enforce this with BUG_ON() since data
 corruption could potentially result if this happened. Also WARN_ON() if we 
 try
 and send result taskfile commands while NCQ commands are still active, since 
 the
 hardware doesn't allow this.

 Signed-off-by: Robert Hancock [EMAIL PROTECTED]

 @@ -1425,9 +1427,17 @@
 +/* We can't handle result taskfile with NCQ commands active, since
 +   retrieving the taskfile switches us out of ADMA mode and would abort
 +   existing commands. */
 +WARN_ON((qc-flags  ATA_QCFLAG_RESULT_TF) 
 +(qc-ap-qc_allocated  ~(1  qc-tag)));
 
 I owe an apology here.  ap-qc_allocated  ~(1  qc-tag) test isn't
 correct.  Sorry.  qc deferring happens after qc is allocated so the
 condition can trigger (although it should be rare) even when nothing is
 going wrong, so I guess it should be WARN_ON((qc-flags 
 ATA_QCFLAG_RESULT_TF)  link-sactive).

Or, just make the assumption clear by not allowing NCQ w/ RESULT_TF at all.

if (unlikely(qc-tf.protocol == ATA_PROT_NCQ 
 (qc-flags  ATA_QCFLAG_RESULT_TF)) {
ata_dev_printk(qc-dev, KERN_ERR,
   NCQ w/ RESULT_TF not allowed\n);
return AC_ERR_SYSTEM;
}

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] sata_nv: don't use legacy DMA in ADMA mode (v2)

[Robert Hancock]

We need to run any DMA command with result taskfile requested in ADMA mode
when the port is in ADMA mode, otherwise it may try to use the legacy DMA engine
in ADMA mode which is not allowed. Enforce this with BUG_ON() since data
corruption could potentially result if this happened. Also WARN_ON() if we try
and send result taskfile commands while NCQ commands are still active, since the
hardware doesn't allow this.

Signed-off-by: Robert Hancock <[EMAIL PROTECTED]>

--- linux-2.6.24-rc3-git1/drivers/ata/sata_nv.c 2007-11-20 17:40:09.0 
-0600
+++ linux-2.6.24-rc3-git1edit/drivers/ata/sata_nv.c 2007-11-22 
19:40:58.0 -0600
@@ -791,11 +791,13 @@
 
 static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
 {
-   /* Since commands where a result TF is requested are not
-  executed in ADMA mode, the only time this function will be called
-  in ADMA mode will be if a command fails. In this case we
-  don't care about going into register mode with ADMA commands
-  pending, as the commands will all shortly be aborted anyway. */
+   /* Other than when internal or pass-through commands are executed,
+  the only time this function will be called in ADMA mode will be
+  if a command fails. In the failure case we don't care about going
+  into register mode with ADMA commands pending, as the commands will
+  all shortly be aborted anyway. We assume that NCQ commands are not
+  issued via passthrough, which is the only way that switching into
+  ADMA mode could abort outstanding commands. */
nv_adma_register_mode(ap);
 
ata_tf_read(ap, tf);
@@ -1359,11 +1361,9 @@
struct nv_adma_port_priv *pp = qc->ap->private_data;
 
/* ADMA engine can only be used for non-ATAPI DMA commands,
-  or interrupt-driven no-data commands, where a result taskfile
-  is not required. */
+  or interrupt-driven no-data commands. */
if ((pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
-  (qc->tf.flags & ATA_TFLAG_POLLING) ||
-  (qc->flags & ATA_QCFLAG_RESULT_TF))
+  (qc->tf.flags & ATA_TFLAG_POLLING))
return 1;
 
if ((qc->flags & ATA_QCFLAG_DMAMAP) ||
@@ -1381,6 +1381,8 @@
   NV_CPB_CTL_IEN;
 
if (nv_adma_use_reg_mode(qc)) {
+   BUG_ON(!(pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) &&
+   (qc->flags & ATA_QCFLAG_DMAMAP));
nv_adma_register_mode(qc->ap);
ata_qc_prep(qc);
return;
@@ -1425,9 +1427,17 @@
 
VPRINTK("ENTER\n");
 
+   /* We can't handle result taskfile with NCQ commands active, since
+  retrieving the taskfile switches us out of ADMA mode and would abort
+  existing commands. */
+   WARN_ON((qc->flags & ATA_QCFLAG_RESULT_TF) &&
+   (qc->ap->qc_allocated & ~(1 << qc->tag)));
+
if (nv_adma_use_reg_mode(qc)) {
/* use ATA register mode */
VPRINTK("using ATA register mode: 0x%lx\n", qc->flags);
+   BUG_ON(!(pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) &&
+   (qc->flags & ATA_QCFLAG_DMAMAP));
nv_adma_register_mode(qc->ap);
return ata_qc_issue_prot(qc);
} else

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] sata_nv: don't use legacy DMA in ADMA mode (v2)

[Robert Hancock]

We need to run any DMA command with result taskfile requested in ADMA mode
when the port is in ADMA mode, otherwise it may try to use the legacy DMA engine
in ADMA mode which is not allowed. Enforce this with BUG_ON() since data
corruption could potentially result if this happened. Also WARN_ON() if we try
and send result taskfile commands while NCQ commands are still active, since the
hardware doesn't allow this.

Signed-off-by: Robert Hancock [EMAIL PROTECTED]

--- linux-2.6.24-rc3-git1/drivers/ata/sata_nv.c 2007-11-20 17:40:09.0 
-0600
+++ linux-2.6.24-rc3-git1edit/drivers/ata/sata_nv.c 2007-11-22 
19:40:58.0 -0600
@@ -791,11 +791,13 @@
 
 static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
 {
-   /* Since commands where a result TF is requested are not
-  executed in ADMA mode, the only time this function will be called
-  in ADMA mode will be if a command fails. In this case we
-  don't care about going into register mode with ADMA commands
-  pending, as the commands will all shortly be aborted anyway. */
+   /* Other than when internal or pass-through commands are executed,
+  the only time this function will be called in ADMA mode will be
+  if a command fails. In the failure case we don't care about going
+  into register mode with ADMA commands pending, as the commands will
+  all shortly be aborted anyway. We assume that NCQ commands are not
+  issued via passthrough, which is the only way that switching into
+  ADMA mode could abort outstanding commands. */
nv_adma_register_mode(ap);
 
ata_tf_read(ap, tf);
@@ -1359,11 +1361,9 @@
struct nv_adma_port_priv *pp = qc-ap-private_data;
 
/* ADMA engine can only be used for non-ATAPI DMA commands,
-  or interrupt-driven no-data commands, where a result taskfile
-  is not required. */
+  or interrupt-driven no-data commands. */
if ((pp-flags  NV_ADMA_ATAPI_SETUP_COMPLETE) ||
-  (qc-tf.flags  ATA_TFLAG_POLLING) ||
-  (qc-flags  ATA_QCFLAG_RESULT_TF))
+  (qc-tf.flags  ATA_TFLAG_POLLING))
return 1;
 
if ((qc-flags  ATA_QCFLAG_DMAMAP) ||
@@ -1381,6 +1381,8 @@
   NV_CPB_CTL_IEN;
 
if (nv_adma_use_reg_mode(qc)) {
+   BUG_ON(!(pp-flags  NV_ADMA_ATAPI_SETUP_COMPLETE) 
+   (qc-flags  ATA_QCFLAG_DMAMAP));
nv_adma_register_mode(qc-ap);
ata_qc_prep(qc);
return;
@@ -1425,9 +1427,17 @@
 
VPRINTK(ENTER\n);
 
+   /* We can't handle result taskfile with NCQ commands active, since
+  retrieving the taskfile switches us out of ADMA mode and would abort
+  existing commands. */
+   WARN_ON((qc-flags  ATA_QCFLAG_RESULT_TF) 
+   (qc-ap-qc_allocated  ~(1  qc-tag)));
+
if (nv_adma_use_reg_mode(qc)) {
/* use ATA register mode */
VPRINTK(using ATA register mode: 0x%lx\n, qc-flags);
+   BUG_ON(!(pp-flags  NV_ADMA_ATAPI_SETUP_COMPLETE) 
+   (qc-flags  ATA_QCFLAG_DMAMAP));
nv_adma_register_mode(qc-ap);
return ata_qc_issue_prot(qc);
} else

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/