Re: queued_cmd ownership

[Albert Lee <albertcc@tw.ibm.com>]

Jeff Garzik wrote:
> 
> I think we need to go to an alloc/free model for ata_queued_cmd, so that
> the creator can own the results after the queued_cmd is complete.
> 
 
I've no good idea for this issue; only the following quick fix.

Changes:
- Add "void *complete_data" to ata_queued_cmd as private parameter for the complete function.

- Let the internal complete function (ata_qc_complete_noop() and ata_identify_complete())
  copy out the err_mask from the HSM thread to the caller thread, via qc->complete_data,
  before the qc is completed.

- add check for the err_mask result in the functions using internal commands:
  ex. - ata_dev_identify()
      - ata_dev_set_xfermode()
      - ata_dev_init_params()
      - ata_dev_reread_id()

- Add a new "ata_identify_complete()" complete function for ata_dev_identify()
  such that if an error is found, the "ata_identify_complete()" returns non-zero
  and qc will not be fully completed. 
  The caller thread can reuse the qc and do additional processing on the qc.  
  (modeled after the qc reuse case as in the recent atapi error handling patch)
  The caller thread has to call __ata_qc_complete() explicitly for such not-fully-completed qc.

- In ata_qc_complete(): notify the thread waiting on the "qc->waiting" completion when the
  callback indicates not to fully complete the command (non-zero).
  This is to make the waiting works for ata_identify_complete() under the not-fully-completed qc case.

Patch against the libata-dev upstream-fixes branch
(c6e6e666cbfe40f0d7fb1a293ff6332973acac37).

For your review and comment, thanks.

Albert

===========

--- upstream-fixes/include/linux/libata.h	2005-11-15 11:28:10.000000000 +0800
+++ check_result/include/linux/libata.h	2005-11-15 15:12:57.000000000 +0800
@@ -280,6 +280,7 @@ struct ata_queued_cmd {
 	struct scatterlist	*__sg;
 
 	ata_qc_cb_t		complete_fn;
+	void			*complete_data;
 
 	struct completion	*waiting;
 
--- upstream-fixes/drivers/scsi/libata-core.c	2005-11-15 11:27:59.000000000 +0800
+++ check_result/drivers/scsi/libata-core.c	2005-11-15 14:51:26.000000000 +0800
@@ -1046,6 +1046,19 @@ static unsigned int ata_pio_modes(const 
 	return modes;
 }
 
+static int ata_identify_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+{
+	*((unsigned int *)qc->complete_data) = err_mask;
+
+	if (err_mask)
+		/* half complete the command since
+		 * we might retry atapi identify.
+		 */
+		return 1;
+
+	return 0;
+}
+
 /**
  *	ata_dev_identify - obtain IDENTIFY x DEVICE page
  *	@ap: port on which device we wish to probe resides
@@ -1079,6 +1092,7 @@ static void ata_dev_identify(struct ata_
 	struct ata_queued_cmd *qc;
 	unsigned long flags;
 	int rc;
+	unsigned int err_mask;
 
 	if (!ata_dev_present(dev)) {
 		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
@@ -1116,7 +1130,8 @@ retry:
 	}
 
 	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
+	qc->complete_fn = ata_identify_complete;
+	qc->complete_data = &err_mask;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	rc = ata_qc_issue(qc);
@@ -1124,14 +1139,13 @@ retry:
 
 	if (rc)
 		goto err_out;
-	else
-		wait_for_completion(&wait);
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->ops->tf_read(ap, &qc->tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	/* Wait until the qc is completed in HSM and
+	 * the err_mask is copied by the complete function.
+	 */
+	wait_for_completion(&wait);
 
-	if (qc->tf.command & ATA_ERR) {
+	if (err_mask) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -1144,8 +1158,12 @@ retry:
 		 * to have this problem.
 		 */
 		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
-			u8 err = qc->tf.feature;
-			if (err & ATA_ABORTED) {
+			spin_lock_irqsave(&ap->host_set->lock, flags);
+			ap->ops->tf_read(ap, &qc->tf);
+			spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+			if((qc->tf.command & ATA_ERR) &&
+			   (qc->tf.feature & ATA_ABORTED)) {
 				dev->class = ATA_DEV_ATAPI;
 				qc->cursg = 0;
 				qc->cursg_ofs = 0;
@@ -1154,6 +1172,8 @@ retry:
 				goto retry;
 			}
 		}
+
+		__ata_qc_complete(qc);
 		goto err_out;
 	}
 
@@ -2244,6 +2264,7 @@ static void ata_dev_set_xfermode(struct 
 	struct ata_queued_cmd *qc;
 	int rc;
 	unsigned long flags;
+	unsigned int err_mask;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
@@ -2259,17 +2280,28 @@ static void ata_dev_set_xfermode(struct 
 
 	qc->waiting = &wait;
 	qc->complete_fn = ata_qc_complete_noop;
+	qc->complete_data = &err_mask;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	rc = ata_qc_issue(qc);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	if (rc)
-		ata_port_disable(ap);
-	else
-		wait_for_completion(&wait);
+		goto err_out;
+
+	/* Wait until the qc is completed in HSM and
+	 * the err_mask is copied by the complete function.
+	 */
+	wait_for_completion(&wait);
+
+	if (err_mask)
+		goto err_out;
 
 	DPRINTK("EXIT\n");
+	return;
+err_out:
+	printk(KERN_ERR "ata%u: set XFER MODE failed\n", ap->id);
+	ata_port_disable(ap);
 }
 
 /**
@@ -2286,6 +2318,7 @@ static void ata_dev_reread_id(struct ata
 	struct ata_queued_cmd *qc;
 	unsigned long flags;
 	int rc;
+	unsigned int err_mask;
 
 	qc = ata_qc_new_init(ap, dev);
 	BUG_ON(qc == NULL);
@@ -2307,6 +2340,7 @@ static void ata_dev_reread_id(struct ata
 
 	qc->waiting = &wait;
 	qc->complete_fn = ata_qc_complete_noop;
+	qc->complete_data = &err_mask;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	rc = ata_qc_issue(qc);
@@ -2315,8 +2349,14 @@ static void ata_dev_reread_id(struct ata
 	if (rc)
 		goto err_out;
 
+	/* Wait until the qc is completed in HSM and
+	 * the err_mask is copied by the complete function.
+	 */
 	wait_for_completion(&wait);
 
+	if (err_mask)
+		goto err_out;
+
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2325,6 +2365,7 @@ static void ata_dev_reread_id(struct ata
 
 	return;
 err_out:
+	printk(KERN_ERR "ata%u: reread IDENTIFY device data failed\n", ap->id);
 	ata_port_disable(ap);
 }
 
@@ -2344,6 +2385,7 @@ static void ata_dev_init_params(struct a
 	unsigned long flags;
 	u16 sectors = dev->id[6];
 	u16 heads   = dev->id[3];
+	unsigned int err_mask;
 
 	/* Number of sectors per track 1-255. Number of heads 1-16 */
 	if (sectors < 1 || sectors > 255 || heads < 1 || heads > 16)
@@ -2363,17 +2405,28 @@ static void ata_dev_init_params(struct a
 
 	qc->waiting = &wait;
 	qc->complete_fn = ata_qc_complete_noop;
+	qc->complete_data = &err_mask;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	rc = ata_qc_issue(qc);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	if (rc)
-		ata_port_disable(ap);
-	else
-		wait_for_completion(&wait);
+		goto err_out;
+
+	/* Wait until the qc is completed in HSM and
+	 * the err_mask is copied by the complete function.
+	 */
+	wait_for_completion(&wait);
+
+	if (err_mask)
+		goto err_out;
 
 	DPRINTK("EXIT\n");
+	return;
+err_out:
+	printk(KERN_ERR "ata%u: init CHS dev params failed\n", ap->id);
+	ata_port_disable(ap);
 }
 
 /**
@@ -3422,6 +3475,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 
 int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask)
 {
+	*((unsigned int *)qc->complete_data) = err_mask;
 	return 0;
 }
 
@@ -3501,8 +3555,14 @@ void ata_qc_complete(struct ata_queued_c
 	/* if callback indicates not to complete command (non-zero),
 	 * return immediately
 	 */
-	if (rc != 0)
+	if (rc != 0) {
+		if (qc->waiting) {
+			struct completion *waiting = qc->waiting;
+			qc->waiting = NULL;
+			complete(waiting);
+		}
 		return;
+	}
 
 	__ata_qc_complete(qc);