[patch 09/15] libata: Improved ata_exec_internal timeout handling

[patch 09/15] libata: Improved ata_exec_internal timeout handling

[brking]

Currently ata_exec_internal does very minimal cleanup if
the executing command times out. This works for most usage
scenarios, but can cause problems for hosts that set
ATA_FLAG_PIO_DMA. For these users, the DMA buffer gets unmapped
while the timed out out command is still active, which has
the potential to cause PCI DMA errors if the command is
still in progress. The following patch modifies ata_exec_internal
to invoke eng_timeout on a timeout to allow for the timed out
command to be cleaned up better before proceeding further.
This patch is also in preparation for SAS attached SATA devices.

Signed-off-by: Brian King <brking@us.ibm.com>
---

 drivers/scsi/ahci.c         |    5 +----
 drivers/scsi/libata-core.c  |   21 ++++-----------------
 drivers/scsi/sata_mv.c      |   10 +++-------
 drivers/scsi/sata_promise.c |    5 +----
 drivers/scsi/sata_sil24.c   |    5 +----
 5 files changed, 10 insertions(+), 36 deletions(-)

diff -puN drivers/scsi/libata-core.c~libata_exec_internal_timeout drivers/scsi/libata-core.c
--- libata-dev/drivers/scsi/libata-core.c~libata_exec_internal_timeout	2006-02-03 12:38:14.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/libata-core.c	2006-02-03 12:41:07.000000000 -0600
@@ -1108,9 +1108,9 @@ void ata_qc_complete_internal(struct ata
  *
  *	Executes libata internal command with timeout.  @tf contains
  *	command on entry and result on return.  Timeout and error
- *	conditions are reported via return value.  No recovery action
- *	is taken after a command times out.  It's caller's duty to
- *	clean up after timeout.
+ *	conditions are reported via return value.  If command
+ *	times out, the timeout will be reported via eng_timeout to
+ *	allow for proper cleanup so the qc can be freed.
  *
  *	LOCKING:
  *	None.  Should be called with kernel context, might sleep.
@@ -1150,18 +1150,11 @@ ata_exec_internal(struct ata_port *ap, s
 
 	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
 		spin_lock_irqsave(ap->lock, flags);
-
-		/* We're racing with irq here.  If we lose, the
-		 * following test prevents us from completing the qc
-		 * again.  If completion irq occurs after here but
-		 * before the caller cleans up, it will result in a
-		 * spurious interrupt.  We can live with that.
-		 */
 		if (qc->flags & ATA_QCFLAG_ACTIVE) {
 			qc->err_mask = AC_ERR_TIMEOUT;
-			ata_qc_complete(qc);
 			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
 			       ap->id, command);
+			ap->ops->eng_timeout(ap);
 		}
 
 		spin_unlock_irqrestore(ap->lock, flags);
@@ -3846,13 +3839,7 @@ void ata_eng_timeout(struct ata_port *ap
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	if (qc)
 		ata_qc_timeout(qc);
-	else {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-		goto out;
-	}
 
-out:
 	DPRINTK("EXIT\n");
 }
 
diff -puN drivers/scsi/ahci.c~libata_exec_internal_timeout drivers/scsi/ahci.c
--- libata-dev/drivers/scsi/ahci.c~libata_exec_internal_timeout	2006-02-03 12:38:14.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/ahci.c	2006-02-03 12:38:14.000000000 -0600
@@ -677,10 +677,7 @@ static void ahci_eng_timeout(struct ata_
 	spin_lock_irqsave(&host_set->lock, flags);
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-	} else {
+	if (qc) {
 		ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
 		qc->err_mask |= AC_ERR_TIMEOUT;
 	}
diff -puN drivers/scsi/sata_mv.c~libata_exec_internal_timeout drivers/scsi/sata_mv.c
--- libata-dev/drivers/scsi/sata_mv.c~libata_exec_internal_timeout	2006-02-03 12:38:14.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/sata_mv.c	2006-02-03 12:38:15.000000000 -0600
@@ -2019,17 +2019,13 @@ static void mv_eng_timeout(struct ata_po
 			 to_pci_dev(ap->host_set->dev));
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-        printk(KERN_ERR "mmio_base %p ap %p qc %p scsi_cmnd %p &cmnd %p\n",
-	       ap->host_set->mmio_base, ap, qc, qc->scsicmd,
-	       &qc->scsicmd->cmnd);
+        printk(KERN_ERR "mmio_base %p ap %p qc %p scsi_cmnd %p\n",
+	       ap->host_set->mmio_base, ap, qc, qc->scsicmd);
 
 	mv_err_intr(ap);
 	mv_stop_and_reset(ap);
 
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-	} else {
+	if (qc) {
 		qc->err_mask |= AC_ERR_TIMEOUT;
 		ata_eh_qc_complete(qc);
 	}
diff -puN drivers/scsi/sata_promise.c~libata_exec_internal_timeout drivers/scsi/sata_promise.c
--- libata-dev/drivers/scsi/sata_promise.c~libata_exec_internal_timeout	2006-02-03 12:38:14.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/sata_promise.c	2006-02-03 12:38:14.000000000 -0600
@@ -431,11 +431,8 @@ static void pdc_eng_timeout(struct ata_p
 	spin_lock_irqsave(&host_set->lock, flags);
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
+	if (!qc)
 		goto out;
-	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
diff -puN drivers/scsi/sata_sil24.c~libata_exec_internal_timeout drivers/scsi/sata_sil24.c
--- libata-dev/drivers/scsi/sata_sil24.c~libata_exec_internal_timeout	2006-02-03 12:38:14.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/sata_sil24.c	2006-02-03 12:38:14.000000000 -0600
@@ -638,11 +638,8 @@ static void sil24_eng_timeout(struct ata
 	struct ata_queued_cmd *qc;
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
+	if (!qc)
 		return;
-	}
 
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->err_mask |= AC_ERR_TIMEOUT;
_

Re: [patch 09/15] libata: Improved ata_exec_internal timeout handling

[Tejun Heo]

brking@us.ibm.com wrote:
> Currently ata_exec_internal does very minimal cleanup if
> the executing command times out. This works for most usage
> scenarios, but can cause problems for hosts that set
> ATA_FLAG_PIO_DMA. For these users, the DMA buffer gets unmapped
> while the timed out out command is still active, which has
> the potential to cause PCI DMA errors if the command is
> still in progress. The following patch modifies ata_exec_internal
> to invoke eng_timeout on a timeout to allow for the timed out
> command to be cleaned up better before proceeding further.
> This patch is also in preparation for SAS attached SATA devices.
> 

Hello, again.

ata_exec_internal() will be used during EH, so if you call error handler
from ata_exec_internal()....  In my pending EH patchset, there's a patch
to implement ->post_internal() callback which cleans up after an
internal command (successful or not).  Hopefully, it will be merged in
not too distant future.  Can ata_exec_internal() timeout handling wait
till then?

-- 
tejun

Re: [patch 09/15] libata: Improved ata_exec_internal timeout handling

[Brian King]

Tejun Heo wrote:
> brking@us.ibm.com wrote:
>> Currently ata_exec_internal does very minimal cleanup if
>> the executing command times out. This works for most usage
>> scenarios, but can cause problems for hosts that set
>> ATA_FLAG_PIO_DMA. For these users, the DMA buffer gets unmapped
>> while the timed out out command is still active, which has
>> the potential to cause PCI DMA errors if the command is
>> still in progress. The following patch modifies ata_exec_internal
>> to invoke eng_timeout on a timeout to allow for the timed out
>> command to be cleaned up better before proceeding further.
>> This patch is also in preparation for SAS attached SATA devices.
>>
> 
> Hello, again.
> 
> ata_exec_internal() will be used during EH, so if you call error handler
> from ata_exec_internal()....  In my pending EH patchset, there's a patch
> to implement ->post_internal() callback which cleans up after an
> internal command (successful or not).  Hopefully, it will be merged in
> not too distant future.  Can ata_exec_internal() timeout handling wait
> till then?

Do you have a pointer to this patchset? I did a quick search for
post_internal on the archive and didn't find anything.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [patch 09/15] libata: Improved ata_exec_internal timeout handling

[Tejun]

Brian King wrote:
> Tejun Heo wrote:
> 
>>brking@us.ibm.com wrote:
>>
>>>Currently ata_exec_internal does very minimal cleanup if
>>>the executing command times out. This works for most usage
>>>scenarios, but can cause problems for hosts that set
>>>ATA_FLAG_PIO_DMA. For these users, the DMA buffer gets unmapped
>>>while the timed out out command is still active, which has
>>>the potential to cause PCI DMA errors if the command is
>>>still in progress. The following patch modifies ata_exec_internal
>>>to invoke eng_timeout on a timeout to allow for the timed out
>>>command to be cleaned up better before proceeding further.
>>>This patch is also in preparation for SAS attached SATA devices.
>>>
>>
>>Hello, again.
>>
>>ata_exec_internal() will be used during EH, so if you call error handler
>>from ata_exec_internal()....  In my pending EH patchset, there's a patch
>>to implement ->post_internal() callback which cleans up after an
>>internal command (successful or not).  Hopefully, it will be merged in
>>not too distant future.  Can ata_exec_internal() timeout handling wait
>>till then?
> 
> 
> Do you have a pointer to this patchset? I did a quick search for
> post_internal on the archive and didn't find anything.
> 

Hi,

No, not posted yet.  It's pretty simple though.  Hmmm... I'll post the
patch against the current upstream tomorrow.  It's pretty much
independent from other changes anyway.

-- 
tejun

Re: [patch 09/15] libata: Improved ata_exec_internal timeout handling

[Jeff Garzik]

brking@us.ibm.com wrote:
> Currently ata_exec_internal does very minimal cleanup if
> the executing command times out. This works for most usage
> scenarios, but can cause problems for hosts that set
> ATA_FLAG_PIO_DMA. For these users, the DMA buffer gets unmapped
> while the timed out out command is still active, which has
> the potential to cause PCI DMA errors if the command is
> still in progress. The following patch modifies ata_exec_internal
> to invoke eng_timeout on a timeout to allow for the timed out
> command to be cleaned up better before proceeding further.
> This patch is also in preparation for SAS attached SATA devices.
> 
> Signed-off-by: Brian King <brking@us.ibm.com>

Really this sort of thing is definitely changing an area that Tejun is 
also changing pretty heavily...

	Jeff

Re: [patch 09/15] libata: Improved ata_exec_internal timeout handling

[Tejun Heo]

On Wed, Feb 08, 2006 at 11:07:55AM -0600, Brian King wrote:
> Tejun Heo wrote:
> > brking@us.ibm.com wrote:
> >> Currently ata_exec_internal does very minimal cleanup if
> >> the executing command times out. This works for most usage
> >> scenarios, but can cause problems for hosts that set
> >> ATA_FLAG_PIO_DMA. For these users, the DMA buffer gets unmapped
> >> while the timed out out command is still active, which has
> >> the potential to cause PCI DMA errors if the command is
> >> still in progress. The following patch modifies ata_exec_internal
> >> to invoke eng_timeout on a timeout to allow for the timed out
> >> command to be cleaned up better before proceeding further.
> >> This patch is also in preparation for SAS attached SATA devices.
> >>
> > 
> > Hello, again.
> > 
> > ata_exec_internal() will be used during EH, so if you call error handler
> > from ata_exec_internal()....  In my pending EH patchset, there's a patch
> > to implement ->post_internal() callback which cleans up after an
> > internal command (successful or not).  Hopefully, it will be merged in
> > not too distant future.  Can ata_exec_internal() timeout handling wait
> > till then?
> 
> Do you have a pointer to this patchset? I did a quick search for
> post_internal on the archive and didn't find anything.
> 

Hello, Brian.

Sorry it took long.  Here's the patch against the current upstream.
I've just cut & pasted related part from my working tree and as
earlier bits of EH changes aren't in the upstream, the following patch
doesn't compile, let alone work.  But it should give enough idea about
->post_internal().

Thanks.

diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 81ba5f5..87ffd80 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -193,6 +193,7 @@ static irqreturn_t ahci_interrupt (int i
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void ahci_irq_clear(struct ata_port *ap);
 static void ahci_eng_timeout(struct ata_port *ap);
+static void ahci_post_internal_cmd(struct ata_queued_cmd *qc);
 static int ahci_port_start(struct ata_port *ap);
 static void ahci_port_stop(struct ata_port *ap);
 static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
@@ -235,6 +236,7 @@ static const struct ata_port_operations 
 	.qc_issue		= ahci_qc_issue,
 
 	.eng_timeout		= ahci_eng_timeout,
+	.post_internal_cmd	= ahci_post_internal_cmd,
 
 	.irq_handler		= ahci_interrupt,
 	.irq_clear		= ahci_irq_clear,
@@ -827,6 +829,20 @@ static void ahci_eng_timeout(struct ata_
 	ata_eh_qc_complete(qc);
 }
 
+static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
+{
+	struct ahci_port_priv *pp = qc->ap->private_data;
+
+	if (!qc->err_mask)
+		return;
+
+	if (qc->err_mask & AC_ERR_PSEUDO && pp->eh_irq_stat & PORT_IRQ_TF_ERR)
+		qc->err_mask |= AC_ERR_DEV;
+
+	ahci_stop_engine(qc->ap);
+	ahci_start_engine(qc->ap);
+}
+
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
 	void __iomem *mmio = ap->host_set->mmio_base;
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index f312ea9..a81b38e 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -215,6 +215,7 @@ static const struct ata_port_operations 
 	.qc_issue		= ata_qc_issue_prot,
 
 	.eng_timeout		= ata_eng_timeout,
+	.post_internal_cmd	= ata_std_post_internal_cmd,
 
 	.irq_handler		= ata_interrupt,
 	.irq_clear		= ata_bmdma_irq_clear,
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f6511bd..53550fd 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1214,6 +1214,9 @@ ata_exec_internal(struct ata_port *ap, s
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
 
+	if (qc->ap->ops->post_internal_cmd)
+		qc->ap->ops->post_internal_cmd(qc);
+
 	*tf = qc->tf;
 	err_mask = qc->err_mask;
 
@@ -5471,6 +5474,38 @@ int ata_ratelimit(void)
 	return rc;
 }
 
+/**
+ *	ata_std_post_internal_cmd - Clean up after an internal command
+ *	@qc: Internal command to clean up
+ *
+ *	Stock ->post_internal_cmd handler.  It's supposed to clean up
+ *	after an internal command which could have succeeded or not.
+ *	This function cleans up BMDMA status after a failed internal
+ *	command.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ */
+void ata_std_post_internal_cmd(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+
+	if (!qc->err_mask)
+		return;
+
+	ata_flush_pio_tasks(ap);
+
+	/* Stop DMA engine */
+	if (qc && (qc->tf.protocol == ATA_PROT_DMA ||
+		   qc->tf.protocol == ATA_PROT_ATAPI_DMA))
+		ap->ops->bmdma_stop(qc);
+
+	/* clear irq and ack bmdma irq events */
+	ata_altstatus(ap);
+	ata_chk_status(ap);
+	ap->ops->irq_clear(ap);
+}
+
 /*
  * libata is essentially a library of internal helper functions for
  * low-level ATA host controller drivers.  As such, the API/ABI is
@@ -5487,6 +5522,7 @@ EXPORT_SYMBOL_GPL(ata_sg_init_one);
 EXPORT_SYMBOL_GPL(ata_qc_complete);
 EXPORT_SYMBOL_GPL(ata_qc_issue_prot);
 EXPORT_SYMBOL_GPL(ata_eng_timeout);
+EXPORT_SYMBOL_GPL(ata_std_post_internal_cmd);
 EXPORT_SYMBOL_GPL(ata_tf_load);
 EXPORT_SYMBOL_GPL(ata_tf_read);
 EXPORT_SYMBOL_GPL(ata_noop_dev_select);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f4cd1eb..f0e6ba7 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -439,6 +439,8 @@ struct ata_port_operations {
 
 	void (*eng_timeout) (struct ata_port *ap);
 
+	void (*post_internal_cmd) (struct ata_queued_cmd *qc);
+
 	irqreturn_t (*irq_handler)(int, void *, struct pt_regs *);
 	void (*irq_clear) (struct ata_port *);
 
@@ -553,6 +555,7 @@ extern u8   ata_bmdma_status(struct ata_
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
 extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eng_timeout(struct ata_port *ap);
+extern void ata_std_post_internal_cmd(struct ata_queued_cmd *qc);
 extern void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
 			      struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));