[PATCHSET] libata: various fixes related to EH

[PATCHSET] libata: various fixes related to EH

[Tejun Heo]

Hello, Jeff, Albert.

This patchset is composed of 12 patches fixing/updating various EH
related stuff in libata.  Although not all of the patches are
logically related, they need to be ordered because they modify
similar parts of code.

#01	: cosmetic
#02	: ata_qc_new/free model
#03	: ata_qc_issue() error handling fix
#04	: detailed AC_ERR_* flags
#05	: return AC_ERR_* from issue functions
#06-07	: ata_eh_qc_retry/complete
#08-12	: eh synchronization (#12 is the pio/eh sync patch I talked
	  about in the thread "update timer for PIO polling - revised")

Jeff, these are hopefully more acceptable patches from my recent EH
work.  I'll soon follow up with more pervasive patches.  My working
version of new EH now does most things described in ATA EH doc
including reset, revalidation and gearing down.  I've also ported
Jen's NCQ support over it, and, although it has a few issues, it's
generally working okay.

I think libata really needs better EH and NCQ/hotplug stuff is already
somewhat late.  I hope we can go somewhere this time.

Thanks

--
tejun

[PATCH 01/12] libata: fold __ata_qc_complete() into ata_qc_free()

[Tejun Heo]

All ata_qc_free() does is calling __ata_qc_complete() which isn't used
anywhere else.  Fold __ata_qc_complete() into ata_qc_free().

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

---

 drivers/scsi/libata-core.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

7474f1f37d6bcfa451b24e9a6905b56a1bd45a29
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index a07bd35..0e49323 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -73,7 +73,6 @@ static int fgb(u32 bitmap);
 static int ata_choose_xfer_mode(const struct ata_port *ap,
 				u8 *xfer_mode_out,
 				unsigned int *xfer_shift_out);
-static void __ata_qc_complete(struct ata_queued_cmd *qc);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -3593,21 +3592,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-static void __ata_qc_complete(struct ata_queued_cmd *qc)
-{
-	struct ata_port *ap = qc->ap;
-	unsigned int tag;
-
-	qc->flags = 0;
-	tag = qc->tag;
-	if (likely(ata_tag_valid(tag))) {
-		if (tag == ap->active_tag)
-			ap->active_tag = ATA_TAG_POISON;
-		qc->tag = ATA_TAG_POISON;
-		clear_bit(tag, &ap->qactive);
-	}
-}
-
 /**
  *	ata_qc_free - free unused ata_queued_cmd
  *	@qc: Command to complete
@@ -3620,9 +3604,19 @@ static void __ata_qc_complete(struct ata
  */
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
+	struct ata_port *ap = qc->ap;
+	unsigned int tag;
+
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 
-	__ata_qc_complete(qc);
+	qc->flags = 0;
+	tag = qc->tag;
+	if (likely(ata_tag_valid(tag))) {
+		if (tag == ap->active_tag)
+			ap->active_tag = ATA_TAG_POISON;
+		qc->tag = ATA_TAG_POISON;
+		clear_bit(tag, &ap->qactive);
+	}
 }
 
 /**
@@ -3662,7 +3656,7 @@ void ata_qc_complete(struct ata_queued_c
 	if (rc != 0)
 		return;
 
-	__ata_qc_complete(qc);
+	ata_qc_free(qc);
 
 	VPRINTK("EXIT\n");
 }
-- 
1.0.8

[PATCH 02/12] libata: make the owner of a qc responsible for freeing it

[Tejun Heo]

qc used to be freed automatically on command completion.  However, as
a qc can carry information about its completion status, it can be
useful to its owner/issuer after command completion.  This patch makes
freeing qc responsibility of its owner.  This simplifies
ata_exec_internal() and makes command turn-around for atapi request
sensing less hackish.

This change was originally suggested by Jeff Garzik.

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

---

 drivers/scsi/libata-core.c |   47 ++++++++++++--------------------------------
 drivers/scsi/libata-scsi.c |   14 +++++++------
 include/linux/libata.h     |    2 +-
 3 files changed, 21 insertions(+), 42 deletions(-)

576c63b9e5ffef8fac2a379b639fbe198fd150ac
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 0e49323..15df633 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1072,24 +1072,12 @@ static unsigned int ata_pio_modes(const 
 	   timing API will get this right anyway */
 }
 
-struct ata_exec_internal_arg {
-	unsigned int err_mask;
-	struct ata_taskfile *tf;
-	struct completion *waiting;
-};
-
-int ata_qc_complete_internal(struct ata_queued_cmd *qc)
+void ata_qc_complete_internal(struct ata_queued_cmd *qc)
 {
-	struct ata_exec_internal_arg *arg = qc->private_data;
-	struct completion *waiting = arg->waiting;
+	struct completion *waiting = qc->private_data;
 
-	if (!(qc->err_mask & ~AC_ERR_DEV))
-		qc->ap->ops->tf_read(qc->ap, arg->tf);
-	arg->err_mask = qc->err_mask;
-	arg->waiting = NULL;
+	qc->ap->ops->tf_read(qc->ap, &qc->tf);
 	complete(waiting);
-
-	return 0;
 }
 
 /**
@@ -1120,7 +1108,7 @@ ata_exec_internal(struct ata_port *ap, s
 	struct ata_queued_cmd *qc;
 	DECLARE_COMPLETION(wait);
 	unsigned long flags;
-	struct ata_exec_internal_arg arg;
+	unsigned int err_mask;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 
@@ -1134,9 +1122,7 @@ ata_exec_internal(struct ata_port *ap, s
 		qc->nsect = buflen / ATA_SECT_SIZE;
 	}
 
-	arg.waiting = &wait;
-	arg.tf = tf;
-	qc->private_data = &arg;
+	qc->private_data = &wait;
 	qc->complete_fn = ata_qc_complete_internal;
 
 	if (ata_qc_issue(qc))
@@ -1153,7 +1139,7 @@ ata_exec_internal(struct ata_port *ap, s
 		 * before the caller cleans up, it will result in a
 		 * spurious interrupt.  We can live with that.
 		 */
-		if (arg.waiting) {
+		if (qc->flags & ATA_QCFLAG_ACTIVE) {
 			qc->err_mask = AC_ERR_OTHER;
 			ata_qc_complete(qc);
 			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
@@ -1163,7 +1149,12 @@ ata_exec_internal(struct ata_port *ap, s
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
 
-	return arg.err_mask;
+	*tf = qc->tf;
+	err_mask = qc->err_mask;
+
+	ata_qc_free(qc);
+
+	return err_mask;
 
  issue_fail:
 	ata_qc_free(qc);
@@ -3633,8 +3624,6 @@ void ata_qc_free(struct ata_queued_cmd *
 
 void ata_qc_complete(struct ata_queued_cmd *qc)
 {
-	int rc;
-
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
 
@@ -3648,17 +3637,7 @@ void ata_qc_complete(struct ata_queued_c
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 
 	/* call completion callback */
-	rc = qc->complete_fn(qc);
-
-	/* if callback indicates not to complete command (non-zero),
-	 * return immediately
-	 */
-	if (rc != 0)
-		return;
-
-	ata_qc_free(qc);
-
-	VPRINTK("EXIT\n");
+	qc->complete_fn(qc);
 }
 
 static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 0e65bfe..ce3fe92 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1219,7 +1219,7 @@ nothing_to_do:
 	return 1;
 }
 
-static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
+static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
@@ -1256,7 +1256,7 @@ static int ata_scsi_qc_complete(struct a
 
 	qc->scsidone(cmd);
 
-	return 0;
+	ata_qc_free(qc);
 }
 
 /**
@@ -1982,7 +1982,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
 	done(cmd);
 }
 
-static int atapi_sense_complete(struct ata_queued_cmd *qc)
+static void atapi_sense_complete(struct ata_queued_cmd *qc)
 {
 	if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
 		/* FIXME: not quite right; we don't want the
@@ -1993,7 +1993,7 @@ static int atapi_sense_complete(struct a
 		ata_gen_ata_desc_sense(qc);
 
 	qc->scsidone(qc->scsicmd);
-	return 0;
+	ata_qc_free(qc);
 }
 
 /* is it pointless to prefer PIO for "safety reasons"? */
@@ -2050,7 +2050,7 @@ static void atapi_request_sense(struct a
 	DPRINTK("EXIT\n");
 }
 
-static int atapi_qc_complete(struct ata_queued_cmd *qc)
+static void atapi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	unsigned int err_mask = qc->err_mask;
@@ -2060,7 +2060,7 @@ static int atapi_qc_complete(struct ata_
 	if (unlikely(err_mask & AC_ERR_DEV)) {
 		cmd->result = SAM_STAT_CHECK_CONDITION;
 		atapi_request_sense(qc);
-		return 1;
+		return;
 	}
 
 	else if (unlikely(err_mask))
@@ -2100,7 +2100,7 @@ static int atapi_qc_complete(struct ata_
 	}
 
 	qc->scsidone(cmd);
-	return 0;
+	ata_qc_free(qc);
 }
 /**
  *	atapi_xlat - Initialize PACKET taskfile
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 46ccea2..d58b659 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -235,7 +235,7 @@ struct ata_port;
 struct ata_queued_cmd;
 
 /* typedefs */
-typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
+typedef void (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
 
 struct ata_ioports {
 	unsigned long		cmd_addr;
-- 
1.0.8

Re: [PATCH 02/12] libata: make the owner of a qc responsible for freeing it

[Jeff Garzik]

Tejun Heo wrote:
> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c
> @@ -1219,7 +1219,7 @@ nothing_to_do:
>  	return 1;
>  }
>  
> -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> +static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	u8 *cdb = cmd->cmnd;
> @@ -1256,7 +1256,7 @@ static int ata_scsi_qc_complete(struct a
>  
>  	qc->scsidone(cmd);
>  
> -	return 0;
> +	ata_qc_free(qc);
>  }
>  
>  /**
> @@ -1982,7 +1982,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
>  	done(cmd);
>  }
>  
> -static int atapi_sense_complete(struct ata_queued_cmd *qc)
> +static void atapi_sense_complete(struct ata_queued_cmd *qc)
>  {
>  	if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
>  		/* FIXME: not quite right; we don't want the
> @@ -1993,7 +1993,7 @@ static int atapi_sense_complete(struct a
>  		ata_gen_ata_desc_sense(qc);
>  
>  	qc->scsidone(qc->scsicmd);
> -	return 0;
> +	ata_qc_free(qc);
>  }
>  
>  /* is it pointless to prefer PIO for "safety reasons"? */
> @@ -2050,7 +2050,7 @@ static void atapi_request_sense(struct a
>  	DPRINTK("EXIT\n");
>  }
>  
> -static int atapi_qc_complete(struct ata_queued_cmd *qc)
> +static void atapi_qc_complete(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	unsigned int err_mask = qc->err_mask;
> @@ -2060,7 +2060,7 @@ static int atapi_qc_complete(struct ata_
>  	if (unlikely(err_mask & AC_ERR_DEV)) {
>  		cmd->result = SAM_STAT_CHECK_CONDITION;
>  		atapi_request_sense(qc);
> -		return 1;
> +		return;
>  	}
>  
>  	else if (unlikely(err_mask))
> @@ -2100,7 +2100,7 @@ static int atapi_qc_complete(struct ata_
>  	}
>  
>  	qc->scsidone(cmd);
> -	return 0;
> +	ata_qc_free(qc);


Can you describe the ATAPI error handling flow, after applying these two 
changes (patch #1 and this one, patch #2)?

	Jeff

Re: [PATCH 02/12] libata: make the owner of a qc responsible for freeing it

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> --- a/drivers/scsi/libata-scsi.c
>> +++ b/drivers/scsi/libata-scsi.c
>> @@ -1219,7 +1219,7 @@ nothing_to_do:
>>      return 1;
>>  }
>>  
>> -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>> +static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>>  {
>>      struct scsi_cmnd *cmd = qc->scsicmd;
>>      u8 *cdb = cmd->cmnd;
>> @@ -1256,7 +1256,7 @@ static int ata_scsi_qc_complete(struct a
>>  
>>      qc->scsidone(cmd);
>>  
>> -    return 0;
>> +    ata_qc_free(qc);
>>  }
>>  
>>  /**
>> @@ -1982,7 +1982,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
>>      done(cmd);
>>  }
>>  
>> -static int atapi_sense_complete(struct ata_queued_cmd *qc)
>> +static void atapi_sense_complete(struct ata_queued_cmd *qc)
>>  {
>>      if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
>>          /* FIXME: not quite right; we don't want the
>> @@ -1993,7 +1993,7 @@ static int atapi_sense_complete(struct a
>>          ata_gen_ata_desc_sense(qc);
>>  
>>      qc->scsidone(qc->scsicmd);
>> -    return 0;
>> +    ata_qc_free(qc);
>>  }
>>  
>>  /* is it pointless to prefer PIO for "safety reasons"? */
>> @@ -2050,7 +2050,7 @@ static void atapi_request_sense(struct a
>>      DPRINTK("EXIT\n");
>>  }
>>  
>> -static int atapi_qc_complete(struct ata_queued_cmd *qc)
>> +static void atapi_qc_complete(struct ata_queued_cmd *qc)
>>  {
>>      struct scsi_cmnd *cmd = qc->scsicmd;
>>      unsigned int err_mask = qc->err_mask;
>> @@ -2060,7 +2060,7 @@ static int atapi_qc_complete(struct ata_
>>      if (unlikely(err_mask & AC_ERR_DEV)) {
>>          cmd->result = SAM_STAT_CHECK_CONDITION;
>>          atapi_request_sense(qc);
>> -        return 1;
>> +        return;
>>      }
>>  
>>      else if (unlikely(err_mask))
>> @@ -2100,7 +2100,7 @@ static int atapi_qc_complete(struct ata_
>>      }
>>  
>>      qc->scsidone(cmd);
>> -    return 0;
>> +    ata_qc_free(qc);
> 
> 
> 
> Can you describe the ATAPI error handling flow, after applying these two 
> changes (patch #1 and this one, patch #2)?
> 

Sure.  Except for where ata_qc_free() is called, it isn't different from 
the original.

A. Before change.

1. ATAPI qc completes with ERR_DEV.
2. ata_qc_complete calls atapi_qc_complete which in turn sees AC_ERR_DEV 
and calls atapi_request_sense.
3. atapi_request_sense re-init qc and issues it again for REQUEST SENSE
4. atapi_qc_complete returns 1 to instruct ata_qc_complete to not call 
ata_qc_free().
5. REQUEST_SENSE completes.  ata_qc_complete is called and it calls 
atapi_sense_complete() which notifies SCSI layer and returns 0.
6. ata_qc_complete sees return value 0 and calls ata_qc_free().

B. After change.

1. ATAPI qc completes with ERR_DEV.
2. ata_qc_complete calls atapi_qc_complete which in turn sees AC_ERR_DEV 
and calls atapi_request_sense.
3. atapi_request_sense re-init qc and issues it again for REQUEST SENSE.
4. atapi_qc_complete returns without calling ata_qc_free. 
ata_qc_complete finishes.  qc isn't freed and still in-flight.
5. REQUEST_SENSE completes.  ata_qc_complete is called and it calls 
atapi_sense_complete() which notifies SCSI layer, free's the qc and returns.
6. ata_qc_complete returns.

-- 
tejun

[PATCH 03/12] libata: fix ata_qc_issue() error handling

[Tejun Heo]

When ata_qc_issue() fails, the qc might have been dma mapped or not.
So, performing only ata_qc_free() results in dma map leak.  This patch
makes ata_qc_issue() mark dma map flags correctly on failure and calls
ata_qc_complete() after ata_qc_issue() fails.

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

---

 drivers/scsi/libata-core.c |   18 ++++++++----------
 drivers/scsi/libata-scsi.c |    6 ++++--
 2 files changed, 12 insertions(+), 12 deletions(-)

1a74b7390a28190a7031e7fc444be5792c82d256
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 15df633..43a2328 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1125,8 +1125,10 @@ ata_exec_internal(struct ata_port *ap, s
 	qc->private_data = &wait;
 	qc->complete_fn = ata_qc_complete_internal;
 
-	if (ata_qc_issue(qc))
-		goto issue_fail;
+	if (ata_qc_issue(qc)) {
+		qc->err_mask = AC_ERR_OTHER;
+		ata_qc_complete(qc);
+	}
 
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
@@ -1155,11 +1157,6 @@ ata_exec_internal(struct ata_port *ap, s
 	ata_qc_free(qc);
 
 	return err_mask;
-
- issue_fail:
-	ata_qc_free(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-	return AC_ERR_OTHER;
 }
 
 /**
@@ -3687,10 +3684,10 @@ int ata_qc_issue(struct ata_queued_cmd *
 	if (ata_should_dma_map(qc)) {
 		if (qc->flags & ATA_QCFLAG_SG) {
 			if (ata_sg_setup(qc))
-				goto err_out;
+				goto sg_err;
 		} else if (qc->flags & ATA_QCFLAG_SINGLE) {
 			if (ata_sg_setup_one(qc))
-				goto err_out;
+				goto sg_err;
 		}
 	} else {
 		qc->flags &= ~ATA_QCFLAG_DMAMAP;
@@ -3703,7 +3700,8 @@ int ata_qc_issue(struct ata_queued_cmd *
 
 	return ap->ops->qc_issue(qc);
 
-err_out:
+sg_err:
+	qc->flags &= ~ATA_QCFLAG_DMAMAP;
 	return -1;
 }
 
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index ce3fe92..c496309 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1322,8 +1322,10 @@ static void ata_scsi_translate(struct at
 		goto early_finish;
 
 	/* select device, send command to hardware */
-	if (ata_qc_issue(qc))
-		goto err_did;
+	if (ata_qc_issue(qc)) {
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
+	}
 
 	VPRINTK("EXIT\n");
 	return;
-- 
1.0.8

Re: [PATCH 03/12] libata: fix ata_qc_issue() error handling

[Jeff Garzik]

Tejun Heo wrote:
> When ata_qc_issue() fails, the qc might have been dma mapped or not.
> So, performing only ata_qc_free() results in dma map leak.  This patch
> makes ata_qc_issue() mark dma map flags correctly on failure and calls
> ata_qc_complete() after ata_qc_issue() fails.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 1-3

[PATCH 06/12] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

Export two SCSI EH command handling functions.  To be used by libata EH.

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

---

 drivers/scsi/scsi_error.c |    7 ++++---
 include/scsi/scsi_eh.h    |    3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

089544e4124c0a027694e7234f3b3cd1a5103d55
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a2333d2..6bac3d2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -584,8 +584,7 @@ static int scsi_request_sense(struct scs
  *    keep a list of pending commands for final completion, and once we
  *    are ready to leave error handling we handle completion for real.
  **/
-static void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
-			       struct list_head *done_q)
+void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
 	scmd->device->host->host_failed--;
 	scmd->eh_eflags = 0;
@@ -597,6 +596,7 @@ static void scsi_eh_finish_cmd(struct sc
 	scsi_setup_cmd_retry(scmd);
 	list_move_tail(&scmd->eh_entry, done_q);
 }
+EXPORT_SYMBOL(scsi_eh_finish_cmd);
 
 /**
  * scsi_eh_get_sense - Get device sense data.
@@ -1425,7 +1425,7 @@ static void scsi_eh_ready_devs(struct Sc
  * @done_q:	list_head of processed commands.
  *
  **/
-static void scsi_eh_flush_done_q(struct list_head *done_q)
+void scsi_eh_flush_done_q(struct list_head *done_q)
 {
 	struct scsi_cmnd *scmd, *next;
 
@@ -1454,6 +1454,7 @@ static void scsi_eh_flush_done_q(struct 
 		}
 	}
 }
+EXPORT_SYMBOL(scsi_eh_flush_done_q);
 
 /**
  * scsi_unjam_host - Attempt to fix a host which has a cmd that failed.
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index fabd879..d160880 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -35,6 +35,9 @@ static inline int scsi_sense_valid(struc
 }
 
 
+extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
+			       struct list_head *done_q);
+extern void scsi_eh_flush_done_q(struct list_head *done_q);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);
-- 
1.0.8

Re: [PATCH 06/12] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Jeff Garzik]

Tejun Heo wrote:
> Export two SCSI EH command handling functions.  To be used by libata EH.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

OK, though don't forget to CC linux-scsi on this patch.

[PATCH 04/12] libata: add detailed AC_ERR_* flags

[Tejun Heo]

Add detailed AC_ERR_* flags and use them.  Long-term goal is to
describe all errors with err_mask and tf combination (tf for failed
sector information, etc...).  After proper error diagnosis is
implemented, sense data should also be generated from err_mask instead
of directly from hardware tf registers as it is currently.

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

---

 drivers/scsi/ahci.c        |    6 ++----
 drivers/scsi/libata-core.c |   12 ++++++------
 drivers/scsi/sata_mv.c     |    2 +-
 drivers/scsi/sata_sil24.c  |    2 +-
 include/linux/libata.h     |   15 ++++++++++-----
 5 files changed, 20 insertions(+), 17 deletions(-)

1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 30676b0..e7b937b 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
 	 	 * not being called from the SCSI EH.
 	 	 */
 		qc->scsidone = scsi_finish_command;
-		qc->err_mask |= AC_ERR_OTHER;
+		qc->err_mask |= AC_ERR_TIMEOUT;
 		ata_qc_complete(qc);
 	}
 
@@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct 
 		/* command processing has stopped due to error; restart */
 		ahci_restart_port(ap, status);
 
-		if (qc) {
-			qc->err_mask |= AC_ERR_OTHER;
+		if (qc)
 			ata_qc_complete(qc);
-		}
 	}
 
 	return 1;
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 43a2328..f5519f0 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1142,7 +1142,7 @@ ata_exec_internal(struct ata_port *ap, s
 		 * spurious interrupt.  We can live with that.
 		 */
 		if (qc->flags & ATA_QCFLAG_ACTIVE) {
-			qc->err_mask = AC_ERR_OTHER;
+			qc->err_mask = AC_ERR_TIMEOUT;
 			ata_qc_complete(qc);
 			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
 			       ap->id, command);
@@ -2917,7 +2917,7 @@ static unsigned long ata_pio_poll(struct
 	status = ata_chk_status(ap);
 	if (status & ATA_BUSY) {
 		if (time_after(jiffies, ap->pio_task_timeout)) {
-			qc->err_mask |= AC_ERR_ATA_BUS;
+			qc->err_mask |= AC_ERR_TIMEOUT;
 			ap->hsm_task_state = HSM_ST_TMOUT;
 			return 0;
 		}
@@ -3295,7 +3295,7 @@ static void atapi_pio_bytes(struct ata_q
 err_out:
 	printk(KERN_INFO "ata%u: dev %u: ATAPI check failed\n",
 	      ap->id, dev->devno);
-	qc->err_mask |= AC_ERR_ATA_BUS;
+	qc->err_mask |= AC_ERR_HSM;
 	ap->hsm_task_state = HSM_ST_ERR;
 }
 
@@ -3353,7 +3353,7 @@ static void ata_pio_block(struct ata_por
 	} else {
 		/* handle BSY=0, DRQ=0 as error */
 		if ((status & ATA_DRQ) == 0) {
-			qc->err_mask |= AC_ERR_ATA_BUS;
+			qc->err_mask |= AC_ERR_HSM;
 			ap->hsm_task_state = HSM_ST_ERR;
 			return;
 		}
@@ -4159,14 +4159,14 @@ static void atapi_packet_task(void *_dat
 	/* sleep-wait for BSY to clear */
 	DPRINTK("busy wait\n");
 	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
-		qc->err_mask |= AC_ERR_ATA_BUS;
+		qc->err_mask |= AC_ERR_TIMEOUT;
 		goto err_out;
 	}
 
 	/* make sure DRQ is set */
 	status = ata_chk_status(ap);
 	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
-		qc->err_mask |= AC_ERR_ATA_BUS;
+		qc->err_mask |= AC_ERR_HSM;
 		goto err_out;
 	}
 
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index cd54244..89bcd85 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1866,7 +1866,7 @@ static void mv_eng_timeout(struct ata_po
 	 	 */
 		spin_lock_irqsave(&ap->host_set->lock, flags);
 		qc->scsidone = scsi_finish_command;
-		qc->err_mask |= AC_ERR_OTHER;
+		qc->err_mask |= AC_ERR_TIMEOUT;
 		ata_qc_complete(qc);
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 9231301..fb59012 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -653,7 +653,7 @@ static void sil24_eng_timeout(struct ata
 	 */
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->scsidone = scsi_finish_command;
-	qc->err_mask |= AC_ERR_OTHER;
+	qc->err_mask |= AC_ERR_TIMEOUT;
 	ata_qc_complete(qc);
 
 	sil24_reset_controller(ap);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d58b659..8ff3a7f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -222,10 +222,15 @@ enum hsm_task_states {
 };
 
 enum ata_completion_errors {
-	AC_ERR_OTHER		= (1 << 0),
-	AC_ERR_DEV		= (1 << 1),
-	AC_ERR_ATA_BUS		= (1 << 2),
-	AC_ERR_HOST_BUS		= (1 << 3),
+	AC_ERR_DEV		= (1 << 0), /* device reported error */
+	AC_ERR_HSM		= (1 << 1), /* host state machine violation */
+	AC_ERR_TIMEOUT		= (1 << 2), /* timeout */
+	AC_ERR_MEDIA		= (1 << 3), /* media error */
+	AC_ERR_ATA_BUS		= (1 << 4), /* ATA bus error */
+	AC_ERR_HOST_BUS		= (1 << 5), /* host bus error */
+	AC_ERR_SYSTEM		= (1 << 6), /* system error */
+	AC_ERR_INVALID		= (1 << 7), /* invalid argument */
+	AC_ERR_OTHER		= (1 << 8), /* unknown */
 };
 
 /* forward declarations */
@@ -833,7 +838,7 @@ static inline int ata_try_flush_cache(co
 static inline unsigned int ac_err_mask(u8 status)
 {
 	if (status & ATA_BUSY)
-		return AC_ERR_ATA_BUS;
+		return AC_ERR_HSM;
 	if (status & (ATA_ERR | ATA_DF))
 		return AC_ERR_DEV;
 	return 0;
-- 
1.0.8

Re: [PATCH 04/12] libata: add detailed AC_ERR_* flags

[Jeff Garzik]

Tejun Heo wrote:
> Add detailed AC_ERR_* flags and use them.  Long-term goal is to
> describe all errors with err_mask and tf combination (tf for failed
> sector information, etc...).  After proper error diagnosis is
> implemented, sense data should also be generated from err_mask instead
> of directly from hardware tf registers as it is currently.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/ahci.c        |    6 ++----
>  drivers/scsi/libata-core.c |   12 ++++++------
>  drivers/scsi/sata_mv.c     |    2 +-
>  drivers/scsi/sata_sil24.c  |    2 +-
>  include/linux/libata.h     |   15 ++++++++++-----
>  5 files changed, 20 insertions(+), 17 deletions(-)
> 
> 1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
> index 30676b0..e7b937b 100644
> --- a/drivers/scsi/ahci.c
> +++ b/drivers/scsi/ahci.c
> @@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
>  	 	 * not being called from the SCSI EH.
>  	 	 */
>  		qc->scsidone = scsi_finish_command;
> -		qc->err_mask |= AC_ERR_OTHER;
> +		qc->err_mask |= AC_ERR_TIMEOUT;
>  		ata_qc_complete(qc);
>  	}
>  
> @@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct 
>  		/* command processing has stopped due to error; restart */
>  		ahci_restart_port(ap, status);
>  
> -		if (qc) {
> -			qc->err_mask |= AC_ERR_OTHER;
> +		if (qc)
>  			ata_qc_complete(qc);
> -		}

why are you erasing the error mask here?


> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 43a2328..f5519f0 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -1142,7 +1142,7 @@ ata_exec_internal(struct ata_port *ap, s
>  		 * spurious interrupt.  We can live with that.
>  		 */
>  		if (qc->flags & ATA_QCFLAG_ACTIVE) {
> -			qc->err_mask = AC_ERR_OTHER;
> +			qc->err_mask = AC_ERR_TIMEOUT;
>  			ata_qc_complete(qc);
>  			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
>  			       ap->id, command);
> @@ -2917,7 +2917,7 @@ static unsigned long ata_pio_poll(struct
>  	status = ata_chk_status(ap);
>  	if (status & ATA_BUSY) {
>  		if (time_after(jiffies, ap->pio_task_timeout)) {
> -			qc->err_mask |= AC_ERR_ATA_BUS;
> +			qc->err_mask |= AC_ERR_TIMEOUT;
>  			ap->hsm_task_state = HSM_ST_TMOUT;
>  			return 0;
>  		}

perhaps you want to set both flags?  If BSY is stuck on, that's a 
problem with the ATA bus.


> @@ -4159,14 +4159,14 @@ static void atapi_packet_task(void *_dat
>  	/* sleep-wait for BSY to clear */
>  	DPRINTK("busy wait\n");
>  	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
> -		qc->err_mask |= AC_ERR_ATA_BUS;
> +		qc->err_mask |= AC_ERR_TIMEOUT;
>  		goto err_out;
>  	}

ditto

Re: [PATCH 04/12] libata: add detailed AC_ERR_* flags

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Add detailed AC_ERR_* flags and use them.  Long-term goal is to
>> describe all errors with err_mask and tf combination (tf for failed
>> sector information, etc...).  After proper error diagnosis is
>> implemented, sense data should also be generated from err_mask instead
>> of directly from hardware tf registers as it is currently.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>>  drivers/scsi/ahci.c        |    6 ++----
>>  drivers/scsi/libata-core.c |   12 ++++++------
>>  drivers/scsi/sata_mv.c     |    2 +-
>>  drivers/scsi/sata_sil24.c  |    2 +-
>>  include/linux/libata.h     |   15 ++++++++++-----
>>  5 files changed, 20 insertions(+), 17 deletions(-)
>>
>> 1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
>> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
>> index 30676b0..e7b937b 100644
>> --- a/drivers/scsi/ahci.c
>> +++ b/drivers/scsi/ahci.c
>> @@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
>>            * not being called from the SCSI EH.
>>            */
>>          qc->scsidone = scsi_finish_command;
>> -        qc->err_mask |= AC_ERR_OTHER;
>> +        qc->err_mask |= AC_ERR_TIMEOUT;
>>          ata_qc_complete(qc);
>>      }
>>  
>> @@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct          
>> /* command processing has stopped due to error; restart */
>>          ahci_restart_port(ap, status);
>>  
>> -        if (qc) {
>> -            qc->err_mask |= AC_ERR_OTHER;
>> +        if (qc)
>>              ata_qc_complete(qc);
>> -        }
> 
> 
> why are you erasing the error mask here?
> 

Oh.. I forgot to write about it.  About 10 lines above, detailed 
err_mask is determined from irq status but qc->err_mask always used to 
be set to AC_ERR_OTHER regardless of the determined value.

> 
>> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
>> index 43a2328..f5519f0 100644
>> --- a/drivers/scsi/libata-core.c
>> +++ b/drivers/scsi/libata-core.c
>> @@ -1142,7 +1142,7 @@ ata_exec_internal(struct ata_port *ap, s
>>           * spurious interrupt.  We can live with that.
>>           */
>>          if (qc->flags & ATA_QCFLAG_ACTIVE) {
>> -            qc->err_mask = AC_ERR_OTHER;
>> +            qc->err_mask = AC_ERR_TIMEOUT;
>>              ata_qc_complete(qc);
>>              printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
>>                     ap->id, command);
>> @@ -2917,7 +2917,7 @@ static unsigned long ata_pio_poll(struct
>>      status = ata_chk_status(ap);
>>      if (status & ATA_BUSY) {
>>          if (time_after(jiffies, ap->pio_task_timeout)) {
>> -            qc->err_mask |= AC_ERR_ATA_BUS;
>> +            qc->err_mask |= AC_ERR_TIMEOUT;
>>              ap->hsm_task_state = HSM_ST_TMOUT;
>>              return 0;
>>          }
> 
> 
> perhaps you want to set both flags?  If BSY is stuck on, that's a 
> problem with the ATA bus.
> 

Two things.

1. My intention was to use AC_ERR_ATA_BUS solely for recognized 
transport errors (ICRC, transport error bits in SError...) such that 
it's more useful for determining whether lowering transport speed is 
necessary or not.  For example, in my working tree, three ATA_BUS errors 
or ten HSM/DEV errors for known supported commands (SCSI rw commands) 
during last 15 mins trigger speed down.

2. For SATA, if it's actually an ATA_BUS error, later SError analysis 
should turn on ATA_BUS error.  I'm not sure about PATA.  Maybe always 
turning on ATA_BUS with HSM is a good idea for PATA.  Anyways, I think 
this deeper analysis is better done during EH.

-- 
tejun

Re: [PATCH 04/12] libata: add detailed AC_ERR_* flags

[Tejun Heo]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>> Add detailed AC_ERR_* flags and use them.  Long-term goal is to
>>> describe all errors with err_mask and tf combination (tf for failed
>>> sector information, etc...).  After proper error diagnosis is
>>> implemented, sense data should also be generated from err_mask instead
>>> of directly from hardware tf registers as it is currently.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>
>>> ---
>>>
>>>  drivers/scsi/ahci.c        |    6 ++----
>>>  drivers/scsi/libata-core.c |   12 ++++++------
>>>  drivers/scsi/sata_mv.c     |    2 +-
>>>  drivers/scsi/sata_sil24.c  |    2 +-
>>>  include/linux/libata.h     |   15 ++++++++++-----
>>>  5 files changed, 20 insertions(+), 17 deletions(-)
>>>
>>> 1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
>>> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
>>> index 30676b0..e7b937b 100644
>>> --- a/drivers/scsi/ahci.c
>>> +++ b/drivers/scsi/ahci.c
>>> @@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
>>>            * not being called from the SCSI EH.
>>>            */
>>>          qc->scsidone = scsi_finish_command;
>>> -        qc->err_mask |= AC_ERR_OTHER;
>>> +        qc->err_mask |= AC_ERR_TIMEOUT;
>>>          ata_qc_complete(qc);
>>>      }
>>>  
>>> @@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct          
>>> /* command processing has stopped due to error; restart */
>>>          ahci_restart_port(ap, status);
>>>  
>>> -        if (qc) {
>>> -            qc->err_mask |= AC_ERR_OTHER;
>>> +        if (qc)
>>>              ata_qc_complete(qc);
>>> -        }
>>
>>
>>
>> why are you erasing the error mask here?
>>
> 
> Oh.. I forgot to write about it.  About 10 lines above, detailed 
> err_mask is determined from irq status but qc->err_mask always used to 
> be set to AC_ERR_OTHER regardless of the determined value.
> 

And, yeah, I forgot to do qc->err_mask |= err_mask.  :-P

-- 
tejun

[PATCH 05/12] libata: return AC_ERR_* from issue functions

[Tejun Heo]

Return AC_ERR_* mask from issue fuctions instead of 0/-1.  This
enables things like failing a qc with AC_ERR_HSM when the device
doesn't set DRDY when the qc is about to be issued.

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

---

 drivers/scsi/ahci.c         |    4 ++--
 drivers/scsi/libata-core.c  |   17 ++++++++---------
 drivers/scsi/libata-scsi.c  |   10 ++++------
 drivers/scsi/libata.h       |    2 +-
 drivers/scsi/pdc_adma.c     |    4 ++--
 drivers/scsi/sata_mv.c      |    4 ++--
 drivers/scsi/sata_promise.c |    4 ++--
 drivers/scsi/sata_qstor.c   |    4 ++--
 drivers/scsi/sata_sil24.c   |    4 ++--
 drivers/scsi/sata_sx4.c     |    4 ++--
 include/linux/libata.h      |    4 ++--
 11 files changed, 29 insertions(+), 32 deletions(-)

027f06c43b4e575cc58d2dcb6af8723ef3cd3f2d
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index e7b937b..75b1fb5 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -184,7 +184,7 @@ struct ahci_port_priv {
 static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static int ahci_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
 static irqreturn_t ahci_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
 static void ahci_phy_reset(struct ata_port *ap);
 static void ahci_irq_clear(struct ata_port *ap);
@@ -798,7 +798,7 @@ static irqreturn_t ahci_interrupt (int i
 	return IRQ_RETVAL(handled);
 }
 
-static int ahci_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f5519f0..b29bf0d 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1125,10 +1125,9 @@ ata_exec_internal(struct ata_port *ap, s
 	qc->private_data = &wait;
 	qc->complete_fn = ata_qc_complete_internal;
 
-	if (ata_qc_issue(qc)) {
-		qc->err_mask = AC_ERR_OTHER;
+	qc->err_mask = ata_qc_issue(qc);
+	if (qc->err_mask)
 		ata_qc_complete(qc);
-	}
 
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
@@ -3674,10 +3673,10 @@ static inline int ata_should_dma_map(str
  *	spin_lock_irqsave(host_set lock)
  *
  *	RETURNS:
- *	Zero on success, negative on error.
+ *	Zero on success, AC_ERR_* mask on failure
  */
 
-int ata_qc_issue(struct ata_queued_cmd *qc)
+unsigned int ata_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 
@@ -3702,7 +3701,7 @@ int ata_qc_issue(struct ata_queued_cmd *
 
 sg_err:
 	qc->flags &= ~ATA_QCFLAG_DMAMAP;
-	return -1;
+	return AC_ERR_SYSTEM;
 }
 
 
@@ -3721,10 +3720,10 @@ sg_err:
  *	spin_lock_irqsave(host_set lock)
  *
  *	RETURNS:
- *	Zero on success, negative on error.
+ *	Zero on success, AC_ERR_* mask on failure
  */
 
-int ata_qc_issue_prot(struct ata_queued_cmd *qc)
+unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 
@@ -3769,7 +3768,7 @@ int ata_qc_issue_prot(struct ata_queued_
 
 	default:
 		WARN_ON(1);
-		return -1;
+		return AC_ERR_SYSTEM;
 	}
 
 	return 0;
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index c496309..95e3c27 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1322,10 +1322,9 @@ static void ata_scsi_translate(struct at
 		goto early_finish;
 
 	/* select device, send command to hardware */
-	if (ata_qc_issue(qc)) {
-		qc->err_mask |= AC_ERR_OTHER;
+	qc->err_mask = ata_qc_issue(qc);
+	if (qc->err_mask)
 		ata_qc_complete(qc);
-	}
 
 	VPRINTK("EXIT\n");
 	return;
@@ -2044,10 +2043,9 @@ static void atapi_request_sense(struct a
 
 	qc->complete_fn = atapi_sense_complete;
 
-	if (ata_qc_issue(qc)) {
-		qc->err_mask |= AC_ERR_OTHER;
+	qc->err_mask = ata_qc_issue(qc);
+	if (qc->err_mask)
 		ata_qc_complete(qc);
-	}
 
 	DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index e03ce48..9d76923 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -45,7 +45,7 @@ extern struct ata_queued_cmd *ata_qc_new
 				      struct ata_device *dev);
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
-extern int ata_qc_issue(struct ata_queued_cmd *qc);
+extern unsigned int 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);
diff --git a/drivers/scsi/pdc_adma.c b/drivers/scsi/pdc_adma.c
index e8df0c9..3a6bf58 100644
--- a/drivers/scsi/pdc_adma.c
+++ b/drivers/scsi/pdc_adma.c
@@ -131,7 +131,7 @@ static void adma_host_stop(struct ata_ho
 static void adma_port_stop(struct ata_port *ap);
 static void adma_phy_reset(struct ata_port *ap);
 static void adma_qc_prep(struct ata_queued_cmd *qc);
-static int adma_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int adma_qc_issue(struct ata_queued_cmd *qc);
 static int adma_check_atapi_dma(struct ata_queued_cmd *qc);
 static void adma_bmdma_stop(struct ata_queued_cmd *qc);
 static u8 adma_bmdma_status(struct ata_port *ap);
@@ -419,7 +419,7 @@ static inline void adma_packet_start(str
 	writew(aPIOMD4 | aGO, chan + ADMA_CONTROL);
 }
 
-static int adma_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int adma_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct adma_port_priv *pp = qc->ap->private_data;
 
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 89bcd85..281223a 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -328,7 +328,7 @@ static void mv_host_stop(struct ata_host
 static int mv_port_start(struct ata_port *ap);
 static void mv_port_stop(struct ata_port *ap);
 static void mv_qc_prep(struct ata_queued_cmd *qc);
-static int mv_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
 static irqreturn_t mv_interrupt(int irq, void *dev_instance,
 				struct pt_regs *regs);
 static void mv_eng_timeout(struct ata_port *ap);
@@ -1040,7 +1040,7 @@ static void mv_qc_prep(struct ata_queued
  *      LOCKING:
  *      Inherited from caller.
  */
-static int mv_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int mv_qc_issue(struct ata_queued_cmd *qc)
 {
 	void __iomem *port_mmio = mv_ap_base(qc->ap);
 	struct mv_port_priv *pp = qc->ap->private_data;
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index b0b0a69..bac36d5 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -95,7 +95,7 @@ static void pdc_qc_prep(struct ata_queue
 static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
 static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
 static void pdc_irq_clear(struct ata_port *ap);
-static int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
+static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
 
 
 static struct scsi_host_template pdc_ata_sht = {
@@ -544,7 +544,7 @@ static inline void pdc_packet_start(stru
 	readl((void __iomem *) ap->ioaddr.cmd_addr + PDC_PKT_SUBMIT); /* flush */
 }
 
-static int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
+static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
 {
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
index de05e28..2afbeb7 100644
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -120,7 +120,7 @@ static void qs_host_stop(struct ata_host
 static void qs_port_stop(struct ata_port *ap);
 static void qs_phy_reset(struct ata_port *ap);
 static void qs_qc_prep(struct ata_queued_cmd *qc);
-static int qs_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int qs_qc_issue(struct ata_queued_cmd *qc);
 static int qs_check_atapi_dma(struct ata_queued_cmd *qc);
 static void qs_bmdma_stop(struct ata_queued_cmd *qc);
 static u8 qs_bmdma_status(struct ata_port *ap);
@@ -352,7 +352,7 @@ static inline void qs_packet_start(struc
 	readl(chan + QS_CCT_CFF);          /* flush */
 }
 
-static int qs_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int qs_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct qs_port_priv *pp = qc->ap->private_data;
 
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index fb59012..5a7a7b1 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -251,7 +251,7 @@ static void sil24_scr_write(struct ata_p
 static void sil24_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 static void sil24_phy_reset(struct ata_port *ap);
 static void sil24_qc_prep(struct ata_queued_cmd *qc);
-static int sil24_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc);
 static void sil24_irq_clear(struct ata_port *ap);
 static void sil24_eng_timeout(struct ata_port *ap);
 static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
@@ -557,7 +557,7 @@ static void sil24_qc_prep(struct ata_que
 		sil24_fill_sg(qc, sge);
 }
 
-static int sil24_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index bc87c16..3175c6b 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -174,7 +174,7 @@ static void pdc20621_get_from_dimm(struc
 static void pdc20621_put_to_dimm(struct ata_probe_ent *pe,
 				 void *psource, u32 offset, u32 size);
 static void pdc20621_irq_clear(struct ata_port *ap);
-static int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc);
+static unsigned int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc);
 
 
 static struct scsi_host_template pdc_sata_sht = {
@@ -678,7 +678,7 @@ static void pdc20621_packet_start(struct
 	}
 }
 
-static int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc)
+static unsigned int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc)
 {
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8ff3a7f..b1ea2f9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -427,7 +427,7 @@ struct ata_port_operations {
 	void (*bmdma_start) (struct ata_queued_cmd *qc);
 
 	void (*qc_prep) (struct ata_queued_cmd *qc);
-	int (*qc_issue) (struct ata_queued_cmd *qc);
+	unsigned int (*qc_issue) (struct ata_queued_cmd *qc);
 
 	void (*eng_timeout) (struct ata_port *ap);
 
@@ -515,7 +515,7 @@ extern void ata_port_stop (struct ata_po
 extern void ata_host_stop (struct ata_host_set *host_set);
 extern irqreturn_t ata_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
 extern void ata_qc_prep(struct ata_queued_cmd *qc);
-extern int ata_qc_issue_prot(struct ata_queued_cmd *qc);
+extern unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc);
 extern void ata_sg_init_one(struct ata_queued_cmd *qc, void *buf,
 		unsigned int buflen);
 extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
-- 
1.0.8

Re: [PATCH 05/12] libata: return AC_ERR_* from issue functions

[Jeff Garzik]

Tejun Heo wrote:
> Return AC_ERR_* mask from issue fuctions instead of 0/-1.  This
> enables things like failing a qc with AC_ERR_HSM when the device
> doesn't set DRDY when the qc is about to be issued.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/ahci.c         |    4 ++--
>  drivers/scsi/libata-core.c  |   17 ++++++++---------
>  drivers/scsi/libata-scsi.c  |   10 ++++------
>  drivers/scsi/libata.h       |    2 +-
>  drivers/scsi/pdc_adma.c     |    4 ++--
>  drivers/scsi/sata_mv.c      |    4 ++--
>  drivers/scsi/sata_promise.c |    4 ++--
>  drivers/scsi/sata_qstor.c   |    4 ++--
>  drivers/scsi/sata_sil24.c   |    4 ++--
>  drivers/scsi/sata_sx4.c     |    4 ++--
>  include/linux/libata.h      |    4 ++--
>  11 files changed, 29 insertions(+), 32 deletions(-)

OK, except, this makes me wonder if an ata_err_t type is needed.

	Jeff

[PATCH 07/12] libata: implement and apply ata_eh_qc_complete/retry()

[Tejun Heo]

Implement ata_eh_qc_complete/retry() using scsi_eh_finish_cmd() and
scsi_eh_flush_done_q().  This removes all eh scsicmd finish hacks from
low level drivers.

This change was first suggested by Jeff Garzik.

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

---

 drivers/scsi/ahci.c         |   12 ++-------
 drivers/scsi/libata-core.c  |   14 ++++------
 drivers/scsi/libata-scsi.c  |   59 +++++++++++++++++++++++++++++++++++++++----
 drivers/scsi/sata_mv.c      |   12 +--------
 drivers/scsi/sata_promise.c |   12 +--------
 drivers/scsi/sata_sil24.c   |   10 +------
 drivers/scsi/sata_sx4.c     |   12 +--------
 include/linux/libata.h      |    3 ++
 8 files changed, 70 insertions(+), 64 deletions(-)

fa3ba673fc07197ab8a882d68e9641f3e8dc041f
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 75b1fb5..89ba2bd 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -672,19 +672,13 @@ static void ahci_eng_timeout(struct ata_
 		       ap->id);
 	} else {
 		ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
-
-		/* hack alert!  We cannot use the supplied completion
-	 	 * function from inside the ->eh_strategy_handler() thread.
-	 	 * libata is the only user of ->eh_strategy_handler() in
-	 	 * any kernel, so the default scsi_done() assumes it is
-	 	 * not being called from the SCSI EH.
-	 	 */
-		qc->scsidone = scsi_finish_command;
 		qc->err_mask |= AC_ERR_TIMEOUT;
-		ata_qc_complete(qc);
 	}
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
+
+	if (qc)
+		ata_eh_qc_complete(qc);
 }
 
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index b29bf0d..93ae2dc 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3449,14 +3449,6 @@ static void ata_qc_timeout(struct ata_qu
 
 	spin_lock_irqsave(&host_set->lock, flags);
 
-	/* hack alert!  We cannot use the supplied completion
-	 * function from inside the ->eh_strategy_handler() thread.
-	 * libata is the only user of ->eh_strategy_handler() in
-	 * any kernel, so the default scsi_done() assumes it is
-	 * not being called from the SCSI EH.
-	 */
-	qc->scsidone = scsi_finish_command;
-
 	switch (qc->tf.protocol) {
 
 	case ATA_PROT_DMA:
@@ -3480,12 +3472,13 @@ static void ata_qc_timeout(struct ata_qu
 
 		/* complete taskfile transaction */
 		qc->err_mask |= ac_err_mask(drv_stat);
-		ata_qc_complete(qc);
 		break;
 	}
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
 
+	ata_eh_qc_complete(qc);
+
 	DPRINTK("EXIT\n");
 }
 
@@ -4422,6 +4415,7 @@ static void ata_host_init(struct ata_por
 
 	INIT_WORK(&ap->packet_task, atapi_packet_task, ap);
 	INIT_WORK(&ap->pio_task, ata_pio_task, ap);
+	INIT_LIST_HEAD(&ap->eh_done_q);
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++)
 		ap->device[i].devno = i;
@@ -5165,6 +5159,8 @@ EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_id_string);
 EXPORT_SYMBOL_GPL(ata_dev_config);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
+EXPORT_SYMBOL_GPL(ata_eh_qc_complete);
+EXPORT_SYMBOL_GPL(ata_eh_qc_retry);
 
 EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
 EXPORT_SYMBOL_GPL(ata_timing_compute);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 95e3c27..ab6b533 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -738,17 +738,64 @@ int ata_scsi_error(struct Scsi_Host *hos
 	ap = (struct ata_port *) &host->hostdata[0];
 	ap->ops->eng_timeout(ap);
 
-	/* TODO: this is per-command; when queueing is supported
-	 * this code will either change or move to a more
-	 * appropriate place
-	 */
-	host->host_failed--;
-	INIT_LIST_HEAD(&host->eh_cmd_q);
+	assert(host->host_failed == 0 && list_empty(&host->eh_cmd_q));
+
+	scsi_eh_flush_done_q(&ap->eh_done_q);
 
 	DPRINTK("EXIT\n");
 	return 0;
 }
 
+static void ata_eh_scsidone(struct scsi_cmnd *scmd)
+{
+	/* nada */
+}
+
+static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct scsi_cmnd *scmd = qc->scsicmd;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	qc->scsidone = ata_eh_scsidone;
+	ata_qc_complete(qc);
+	assert(!ata_tag_valid(qc->tag));
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
+}
+
+/**
+ *	ata_eh_qc_complete - Complete an active ATA command from EH
+ *	@qc: Command to complete
+ *
+ *	Indicate to the mid and upper layers that an ATA command has
+ *	completed.  To be used from EH.
+ */
+void ata_eh_qc_complete(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *scmd = qc->scsicmd;
+	scmd->retries = scmd->allowed;
+	__ata_eh_qc_complete(qc);
+}
+
+/**
+ *	ata_eh_qc_retry - Tell midlayer to retry an ATA command after EH
+ *	@qc: Command to retry
+ *
+ *	Indicate to the mid and upper layers that an ATA command
+ *	should be retried.  To be used from EH.
+ *
+ *	SCSI midlayer limits the number of retries to scmd->allowed.
+ *	This function might need to adjust scmd->retries for commands
+ *	which get retried due to unrelated NCQ failures.
+ */
+void ata_eh_qc_retry(struct ata_queued_cmd *qc)
+{
+	__ata_eh_qc_complete(qc);
+}
+
 /**
  *	ata_scsi_start_stop_xlat - Translate SCSI START STOP UNIT command
  *	@qc: Storage for translated ATA taskfile
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 281223a..d9a554c 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1839,7 +1839,6 @@ static void mv_phy_reset(struct ata_port
 static void mv_eng_timeout(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
-	unsigned long flags;
 
 	printk(KERN_ERR "ata%u: Entering mv_eng_timeout\n",ap->id);
 	DPRINTK("All regs @ start of eng_timeout\n");
@@ -1858,17 +1857,8 @@ static void mv_eng_timeout(struct ata_po
 		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
 		       ap->id);
 	} else {
-		/* hack alert!  We cannot use the supplied completion
-	 	 * function from inside the ->eh_strategy_handler() thread.
-	 	 * libata is the only user of ->eh_strategy_handler() in
-	 	 * any kernel, so the default scsi_done() assumes it is
-	 	 * not being called from the SCSI EH.
-	 	 */
-		spin_lock_irqsave(&ap->host_set->lock, flags);
-		qc->scsidone = scsi_finish_command;
 		qc->err_mask |= AC_ERR_TIMEOUT;
-		ata_qc_complete(qc);
-		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+		ata_eh_qc_complete(qc);
 	}
 }
 
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index bac36d5..77ef646 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -400,21 +400,12 @@ static void pdc_eng_timeout(struct ata_p
 		goto out;
 	}
 
-	/* hack alert!  We cannot use the supplied completion
-	 * function from inside the ->eh_strategy_handler() thread.
-	 * libata is the only user of ->eh_strategy_handler() in
-	 * any kernel, so the default scsi_done() assumes it is
-	 * not being called from the SCSI EH.
-	 */
-	qc->scsidone = scsi_finish_command;
-
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 		drv_stat = ata_wait_idle(ap);
 		qc->err_mask |= __ac_err_mask(drv_stat);
-		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -424,12 +415,13 @@ static void pdc_eng_timeout(struct ata_p
 		       ap->id, qc->tf.command, drv_stat);
 
 		qc->err_mask |= ac_err_mask(drv_stat);
-		ata_qc_complete(qc);
 		break;
 	}
 
 out:
 	spin_unlock_irqrestore(&host_set->lock, flags);
+	if (qc)
+		ata_eh_qc_complete(qc);
 	DPRINTK("EXIT\n");
 }
 
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 5a7a7b1..7222fc7 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -644,17 +644,9 @@ static void sil24_eng_timeout(struct ata
 		return;
 	}
 
-	/*
-	 * hack alert!  We cannot use the supplied completion
-	 * function from inside the ->eh_strategy_handler() thread.
-	 * libata is the only user of ->eh_strategy_handler() in
-	 * any kernel, so the default scsi_done() assumes it is
-	 * not being called from the SCSI EH.
-	 */
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
-	qc->scsidone = scsi_finish_command;
 	qc->err_mask |= AC_ERR_TIMEOUT;
-	ata_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 
 	sil24_reset_controller(ap);
 }
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 3175c6b..9f992fb 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -872,20 +872,11 @@ static void pdc_eng_timeout(struct ata_p
 		goto out;
 	}
 
-	/* hack alert!  We cannot use the supplied completion
-	 * function from inside the ->eh_strategy_handler() thread.
-	 * libata is the only user of ->eh_strategy_handler() in
-	 * any kernel, so the default scsi_done() assumes it is
-	 * not being called from the SCSI EH.
-	 */
-	qc->scsidone = scsi_finish_command;
-
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 		qc->err_mask |= __ac_err_mask(ata_wait_idle(ap));
-		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -895,12 +886,13 @@ static void pdc_eng_timeout(struct ata_p
 		       ap->id, qc->tf.command, drv_stat);
 
 		qc->err_mask |= ac_err_mask(drv_stat);
-		ata_qc_complete(qc);
 		break;
 	}
 
 out:
 	spin_unlock_irqrestore(&host_set->lock, flags);
+	if (qc)
+		ata_eh_qc_complete(qc);
 	DPRINTK("EXIT\n");
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b1ea2f9..576788d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -398,6 +398,7 @@ struct ata_port {
 	unsigned long		pio_task_timeout;
 
 	u32			msg_enable;
+	struct list_head	eh_done_q;
 
 	void			*private_data;
 };
@@ -490,6 +491,8 @@ extern int ata_scsi_detect(struct scsi_h
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
 extern int ata_scsi_error(struct Scsi_Host *host);
+extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
+extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
 extern int ata_scsi_release(struct Scsi_Host *host);
 extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
 extern int ata_scsi_device_resume(struct scsi_device *);
-- 
1.0.8

[PATCH 09/12] libata: kill NULL qc handling from ->eng_timeout callbacks

[Tejun Heo]

->eng_timeout is not invoked with NULL qc anymore.  Kill NULL qc
handling from all ->eng_timeout callbacks.

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

---

 drivers/scsi/ahci.c         |   12 +++---------
 drivers/scsi/libata-core.c  |   12 +-----------
 drivers/scsi/sata_mv.c      |    9 ++-------
 drivers/scsi/sata_promise.c |    9 +--------
 drivers/scsi/sata_sil24.c   |    5 -----
 drivers/scsi/sata_sx4.c     |    9 +--------
 6 files changed, 8 insertions(+), 48 deletions(-)

ceaea59820d442a5b57e1dd2bf185173dfd2c109
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 89ba2bd..fdd11ee 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -666,19 +666,13 @@ static void ahci_eng_timeout(struct ata_
 
 	spin_lock_irqsave(&host_set->lock, flags);
 
+	ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-	} else {
-		ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
-		qc->err_mask |= AC_ERR_TIMEOUT;
-	}
+	qc->err_mask |= AC_ERR_TIMEOUT;
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
 
-	if (qc)
-		ata_eh_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 }
 
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 93ae2dc..b2f51bb 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3503,20 +3503,10 @@ static void ata_qc_timeout(struct ata_qu
 
 void ata_eng_timeout(struct ata_port *ap)
 {
-	struct ata_queued_cmd *qc;
-
 	DPRINTK("ENTER\n");
 
-	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;
-	}
+	ata_qc_timeout(ata_qc_from_tag(ap, ap->active_tag));
 
-out:
 	DPRINTK("EXIT\n");
 }
 
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index d9a554c..9b56f9b 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1853,13 +1853,8 @@ static void mv_eng_timeout(struct ata_po
 	mv_err_intr(ap);
 	mv_stop_and_reset(ap);
 
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-	} else {
-		qc->err_mask |= AC_ERR_TIMEOUT;
-		ata_eh_qc_complete(qc);
-	}
+	qc->err_mask |= AC_ERR_TIMEOUT;
+	ata_eh_qc_complete(qc);
 }
 
 /**
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index 77ef646..9fc9cbf 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -394,11 +394,6 @@ 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);
-		goto out;
-	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
@@ -418,10 +413,8 @@ static void pdc_eng_timeout(struct ata_p
 		break;
 	}
 
-out:
 	spin_unlock_irqrestore(&host_set->lock, flags);
-	if (qc)
-		ata_eh_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 	DPRINTK("EXIT\n");
 }
 
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 7222fc7..1160fda 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -638,11 +638,6 @@ 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);
-		return;
-	}
 
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->err_mask |= AC_ERR_TIMEOUT;
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 9f992fb..b4ffa3f 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -866,11 +866,6 @@ 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);
-		goto out;
-	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
@@ -889,10 +884,8 @@ static void pdc_eng_timeout(struct ata_p
 		break;
 	}
 
-out:
 	spin_unlock_irqrestore(&host_set->lock, flags);
-	if (qc)
-		ata_eh_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 	DPRINTK("EXIT\n");
 }
 
-- 
1.0.8

[PATCH 08/12] libata: fix handling of race between timeout and completion

[Tejun Heo]

If a qc completes after SCSI timer expires but before libata EH kicks
in, the qc gets completed but the scsicmd still gets passed to libata
EH resulting in ->eng_timeout invocation with NULL qc.  Currently none
of ->eng_timeout callbacks handles this properly.  This patch makes
ata_scsi_error() bypass ->eng_timeout and handle this rare case.

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

---

 drivers/scsi/libata-scsi.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 files changed, 39 insertions(+), 3 deletions(-)

ba51311c2cc3177f9c5d33aee11be1488d783c7c
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index ab6b533..accb63a 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -731,12 +731,48 @@ int ata_scsi_slave_config(struct scsi_de
 
 int ata_scsi_error(struct Scsi_Host *host)
 {
-	struct ata_port *ap;
+	struct ata_port *ap = (struct ata_port *) &host->hostdata[0];
+	struct ata_queued_cmd *qc;
+	unsigned long flags;
 
 	DPRINTK("ENTER\n");
 
-	ap = (struct ata_port *) &host->hostdata[0];
-	ap->ops->eng_timeout(ap);
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (qc) {
+		ap->ops->eng_timeout(ap);
+	} else {
+		struct scsi_cmnd *scmd;
+		unsigned char *sb;
+
+		/* The scmd had timed out but the corresponding qc
+		 * completed successfully inbetween timer expiration
+		 * and here.  Retry if possible.
+		 *
+		 * It is better to enter eng_timeout and perform EH
+		 * before retrying the command, but this case should
+		 * be _very_ rare and eng_timeout isn't ready for
+		 * NULL-qc case.
+		 */
+		scmd = list_entry(host->eh_cmd_q.next,
+				  struct scsi_cmnd, eh_entry);
+		sb = scmd->sense_buffer;
+
+		/* Timeout, fake parity for now */
+		scmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
+		sb[0] = 0x70;
+		sb[7] = 0x0a;
+		sb[2] = ABORTED_COMMAND;
+		sb[12] = 0x47;
+		sb[13] = 0x00;
+
+		printk(KERN_WARNING "ata%u: interrupt and timer raced for "
+		       "scsicmd %p\n", ap->id, scmd);
+
+		scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
+	}
 
 	assert(host->host_failed == 0 && list_empty(&host->eh_cmd_q));
 
-- 
1.0.8

Re: [PATCH 08/12] libata: fix handling of race between timeout and completion

[Jeff Garzik]

Tejun Heo wrote:
> If a qc completes after SCSI timer expires but before libata EH kicks
> in, the qc gets completed but the scsicmd still gets passed to libata
> EH resulting in ->eng_timeout invocation with NULL qc.  Currently none
> of ->eng_timeout callbacks handles this properly.  This patch makes
> ata_scsi_error() bypass ->eng_timeout and handle this rare case.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-scsi.c |   42 +++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 39 insertions(+), 3 deletions(-)
> 
> ba51311c2cc3177f9c5d33aee11be1488d783c7c
> diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
> index ab6b533..accb63a 100644
> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c
> @@ -731,12 +731,48 @@ int ata_scsi_slave_config(struct scsi_de
>  
>  int ata_scsi_error(struct Scsi_Host *host)
>  {
> -	struct ata_port *ap;
> +	struct ata_port *ap = (struct ata_port *) &host->hostdata[0];
> +	struct ata_queued_cmd *qc;
> +	unsigned long flags;
>  
>  	DPRINTK("ENTER\n");
>  
> -	ap = (struct ata_port *) &host->hostdata[0];
> -	ap->ops->eng_timeout(ap);
> +	spin_lock_irqsave(&ap->host_set->lock, flags);
> +	qc = ata_qc_from_tag(ap, ap->active_tag);
> +	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +
> +	if (qc) {
> +		ap->ops->eng_timeout(ap);
> +	} else {
> +		struct scsi_cmnd *scmd;
> +		unsigned char *sb;
> +
> +		/* The scmd had timed out but the corresponding qc
> +		 * completed successfully inbetween timer expiration
> +		 * and here.  Retry if possible.
> +		 *
> +		 * It is better to enter eng_timeout and perform EH
> +		 * before retrying the command, but this case should
> +		 * be _very_ rare and eng_timeout isn't ready for
> +		 * NULL-qc case.
> +		 */
> +		scmd = list_entry(host->eh_cmd_q.next,
> +				  struct scsi_cmnd, eh_entry);
> +		sb = scmd->sense_buffer;
> +
> +		/* Timeout, fake parity for now */
> +		scmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
> +		sb[0] = 0x70;
> +		sb[7] = 0x0a;
> +		sb[2] = ABORTED_COMMAND;
> +		sb[12] = 0x47;
> +		sb[13] = 0x00;
> +
> +		printk(KERN_WARNING "ata%u: interrupt and timer raced for "
> +		       "scsicmd %p\n", ap->id, scmd);
> +
> +		scsi_eh_finish_cmd(scmd, &ap->eh_done_q);

Honestly, I'm not sure how to best solve this.  I suppose this is ok for 
now.

	Jeff

[PATCH 12/12] libata: EH / pio tasks synchronization

[Tejun Heo]

This patch makes sure that pio tasks are flushed before proceeding
with EH.

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

---

 drivers/scsi/libata-core.c |   80 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/libata.h     |    3 +-
 2 files changed, 76 insertions(+), 7 deletions(-)

ea84319b67f28b01f8b09d0151755b890dfff7dc
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9556100..183bc26 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1072,6 +1072,72 @@ static unsigned int ata_pio_modes(const 
 	   timing API will get this right anyway */
 }
 
+static inline void
+ata_queue_packet_task(struct ata_port *ap)
+{
+	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
+		queue_work(ata_wq, &ap->packet_task);
+}
+
+static inline void
+ata_queue_pio_task(struct ata_port *ap)
+{
+	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
+		queue_work(ata_wq, &ap->pio_task);
+}
+
+static inline void
+ata_queue_delayed_pio_task(struct ata_port *ap, unsigned long delay)
+{
+	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
+		queue_delayed_work(ata_wq, &ap->pio_task, delay);
+}
+
+/**
+ *	ata_flush_pio_tasks - Flush pio_task and packet_task
+ *	@ap: the target ata_port
+ *
+ *	After this function completes, pio_task and packet_task are
+ *	guranteed not to be running or scheduled.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ */
+
+static void ata_flush_pio_tasks(struct ata_port *ap)
+{
+	int tmp = 0;
+	unsigned long flags;
+
+	DPRINTK("ENTER\n");
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	DPRINTK("flush #1\n");
+	flush_workqueue(ata_wq);
+
+	/*
+	 * At this point, if a task is running, it's guaranteed to see
+	 * the FLUSH flag; thus, it will never queue pio tasks again.
+	 * Cancel and flush.
+	 */
+	tmp |= cancel_delayed_work(&ap->pio_task);
+	tmp |= cancel_delayed_work(&ap->packet_task);
+	if (!tmp) {
+		DPRINTK("flush #2\n");
+		flush_workqueue(ata_wq);
+	}
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
+	ap->hsm_task_state = HSM_ST_IDLE;
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	DPRINTK("EXIT\n");
+}
+
 void ata_qc_complete_internal(struct ata_queued_cmd *qc)
 {
 	struct completion *waiting = qc->private_data;
@@ -3414,7 +3480,7 @@ fsm_start:
 	}
 
 	if (timeout)
-		queue_delayed_work(ata_wq, &ap->pio_task, timeout);
+		ata_queue_delayed_pio_task(ap, timeout);
 	else if (!qc_completed)
 		goto fsm_start;
 }
@@ -3447,6 +3513,8 @@ static void ata_qc_timeout(struct ata_qu
 
 	DPRINTK("ENTER\n");
 
+	ata_flush_pio_tasks(ap);
+
 	spin_lock_irqsave(&host_set->lock, flags);
 
 	switch (qc->tf.protocol) {
@@ -3733,26 +3801,26 @@ unsigned int ata_qc_issue_prot(struct at
 		ata_qc_set_polling(qc);
 		ata_tf_to_host(ap, &qc->tf);
 		ap->hsm_task_state = HSM_ST;
-		queue_work(ata_wq, &ap->pio_task);
+		ata_queue_pio_task(ap);
 		break;
 
 	case ATA_PROT_ATAPI:
 		ata_qc_set_polling(qc);
 		ata_tf_to_host(ap, &qc->tf);
-		queue_work(ata_wq, &ap->packet_task);
+		ata_queue_packet_task(ap);
 		break;
 
 	case ATA_PROT_ATAPI_NODATA:
 		ap->flags |= ATA_FLAG_NOINTR;
 		ata_tf_to_host(ap, &qc->tf);
-		queue_work(ata_wq, &ap->packet_task);
+		ata_queue_packet_task(ap);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
 		ap->flags |= ATA_FLAG_NOINTR;
 		ap->ops->tf_load(ap, &qc->tf);	 /* load tf registers */
 		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
-		queue_work(ata_wq, &ap->packet_task);
+		ata_queue_packet_task(ap);
 		break;
 
 	default:
@@ -4183,7 +4251,7 @@ static void atapi_packet_task(void *_dat
 
 		/* PIO commands are handled by polling */
 		ap->hsm_task_state = HSM_ST;
-		queue_work(ata_wq, &ap->pio_task);
+		ata_queue_pio_task(ap);
 	}
 
 	return;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 063221d..7837ba3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -162,7 +162,8 @@ enum {
 	ATA_FLAG_PIO_LBA48	= (1 << 13), /* Host DMA engine is LBA28 only */
 	ATA_FLAG_IRQ_MASK	= (1 << 14), /* Mask IRQ in PIO xfers */
 
-	ATA_FLAG_IN_EH		= (1 << 15), /* EH in progress */
+	ATA_FLAG_FLUSH_PIO_TASK	= (1 << 15), /* Flush PIO task */
+	ATA_FLAG_IN_EH		= (1 << 16), /* EH in progress */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
-- 
1.0.8

Re: [PATCH 12/12] libata: EH / pio tasks synchronization

[Jeff Garzik]

Tejun Heo wrote:
> This patch makes sure that pio tasks are flushed before proceeding
> with EH.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

> +static void ata_flush_pio_tasks(struct ata_port *ap)
> +{
> +	int tmp = 0;
> +	unsigned long flags;
> +
> +	DPRINTK("ENTER\n");
> +
> +	spin_lock_irqsave(&ap->host_set->lock, flags);
> +	ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
> +	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +
> +	DPRINTK("flush #1\n");
> +	flush_workqueue(ata_wq);
> +
> +	/*
> +	 * At this point, if a task is running, it's guaranteed to see
> +	 * the FLUSH flag; thus, it will never queue pio tasks again.
> +	 * Cancel and flush.
> +	 */
> +	tmp |= cancel_delayed_work(&ap->pio_task);
> +	tmp |= cancel_delayed_work(&ap->packet_task);
> +	if (!tmp) {
> +		DPRINTK("flush #2\n");
> +		flush_workqueue(ata_wq);
> +	}
> +
> +	spin_lock_irqsave(&ap->host_set->lock, flags);
> +	ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
> +	ap->hsm_task_state = HSM_ST_IDLE;
> +	spin_unlock_irqrestore(&ap->host_set->lock, flags);

Mostly ok, except

1) forcing hsm_task_state to HSM_ST_IDLE seems unlikely to be correct. 
The error handling code should already be doing that.

2) should break this patch up into two pieces:  add and use new helpers, 
and add flush code.

Re: [PATCH 12/12] libata: EH / pio tasks synchronization

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch makes sure that pio tasks are flushed before proceeding
>> with EH.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
>> +static void ata_flush_pio_tasks(struct ata_port *ap)
>> +{
>> +    int tmp = 0;
>> +    unsigned long flags;
>> +
>> +    DPRINTK("ENTER\n");
>> +
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +    ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
>> +    spin_unlock_irqrestore(&ap->host_set->lock, flags);
>> +
>> +    DPRINTK("flush #1\n");
>> +    flush_workqueue(ata_wq);
>> +
>> +    /*
>> +     * At this point, if a task is running, it's guaranteed to see
>> +     * the FLUSH flag; thus, it will never queue pio tasks again.
>> +     * Cancel and flush.
>> +     */
>> +    tmp |= cancel_delayed_work(&ap->pio_task);
>> +    tmp |= cancel_delayed_work(&ap->packet_task);
>> +    if (!tmp) {
>> +        DPRINTK("flush #2\n");
>> +        flush_workqueue(ata_wq);
>> +    }
>> +
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +    ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
>> +    ap->hsm_task_state = HSM_ST_IDLE;
>> +    spin_unlock_irqrestore(&ap->host_set->lock, flags);
> 
> 
> Mostly ok, except
> 
> 1) forcing hsm_task_state to HSM_ST_IDLE seems unlikely to be correct. 
> The error handling code should already be doing that.

Currently no ->eng_timeout does that.  Unless hsm_task_state is useful 
for error analysis, EH handlers would be doing...

ata_flush_pio_tasks(ap);
ap->hsm_task_state = HSM_ST_IDLE;

Currently, it's only ata_qc_timeout and I'm very okay with both.  Do you 
think resetting hsm_task_state should be done in eng_timeout?

> 2) should break this patch up into two pieces:  add and use new helpers, 
> and add flush code.

Sure.

-- 
tejun

[PATCH 10/12] libata: implement ATA_FLAG_IN_EH port flag

[Tejun Heo]

ATA_FLAG_IN_EH flag is set on entry to EH and cleared on completion.
This patch just sets and clears the flag.  Following patches will
build normal qc execution / EH synchronization aroung this flag.

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

---

 drivers/scsi/libata-scsi.c |    6 ++++++
 include/linux/libata.h     |    2 ++
 2 files changed, 8 insertions(+), 0 deletions(-)

cdc39a8dc6c08611e6317f4dcad2abaa5e027525
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index accb63a..cf54536 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -738,6 +738,8 @@ int ata_scsi_error(struct Scsi_Host *hos
 	DPRINTK("ENTER\n");
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
+	assert(!(ap->flags & ATA_FLAG_IN_EH));
+	ap->flags |= ATA_FLAG_IN_EH;
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
@@ -778,6 +780,10 @@ int ata_scsi_error(struct Scsi_Host *hos
 
 	scsi_eh_flush_done_q(&ap->eh_done_q);
 
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	ap->flags &= ~ATA_FLAG_IN_EH;
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
 	DPRINTK("EXIT\n");
 	return 0;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 576788d..063221d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -162,6 +162,8 @@ enum {
 	ATA_FLAG_PIO_LBA48	= (1 << 13), /* Host DMA engine is LBA28 only */
 	ATA_FLAG_IRQ_MASK	= (1 << 14), /* Mask IRQ in PIO xfers */
 
+	ATA_FLAG_IN_EH		= (1 << 15), /* EH in progress */
+
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
 	ATA_QCFLAG_SINGLE	= (1 << 4), /* no s/g, just a single buffer */
-- 
1.0.8

Re: [PATCH 10/12] libata: implement ATA_FLAG_IN_EH port flag

[Jeff Garzik]

Tejun Heo wrote:
> ATA_FLAG_IN_EH flag is set on entry to EH and cleared on completion.
> This patch just sets and clears the flag.  Following patches will
> build normal qc execution / EH synchronization aroung this flag.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

patches 7, 9, 10 are OK

[PATCH 11/12] libata: ignore normal qc completion during EH

[Tejun Heo]

Don't let interrupt handler and pio task complete qc's while EH is in
progress.  This makes sure that qc's don't go away underneath EH.

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

---

 drivers/scsi/libata-core.c |   32 +++++++++++++++++++-------------
 drivers/scsi/libata-scsi.c |    2 +-
 drivers/scsi/libata.h      |    1 +
 3 files changed, 21 insertions(+), 14 deletions(-)

c72a043f83d46dd4b90a9f689beb5ed9b135fc3a
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index b2f51bb..9556100 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3589,19 +3589,7 @@ void ata_qc_free(struct ata_queued_cmd *
 	}
 }
 
-/**
- *	ata_qc_complete - Complete an active ATA command
- *	@qc: Command to complete
- *	@err_mask: ATA Status register contents
- *
- *	Indicate to the mid and upper layers that an ATA
- *	command has completed, with either an ok or not-ok status.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host_set lock)
- */
-
-void ata_qc_complete(struct ata_queued_cmd *qc)
+void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
@@ -3619,6 +3607,24 @@ void ata_qc_complete(struct ata_queued_c
 	qc->complete_fn(qc);
 }
 
+/**
+ *	ata_qc_complete - Complete an active ATA command
+ *	@qc: Command to complete
+ *	@err_mask: ATA Status register contents
+ *
+ *	Indicate to the mid and upper layers that an ATA
+ *	command has completed, with either an ok or not-ok status.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ */
+
+void ata_qc_complete(struct ata_queued_cmd *qc)
+{
+	if (!(qc->ap->flags & ATA_FLAG_IN_EH))
+		__ata_qc_complete(qc);
+}
+
 static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index cf54536..9e4bc3c 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -801,7 +801,7 @@ static void __ata_eh_qc_complete(struct 
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	qc->scsidone = ata_eh_scsidone;
-	ata_qc_complete(qc);
+	__ata_qc_complete(qc);
 	assert(!ata_tag_valid(qc->tag));
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 9d76923..1cd071a 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -46,6 +46,7 @@ extern struct ata_queued_cmd *ata_qc_new
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 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_complete(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);
-- 
1.0.8

Re: [PATCH 11/12] libata: ignore normal qc completion during EH

[Jeff Garzik]

Tejun Heo wrote:
> Don't let interrupt handler and pio task complete qc's while EH is in
> progress.  This makes sure that qc's don't go away underneath EH.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

I haven't done an in-depth analysis, but this feels like the wrong 
approach:  you've still got drivers writing to qc->err_mask, and 
potentially other struct members.

The better solution is most likely to examine the interrupt handling 
paths of each driver...

	Jeff

Re: [PATCH 11/12] libata: ignore normal qc completion during EH

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Don't let interrupt handler and pio task complete qc's while EH is in
>> progress.  This makes sure that qc's don't go away underneath EH.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
> I haven't done an in-depth analysis, but this feels like the wrong 
> approach:  you've still got drivers writing to qc->err_mask, and 
> potentially other struct members.
> 
> The better solution is most likely to examine the interrupt handling 
> paths of each driver...
> 

No matter what we do to the interrupt handlers, once a qc enters EH, the 
qc is owned by EH and other entities must not be allowed to 
complete/free it.  This patch does not eliminate interferences between 
EH and others but it makes sure that oops doesn't occur due to 
completing qc underneath EH.

This patch + flushing solve the whole problem for PIO tasks.

This patch + proper IRQ masking during EH solve it for interrupt driven 
commands.

-- 
tejun

Re: [PATCHSET] libata: various fixes related to EH

[Jeff Garzik]

Tejun Heo wrote:
> Hello, Jeff, Albert.
> 
> This patchset is composed of 12 patches fixing/updating various EH
> related stuff in libata.  Although not all of the patches are
> logically related, they need to be ordered because they modify
> similar parts of code.
> 
> #01	: cosmetic
> #02	: ata_qc_new/free model
> #03	: ata_qc_issue() error handling fix
> #04	: detailed AC_ERR_* flags
> #05	: return AC_ERR_* from issue functions
> #06-07	: ata_eh_qc_retry/complete
> #08-12	: eh synchronization (#12 is the pio/eh sync patch I talked
> 	  about in the thread "update timer for PIO polling - revised")
> 
> Jeff, these are hopefully more acceptable patches from my recent EH
> work.  I'll soon follow up with more pervasive patches.  My working
> version of new EH now does most things described in ATA EH doc
> including reset, revalidation and gearing down.  I've also ported
> Jen's NCQ support over it, and, although it has a few issues, it's
> generally working okay.

At first glance, all 12 patches look pretty good.  Definitely moving in 
the right direction, though I noticed a few things.  Comments will follow...

	Jeff