[PATCH 1/2] libata: fix ata_qc_issue failure path

[PATCH 1/2] libata: fix ata_qc_issue failure path

[Tejun Heo]

On sg_err failure path, ata_qc_issue() doesn't mark the qc active
before returning.  This triggers WARN_ON() in __ata_qc_complete() when
the qc gets completed.  This patch moves ap->active_tag and
QCFLAG_ACTIVE setting to the top of the function.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2006-03-31 20:28:47.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2006-03-31 20:34:02.000000000 +0900
@@ -4006,6 +4006,9 @@ unsigned int ata_qc_issue(struct ata_que
 {
 	struct ata_port *ap = qc->ap;
 
+	qc->ap->active_tag = qc->tag;
+	qc->flags |= ATA_QCFLAG_ACTIVE;
+
 	if (ata_should_dma_map(qc)) {
 		if (qc->flags & ATA_QCFLAG_SG) {
 			if (ata_sg_setup(qc))
@@ -4020,9 +4023,6 @@ unsigned int ata_qc_issue(struct ata_que
 
 	ap->ops->qc_prep(qc);
 
-	qc->ap->active_tag = qc->tag;
-	qc->flags |= ATA_QCFLAG_ACTIVE;
-
 	return ap->ops->qc_issue(qc);
 
 sg_err:

[PATCH 2/2] libata: make ata_qc_issue complete failed qcs

[Tejun Heo]

There is no reason for the issuer to diddle with a failed qc as the
issuer has complete control over when a qc gets freed (usually in
->complete_fn).  Make ata_qc_issue() responsible for completing qcs
which failed to issue.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 libata-core.c |   20 +++++++++-----------
 libata-scsi.c |    8 ++------
 libata.h      |    2 +-
 3 files changed, 12 insertions(+), 18 deletions(-)

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2006-03-31 20:34:02.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2006-03-31 20:34:06.000000000 +0900
@@ -989,9 +989,7 @@ ata_exec_internal(struct ata_port *ap, s
 	qc->private_data = &wait;
 	qc->complete_fn = ata_qc_complete_internal;
 
-	qc->err_mask = ata_qc_issue(qc);
-	if (qc->err_mask)
-		ata_qc_complete(qc);
+	ata_qc_issue(qc);
 
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
@@ -3997,12 +3995,8 @@ static inline int ata_should_dma_map(str
  *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
- *
- *	RETURNS:
- *	Zero on success, AC_ERR_* mask on failure
  */
-
-unsigned int ata_qc_issue(struct ata_queued_cmd *qc)
+void ata_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 
@@ -4023,14 +4017,18 @@ unsigned int ata_qc_issue(struct ata_que
 
 	ap->ops->qc_prep(qc);
 
-	return ap->ops->qc_issue(qc);
+	qc->err_mask |= ap->ops->qc_issue(qc);
+	if (unlikely(qc->err_mask))
+		goto err;
+	return;
 
 sg_err:
 	qc->flags &= ~ATA_QCFLAG_DMAMAP;
-	return AC_ERR_SYSTEM;
+	qc->err_mask |= AC_ERR_SYSTEM;
+err:
+	ata_qc_complete(qc);
 }
 
-
 /**
  *	ata_qc_issue_prot - issue taskfile to device in proto-dependent manner
  *	@qc: command to issue to device
Index: work/drivers/scsi/libata-scsi.c
===================================================================
--- work.orig/drivers/scsi/libata-scsi.c	2006-03-31 20:34:02.000000000 +0900
+++ work/drivers/scsi/libata-scsi.c	2006-03-31 20:34:06.000000000 +0900
@@ -1431,9 +1431,7 @@ static void ata_scsi_translate(struct at
 		goto early_finish;
 
 	/* select device, send command to hardware */
-	qc->err_mask = ata_qc_issue(qc);
-	if (qc->err_mask)
-		ata_qc_complete(qc);
+	ata_qc_issue(qc);
 
 	VPRINTK("EXIT\n");
 	return;
@@ -2199,9 +2197,7 @@ static void atapi_request_sense(struct a
 
 	qc->complete_fn = atapi_sense_complete;
 
-	qc->err_mask = ata_qc_issue(qc);
-	if (qc->err_mask)
-		ata_qc_complete(qc);
+	ata_qc_issue(qc);
 
 	DPRINTK("EXIT\n");
 }
Index: work/drivers/scsi/libata.h
===================================================================
--- work.orig/drivers/scsi/libata.h	2006-03-31 20:34:02.000000000 +0900
+++ work/drivers/scsi/libata.h	2006-03-31 20:34:06.000000000 +0900
@@ -47,7 +47,7 @@ extern struct ata_queued_cmd *ata_qc_new
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_port_flush_task(struct ata_port *ap);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
-extern unsigned int ata_qc_issue(struct ata_queued_cmd *qc);
+extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
 extern void ata_dev_select(struct ata_port *ap, unsigned int device,
                            unsigned int wait, unsigned int can_sleep);

Re: [PATCH 1/2] libata: fix ata_qc_issue failure path

[Jeff Garzik]

Tejun Heo wrote:
> On sg_err failure path, ata_qc_issue() doesn't mark the qc active
> before returning.  This triggers WARN_ON() in __ata_qc_complete() when
> the qc gets completed.  This patch moves ap->active_tag and
> QCFLAG_ACTIVE setting to the top of the function.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-2, but two comments:

* this patch widens the race window for the remaining unlocked uses of 
ATA_QCFLAG_ACTIVE

* in the current code, its questionable whether ATA_QCFLAG_ACTIVE has 
much value.  The flag may have more value after your EH work, but its 
not terribly important in the current #upstream.

	Jeff

Re: [PATCH 1/2] libata: fix ata_qc_issue failure path

[Tejun Heo]

Hello, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
>> On sg_err failure path, ata_qc_issue() doesn't mark the qc active
>> before returning.  This triggers WARN_ON() in __ata_qc_complete() when
>> the qc gets completed.  This patch moves ap->active_tag and
>> QCFLAG_ACTIVE setting to the top of the function.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> applied 1-2, but two comments:
> 
> * this patch widens the race window for the remaining unlocked uses of 
> ATA_QCFLAG_ACTIVE

Hmmm.. the only unloked use of ATA_QCFLAG_ACTIVE I could find was in 
pio_task and later patches will tighten that up. Any other places?

> * in the current code, its questionable whether ATA_QCFLAG_ACTIVE has 
> much value.  The flag may have more value after your EH work, but its 
> not terribly important in the current #upstream.

Yeap, ap->active_tag always coincides with ATA_QCFLAG_ACTIVE. And, yeah, 
it gets more important especially with NCQ as then we have two different 
mechanism to indicate active commands from the port (ap->active_tag and 
ap->sactive), so ATA_QCFLAG_ACTIVE is quite handy as aggregate test 
condition.

Thanks.

-- 
tejun