[PATCH/RFC 0/3] libata: misc fixes

[PATCH/RFC 0/3] libata: misc fixes

[Albert Lee]

Dear all,

Misc fixes for libata:

1/3: if condition fix for ata_dev_identify(). 
2/3: move err_mask to ata_queued_cmd
3/3: check qc->err_mask for the internal commands

Patch against the mainline tree 
(0b154bb7d0cce80e9c0bcf11d4f9e71b59409d26).

For your review and advice, thanks.

Albert

[PATCH 1/3] libata: if condition fix for ata_dev_identify()

[Albert Lee]

Patch 1/3:
  - if condition fix for ata_dev_identify()
  - ata_pio_poll() minor cleanup.

Changes:
   - Use (dev->class == ATA_DEV_ATA) for ata_dev_identify()
     since "qc->tf.command" has been overwritten by the device status
   - Use HSM_ST_TMOUT directly in ata_pio_poll()

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============
--- linux/drivers/scsi/libata-core.c	2005-11-07 18:25:38.000000000 +0800
+++ identify/drivers/scsi/libata-core.c	2005-11-09 10:14:41.000000000 +0800
@@ -1144,7 +1144,7 @@ retry:
 		 * ATA software reset (SRST, the default) does not appear
 		 * to have this problem.
 		 */
-		if ((using_edd) && (qc->tf.command == ATA_CMD_ID_ATA)) {
+		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
 			u8 err = qc->tf.feature;
 			if (err & ATA_ABORTED) {
 				dev->class = ATA_DEV_ATAPI;
@@ -2747,7 +2747,6 @@ static unsigned long ata_pio_poll(struct
 	u8 status;
 	unsigned int poll_state = HSM_ST_UNKNOWN;
 	unsigned int reg_state = HSM_ST_UNKNOWN;
-	const unsigned int tmout_state = HSM_ST_TMOUT;
 
 	switch (ap->hsm_task_state) {
 	case HSM_ST:
@@ -2768,7 +2767,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)) {
-			ap->hsm_task_state = tmout_state;
+			ap->hsm_task_state = HSM_ST_TMOUT;
 			return 0;
 		}
 		ap->hsm_task_state = poll_state;

[PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd

[Albert Lee]

Patch 2/3:
  move err_mask to ata_queued_cmd.

Changes:
  - Move the err_mask parameter of the complete functions to ata_queued_cmd.
    This can make the err_mask more accessible outside the complete functions;
    also makes the HSM easier to pass err_mask between states.

For your review and advice, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- linux/include/linux/libata.h	2005-11-07 18:25:47.000000000 +0800
+++ err_mask/include/linux/libata.h	2005-11-08 16:43:25.000000000 +0800
@@ -190,7 +190,7 @@ struct ata_port;
 struct ata_queued_cmd;
 
 /* typedefs */
-typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc, unsigned int err_mask);
+typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
 
 struct ata_ioports {
 	unsigned long		cmd_addr;
@@ -275,6 +275,8 @@ struct ata_queued_cmd {
 	/* DO NOT iterate over __sg manually, use ata_for_each_sg() */
 	struct scatterlist	*__sg;
 
+	unsigned int		err_mask;
+
 	ata_qc_cb_t		complete_fn;
 
 	struct completion	*waiting;
@@ -472,7 +474,7 @@ extern void ata_bmdma_start (struct ata_
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
 extern u8   ata_bmdma_status(struct ata_port *ap);
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
-extern void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask);
+extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eng_timeout(struct ata_port *ap);
 extern void ata_scsi_simulate(u16 *id, struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
--- linux/drivers/scsi/libata.h	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/libata.h	2005-11-08 16:31:00.000000000 +0800
@@ -39,7 +39,7 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask);
+extern int ata_qc_complete_noop(struct ata_queued_cmd *qc);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
--- identify/drivers/scsi/libata-core.c	2005-11-09 10:14:41.000000000 +0800
+++ err_mask/drivers/scsi/libata-core.c	2005-11-09 10:15:33.000000000 +0800
@@ -1152,6 +1152,7 @@ retry:
 				qc->cursg_ofs = 0;
 				qc->cursect = 0;
 				qc->nsect = 1;
+				qc->err_mask = 0;
 				goto retry;
 			}
 		}
@@ -2719,7 +2720,7 @@ static int ata_sg_setup(struct ata_queue
  *	None.  (grabs host lock)
  */
 
-void ata_poll_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+void ata_poll_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	unsigned long flags;
@@ -2727,7 +2728,7 @@ void ata_poll_qc_complete(struct ata_que
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	ap->flags &= ~ATA_FLAG_NOINTR;
 	ata_irq_on(ap);
-	ata_qc_complete(qc, err_mask);
+	ata_qc_complete(qc);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 }
 
@@ -2744,10 +2745,14 @@ void ata_poll_qc_complete(struct ata_que
 
 static unsigned long ata_pio_poll(struct ata_port *ap)
 {
+	struct ata_queued_cmd *qc;
 	u8 status;
 	unsigned int poll_state = HSM_ST_UNKNOWN;
 	unsigned int reg_state = HSM_ST_UNKNOWN;
 
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+
 	switch (ap->hsm_task_state) {
 	case HSM_ST:
 	case HSM_ST_POLL:
@@ -2767,6 +2772,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;
 			ap->hsm_task_state = HSM_ST_TMOUT;
 			return 0;
 		}
@@ -2812,18 +2818,19 @@ static int ata_pio_complete (struct ata_
 		}
 	}
 
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
 		ap->hsm_task_state = HSM_ST_ERR;
 		return 0;
 	}
 
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	assert(qc != NULL);
-
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	ata_poll_qc_complete(qc, 0);
+	ata_poll_qc_complete(qc);
 
 	/* another command may start at this point */
 
@@ -3131,6 +3138,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;
 	ap->hsm_task_state = HSM_ST_ERR;
 }
 
@@ -3180,6 +3188,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;
 			ap->hsm_task_state = HSM_ST_ERR;
 			return;
 		}
@@ -3199,7 +3208,7 @@ static void ata_pio_error(struct ata_por
 
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	ata_poll_qc_complete(qc, AC_ERR_ATA_BUS);
+	ata_poll_qc_complete(qc);
 }
 
 static void ata_pio_task(void *_data)
@@ -3322,7 +3331,8 @@ static void ata_qc_timeout(struct ata_qu
 		       ap->id, qc->tf.command, drv_stat, host_stat);
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 
@@ -3420,6 +3430,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 		qc->cursect = qc->cursg = qc->cursg_ofs = 0;
 		qc->nsect = 0;
 		qc->nbytes = qc->curbytes = 0;
+		qc->err_mask = 0;
 
 		ata_tf_init(ap, &qc->tf, dev->devno);
 	}
@@ -3427,7 +3438,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask)
+int ata_qc_complete_noop(struct ata_queued_cmd *qc)
 {
 	return 0;
 }
@@ -3486,7 +3497,7 @@ void ata_qc_free(struct ata_queued_cmd *
  *	spin_lock_irqsave(host_set lock)
  */
 
-void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+void ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	int rc;
 
@@ -3503,7 +3514,7 @@ void ata_qc_complete(struct ata_queued_c
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 
 	/* call completion callback */
-	rc = qc->complete_fn(qc, err_mask);
+	rc = qc->complete_fn(qc);
 
 	/* if callback indicates not to complete command (non-zero),
 	 * return immediately
@@ -3941,7 +3952,8 @@ inline unsigned int ata_host_intr (struc
 		ap->ops->irq_clear(ap);
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc, ac_err_mask(status));
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -4035,13 +4047,17 @@ 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))
-		goto err_out_status;
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
+		goto err_out;
+	}
 
 	/* make sure DRQ is set */
 	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
+	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
 		goto err_out;
+	}
 
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
@@ -4073,10 +4089,8 @@ static void atapi_packet_task(void *_dat
 
 	return;
 
-err_out_status:
-	status = ata_chk_status(ap);
 err_out:
-	ata_poll_qc_complete(qc, __ac_err_mask(status));
+	ata_poll_qc_complete(qc);
 }
 
 
--- linux/drivers/scsi/libata-scsi.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/libata-scsi.c	2005-11-08 16:25:51.000000000 +0800
@@ -1218,12 +1218,11 @@ nothing_to_do:
 	return 1;
 }
 
-static int ata_scsi_qc_complete(struct ata_queued_cmd *qc,
-				unsigned int err_mask)
+static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
- 	int need_sense = (err_mask != 0);
+ 	int need_sense = (qc->err_mask != 0);
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
 	 * user mandated it or if there's an error.  Note that if we
@@ -2016,9 +2015,10 @@ void atapi_request_sense(struct ata_port
 	DPRINTK("EXIT\n");
 }
 
-static int atapi_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+static int atapi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
+	unsigned int err_mask = qc->err_mask;
 
 	VPRINTK("ENTER, err_mask 0x%X\n", err_mask);
 
--- linux/drivers/scsi/ahci.c	2005-11-07 18:25:36.000000000 +0800
+++ err_mask/drivers/scsi/ahci.c	2005-11-08 16:48:47.000000000 +0800
@@ -610,7 +610,8 @@ static void ahci_eng_timeout(struct ata_
 	 	 * not being called from the SCSI EH.
 	 	 */
 		qc->scsidone = scsi_finish_command;
-		ata_qc_complete(qc, AC_ERR_OTHER);
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
 	}
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
@@ -631,15 +632,17 @@ static inline int ahci_host_intr(struct 
 	ci = readl(port_mmio + PORT_CMD_ISSUE);
 	if (likely((ci & 0x1) == 0)) {
 		if (qc) {
-			ata_qc_complete(qc, 0);
+			ata_qc_complete(qc);
 			qc = NULL;
 		}
 	}
 
 	if (status & PORT_IRQ_FATAL) {
 		ahci_intr_error(ap, status);
-		if (qc)
-			ata_qc_complete(qc, AC_ERR_OTHER);
+		if (qc) {
+			qc->err_mask |= AC_ERR_OTHER;
+			ata_qc_complete(qc);
+		}
 	}
 
 	return 1;
--- linux/drivers/scsi/pdc_adma.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/pdc_adma.c	2005-11-08 16:52:12.000000000 +0800
@@ -465,14 +465,12 @@ static inline unsigned int adma_intr_pkt
 			continue;
 		qc = ata_qc_from_tag(ap, ap->active_tag);
 		if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
-			unsigned int err_mask = 0;
-
 			if ((status & (aPERR | aPSD | aUIRQ)))
-				err_mask = AC_ERR_OTHER;
+				qc->err_mask |= AC_ERR_OTHER;
 			else if (pp->pkt[0] != cDONE)
-				err_mask = AC_ERR_OTHER;
+				qc->err_mask |= AC_ERR_OTHER;
 
-			ata_qc_complete(qc, err_mask);
+			ata_qc_complete(qc);
 		}
 	}
 	return handled;
@@ -502,7 +500,8 @@ static inline unsigned int adma_intr_mmi
 		
 				/* complete taskfile transaction */
 				pp->state = adma_state_idle;
-				ata_qc_complete(qc, ac_err_mask(status));
+				qc->err_mask |= ac_err_mask(status);
+				ata_qc_complete(qc);
 				handled = 1;
 			}
 		}
--- linux/drivers/scsi/sata_mv.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/sata_mv.c	2005-11-08 17:09:25.000000000 +0800
@@ -1136,7 +1136,8 @@ static void mv_host_intr(struct ata_host
 				VPRINTK("port %u IRQ found for qc, "
 					"ata_status 0x%x\n", port,ata_status);
 				/* mark qc status appropriately */
-				ata_qc_complete(qc, err_mask);
+				qc->err_mask |= err_mask;
+				ata_qc_complete(qc);
 			}
 		}
 	}
@@ -1312,7 +1313,8 @@ static void mv_eng_timeout(struct ata_po
 	 	 */
 		spin_lock_irqsave(&ap->host_set->lock, flags);
 		qc->scsidone = scsi_finish_command;
-		ata_qc_complete(qc, AC_ERR_OTHER);
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
 }
--- linux/drivers/scsi/sata_promise.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/sata_promise.c	2005-11-08 17:10:54.000000000 +0800
@@ -401,7 +401,8 @@ static void pdc_eng_timeout(struct ata_p
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 		drv_stat = ata_wait_idle(ap);
-		ata_qc_complete(qc, __ac_err_mask(drv_stat));
+		qc->err_mask |= __ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -410,7 +411,8 @@ static void pdc_eng_timeout(struct ata_p
 		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat);
 
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 
@@ -422,21 +424,21 @@ out:
 static inline unsigned int pdc_host_intr( struct ata_port *ap,
                                           struct ata_queued_cmd *qc)
 {
-	unsigned int handled = 0, err_mask = 0;
+	unsigned int handled = 0;
 	u32 tmp;
 	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_GLOBAL_CTL;
 
 	tmp = readl(mmio);
 	if (tmp & PDC_ERR_MASK) {
-		err_mask = AC_ERR_DEV;
+		qc->err_mask |= AC_ERR_DEV;
 		pdc_reset_port(ap);
 	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
-		err_mask |= ac_err_mask(ata_wait_idle(ap));
-		ata_qc_complete(qc, err_mask);
+		qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+		ata_qc_complete(qc);
 		handled = 1;
 		break;
 
--- linux/drivers/scsi/sata_qstor.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/sata_qstor.c	2005-11-08 17:37:19.000000000 +0800
@@ -407,8 +407,8 @@ static inline unsigned int qs_intr_pkt(s
 					case 3: /* device error */
 						pp->state = qs_state_idle;
 						qs_enter_reg_mode(qc->ap);
-						ata_qc_complete(qc,
-							ac_err_mask(sDST));
+						qc->err_mask |= ac_err_mask(sDST);
+						ata_qc_complete(qc);
 						break;
 					default:
 						break;
@@ -445,7 +445,8 @@ static inline unsigned int qs_intr_mmio(
 
 				/* complete taskfile transaction */
 				pp->state = qs_state_idle;
-				ata_qc_complete(qc, ac_err_mask(status));
+				qc->err_mask |= ac_err_mask(status);
+				ata_qc_complete(qc);
 				handled = 1;
 			}
 		}
--- linux/drivers/scsi/sata_sil24.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/sata_sil24.c	2005-11-09 10:33:32.000000000 +0800
@@ -518,7 +518,8 @@ static void sil24_eng_timeout(struct ata
 	 */
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->scsidone = scsi_finish_command;
-	ata_qc_complete(qc, AC_ERR_OTHER);
+	qc->err_mask |= AC_ERR_OTHER;
+	ata_qc_complete(qc);
 
 	sil24_reset_controller(ap);
 }
@@ -567,8 +568,10 @@ static void sil24_error_intr(struct ata_
 		err_mask = AC_ERR_OTHER;
 	}
 
-	if (qc)
-		ata_qc_complete(qc, err_mask);
+	if (qc) {
+		qc->err_mask |= err_mask;
+		ata_qc_complete(qc);
+	}
 
 	sil24_reset_controller(ap);
 }
@@ -592,8 +595,10 @@ static inline void sil24_host_intr(struc
 		 */
 		sil24_update_tf(ap);
 
-		if (qc)
-			ata_qc_complete(qc, ac_err_mask(pp->tf.command));
+		if (qc) {
+			qc->err_mask |= ac_err_mask(pp->tf.command);
+			ata_qc_complete(qc);
+		}
 	} else
 		sil24_error_intr(ap, slot_stat);
 }
--- linux/drivers/scsi/sata_sx4.c	2005-11-07 18:25:38.000000000 +0800
+++ err_mask/drivers/scsi/sata_sx4.c	2005-11-08 17:00:45.000000000 +0800
@@ -718,7 +718,8 @@ static inline unsigned int pdc20621_host
 			VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
-			ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap)));
+			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+			ata_qc_complete(qc);
 			pdc20621_pop_hdma(qc);
 		}
 
@@ -756,7 +757,8 @@ static inline unsigned int pdc20621_host
 			VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
-			ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap)));
+			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+			ata_qc_complete(qc);
 			pdc20621_pop_hdma(qc);
 		}
 		handled = 1;
@@ -766,7 +768,8 @@ static inline unsigned int pdc20621_host
 
 		status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
-		ata_qc_complete(qc, ac_err_mask(status));
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
 		handled = 1;
 
 	} else {
@@ -881,7 +884,8 @@ static void pdc_eng_timeout(struct ata_p
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
-		ata_qc_complete(qc, __ac_err_mask(ata_wait_idle(ap)));
+		qc->err_mask |= __ac_err_mask(ata_wait_idle(ap));
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -890,7 +894,8 @@ static void pdc_eng_timeout(struct ata_p
 		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat);
 
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 

[PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands

[Albert Lee]

Patch 3/3:
  check qc->err_mask for the internal commands.

Problem:
  The results of internal commands issued by libata itself were not checked.

  For example, ata_dev_identify() only checks the device status (qc->tf.command).
  However, checking the device status is not enough to detect all errors.

Changes:
  - check qc->err_mask for the internal commands:
    - ata_dev_identify()
    - ata_dev_set_xfermode()
    - ata_dev_reread_id()
    - ata_dev_init_params() 
    - atapi_request_sense()

The patch accesses qc (and ap) after the qc is completed.
This is bad from the object life cycle point of view and might cause race.
However, since libata has complete control over the hardware when libata issues the internal commands
and it seems noboby else may issue command at the same time, 
accessing qc after ata_qc_complete() looks to be ok here.

p.s. 
     The patch above can also make ata_dev_identify() detect the
     "phantom slave device" problem.

     The "phantom slave device" was seen in specific PATA master-only configuration.
     The master device responds to some commands issued to the slave device, and
     make a illusion that the slave device exists.
     The device status register of the "phantom slave device" always read as zero.
     So, using (qc->tf.command & ATA_ERR) cannot detect the error.
     Checking qc->err_mask can make ata_dev_identify() detect such 
     "phantom slave device" and exclude the phantom device.

For your review and advice, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============
--- err_mask/drivers/scsi/libata-core.c	2005-11-09 10:15:33.000000000 +0800
+++ phantom/drivers/scsi/libata-core.c	2005-11-09 10:15:54.000000000 +0800
@@ -1132,7 +1132,7 @@ retry:
 	ap->ops->tf_read(ap, &qc->tf);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
-	if (qc->tf.command & ATA_ERR) {
+	if (qc->err_mask) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -1144,8 +1144,10 @@ retry:
 		 * ATA software reset (SRST, the default) does not appear
 		 * to have this problem.
 		 */
-		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
+		if ((using_edd) && (dev->class == ATA_DEV_ATA) &&
+		    (qc->tf.command & ATA_ERR)) {
 			u8 err = qc->tf.feature;
+
 			if (err & ATA_ABORTED) {
 				dev->class = ATA_DEV_ATAPI;
 				qc->cursg = 0;
@@ -2267,11 +2269,19 @@ static void ata_dev_set_xfermode(struct 
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	if (rc)
-		ata_port_disable(ap);
-	else
-		wait_for_completion(&wait);
+		goto err_out;
+
+	wait_for_completion(&wait);
+
+	if (qc->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);
 }
 
 /**
@@ -2319,6 +2329,9 @@ static void ata_dev_reread_id(struct ata
 
 	wait_for_completion(&wait);
 
+	if (qc->err_mask)
+		goto err_out;
+
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2327,6 +2340,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);
 }
 
@@ -2371,11 +2385,19 @@ static void ata_dev_init_params(struct a
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	if (rc)
-		ata_port_disable(ap);
-	else
-		wait_for_completion(&wait);
+		goto err_out;
+
+	wait_for_completion(&wait);
+
+	if (qc->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);
 }
 
 /**
--- err_mask/drivers/scsi/libata-scsi.c	2005-11-08 16:25:51.000000000 +0800
+++ phantom/drivers/scsi/libata-scsi.c	2005-11-09 10:37:39.000000000 +0800
@@ -2008,11 +2008,19 @@ void atapi_request_sense(struct ata_port
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	if (rc)
-		ata_port_disable(ap);
-	else
-		wait_for_completion(&wait);
+		goto err_out;
+
+	wait_for_completion(&wait);
+
+	if (qc->err_mask)
+		goto err_out;
 
 	DPRINTK("EXIT\n");
+
+	return;
+err_out:
+	printk(KERN_ERR "ata%u: atapi request sense failed\n", ap->id);
+	ata_port_disable(ap);
 }
 
 static int atapi_qc_complete(struct ata_queued_cmd *qc)

Re: [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd

[Jeff Garzik]

Albert Lee wrote:
> Patch 2/3:
>   move err_mask to ata_queued_cmd.
> 
> Changes:
>   - Move the err_mask parameter of the complete functions to ata_queued_cmd.
>     This can make the err_mask more accessible outside the complete functions;
>     also makes the HSM easier to pass err_mask between states.
> 
> For your review and advice, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

This patch is OK in principle, but I would like it split up some more.

In particular, this patch seems to have mixed in some related changes 
along with the single "more err_mask to ata_queued_cmd" change itself.

	Jeff

Re: [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands

[Jeff Garzik]

Albert Lee wrote:
> Patch 3/3:
>   check qc->err_mask for the internal commands.
> 
> Problem:
>   The results of internal commands issued by libata itself were not checked.
> 
>   For example, ata_dev_identify() only checks the device status (qc->tf.command).
>   However, checking the device status is not enough to detect all errors.
> 
> Changes:
>   - check qc->err_mask for the internal commands:
>     - ata_dev_identify()
>     - ata_dev_set_xfermode()
>     - ata_dev_reread_id()
>     - ata_dev_init_params() 
>     - atapi_request_sense()
> 
> The patch accesses qc (and ap) after the qc is completed.
> This is bad from the object life cycle point of view and might cause race.
> However, since libata has complete control over the hardware when libata issues the internal commands
> and it seems noboby else may issue command at the same time, 
> accessing qc after ata_qc_complete() looks to be ok here.
> 
> p.s. 
>      The patch above can also make ata_dev_identify() detect the
>      "phantom slave device" problem.
> 
>      The "phantom slave device" was seen in specific PATA master-only configuration.
>      The master device responds to some commands issued to the slave device, and
>      make a illusion that the slave device exists.
>      The device status register of the "phantom slave device" always read as zero.
>      So, using (qc->tf.command & ATA_ERR) cannot detect the error.
>      Checking qc->err_mask can make ata_dev_identify() detect such 
>      "phantom slave device" and exclude the phantom device.
> 
> For your review and advice, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

In general this seems like an OK direction to take, but I am a bit 
worried about the object lifetime issues that you mention.  It is IMO 
worth considering a better lifetime strategy at this point, before 
applying this [worthwhile] change.

	Jeff

[PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised)

[Albert Lee]

Dear all,

Description:
  Move the err_mask parameter of the complete functions to ata_queued_cmd.
  This can make the err_mask more accessible outside the complete functions;
  also can make the HSM easier to determine and pass err_mask between states.

patch 1/4: minor patch to pave the road for patch 2 and 3
patch 2/4: move err_mask to ata_queued_cmd
patch 3/4: determine the err_mask when the error is found
patch 4/4: determine the err_mask directly in atapi_packet_task()

Revised patch against the mainline tree 
(e4d76e1c0b15590f2ad9bba89426c2520cd22ca6).

For your review and advice, thanks.

Albert

[PATCH/RFC 1/4] libata: minor patch before moving err_mask

[Albert Lee]

Patch 1/4:
  minor patch before moving the err_mask

Changes:
  - add qc to ata_pio_poll()
  - reorder the initialization of qc in ata_pio_complete()

For your review and advice, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============
--- linux/drivers/scsi/libata-core.c	2005-11-10 11:03:03.000000000 +0800
+++ err_mask1/drivers/scsi/libata-core.c	2005-11-10 11:13:54.000000000 +0800
@@ -2744,10 +2744,14 @@ void ata_poll_qc_complete(struct ata_que
 
 static unsigned long ata_pio_poll(struct ata_port *ap)
 {
+	struct ata_queued_cmd *qc;
 	u8 status;
 	unsigned int poll_state = HSM_ST_UNKNOWN;
 	unsigned int reg_state = HSM_ST_UNKNOWN;
 
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+
 	switch (ap->hsm_task_state) {
 	case HSM_ST:
 	case HSM_ST_POLL:
@@ -2812,15 +2816,15 @@ static int ata_pio_complete (struct ata_
 		}
 	}
 
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
 		ap->hsm_task_state = HSM_ST_ERR;
 		return 0;
 	}
 
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	assert(qc != NULL);
-
 	ap->hsm_task_state = HSM_ST_IDLE;
 
 	ata_poll_qc_complete(qc, 0);

[PATCH/RFC 2/4] libata: move err_mask to ata_queued_cmd

[Albert Lee]

Patch 2/4:
  move err_mask to ata_queued_cmd

Changes:
  - remove err_mask from the parameter list of the complete functions
  - move err_mask to ata_queued_cmd
  - initialize qc->err_mask when needed
  - for each function call to ata_qc_complete(), replace the err_mask parameter with qc->err_mask.

For your review and advice, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- linux/include/linux/libata.h	2005-11-10 11:03:23.000000000 +0800
+++ err_mask2/include/linux/libata.h	2005-11-10 11:08:42.000000000 +0800
@@ -190,7 +190,7 @@ struct ata_port;
 struct ata_queued_cmd;
 
 /* typedefs */
-typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc, unsigned int err_mask);
+typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
 
 struct ata_ioports {
 	unsigned long		cmd_addr;
@@ -275,6 +275,8 @@ struct ata_queued_cmd {
 	/* DO NOT iterate over __sg manually, use ata_for_each_sg() */
 	struct scatterlist	*__sg;
 
+	unsigned int		err_mask;
+
 	ata_qc_cb_t		complete_fn;
 
 	struct completion	*waiting;
@@ -472,7 +474,7 @@ extern void ata_bmdma_start (struct ata_
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
 extern u8   ata_bmdma_status(struct ata_port *ap);
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
-extern void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask);
+extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eng_timeout(struct ata_port *ap);
 extern void ata_scsi_simulate(u16 *id, struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
--- linux/drivers/scsi/libata.h	2005-11-10 11:03:03.000000000 +0800
+++ err_mask2/drivers/scsi/libata.h	2005-11-10 11:09:01.000000000 +0800
@@ -39,7 +39,7 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask);
+extern int ata_qc_complete_noop(struct ata_queued_cmd *qc);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
--- err_mask1/drivers/scsi/libata-core.c	2005-11-10 11:13:54.000000000 +0800
+++ err_mask2/drivers/scsi/libata-core.c	2005-11-10 11:28:51.000000000 +0800
@@ -1152,6 +1152,7 @@ retry:
 				qc->cursg_ofs = 0;
 				qc->cursect = 0;
 				qc->nsect = 1;
+				qc->err_mask = 0;
 				goto retry;
 			}
 		}
@@ -2719,7 +2720,7 @@ static int ata_sg_setup(struct ata_queue
  *	None.  (grabs host lock)
  */
 
-void ata_poll_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+void ata_poll_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	unsigned long flags;
@@ -2727,7 +2728,7 @@ void ata_poll_qc_complete(struct ata_que
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	ap->flags &= ~ATA_FLAG_NOINTR;
 	ata_irq_on(ap);
-	ata_qc_complete(qc, err_mask);
+	ata_qc_complete(qc);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 }
 
@@ -2827,7 +2828,8 @@ static int ata_pio_complete (struct ata_
 
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	ata_poll_qc_complete(qc, 0);
+	assert(qc->err_mask == 0);
+	ata_poll_qc_complete(qc);
 
 	/* another command may start at this point */
 
@@ -3203,7 +3205,8 @@ static void ata_pio_error(struct ata_por
 
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	ata_poll_qc_complete(qc, AC_ERR_ATA_BUS);
+	qc->err_mask |= AC_ERR_ATA_BUS;
+	ata_poll_qc_complete(qc);
 }
 
 static void ata_pio_task(void *_data)
@@ -3326,7 +3329,8 @@ static void ata_qc_timeout(struct ata_qu
 		       ap->id, qc->tf.command, drv_stat, host_stat);
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 
@@ -3424,6 +3428,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 		qc->cursect = qc->cursg = qc->cursg_ofs = 0;
 		qc->nsect = 0;
 		qc->nbytes = qc->curbytes = 0;
+		qc->err_mask = 0;
 
 		ata_tf_init(ap, &qc->tf, dev->devno);
 	}
@@ -3431,7 +3436,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask)
+int ata_qc_complete_noop(struct ata_queued_cmd *qc)
 {
 	return 0;
 }
@@ -3490,7 +3495,7 @@ void ata_qc_free(struct ata_queued_cmd *
  *	spin_lock_irqsave(host_set lock)
  */
 
-void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+void ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	int rc;
 
@@ -3507,7 +3512,7 @@ void ata_qc_complete(struct ata_queued_c
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 
 	/* call completion callback */
-	rc = qc->complete_fn(qc, err_mask);
+	rc = qc->complete_fn(qc);
 
 	/* if callback indicates not to complete command (non-zero),
 	 * return immediately
@@ -3945,7 +3950,8 @@ inline unsigned int ata_host_intr (struc
 		ap->ops->irq_clear(ap);
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc, ac_err_mask(status));
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -4080,7 +4086,8 @@ static void atapi_packet_task(void *_dat
 err_out_status:
 	status = ata_chk_status(ap);
 err_out:
-	ata_poll_qc_complete(qc, __ac_err_mask(status));
+	qc->err_mask |= __ac_err_mask(status);
+	ata_poll_qc_complete(qc);
 }
 
 
--- linux/drivers/scsi/libata-scsi.c	2005-11-10 11:03:03.000000000 +0800
+++ err_mask2/drivers/scsi/libata-scsi.c	2005-11-10 11:25:13.000000000 +0800
@@ -1219,12 +1219,11 @@ nothing_to_do:
 	return 1;
 }
 
-static int ata_scsi_qc_complete(struct ata_queued_cmd *qc,
-				unsigned int err_mask)
+static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
- 	int need_sense = (err_mask != 0);
+ 	int need_sense = (qc->err_mask != 0);
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
 	 * user mandated it or if there's an error.  Note that if we
@@ -2017,9 +2016,10 @@ void atapi_request_sense(struct ata_port
 	DPRINTK("EXIT\n");
 }
 
-static int atapi_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+static int atapi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
+	unsigned int err_mask = qc->err_mask;
 
 	VPRINTK("ENTER, err_mask 0x%X\n", err_mask);
 
--- linux/drivers/scsi/ahci.c	2005-11-10 11:03:03.000000000 +0800
+++ err_mask2/drivers/scsi/ahci.c	2005-11-10 11:32:02.000000000 +0800
@@ -610,7 +610,8 @@ static void ahci_eng_timeout(struct ata_
 	 	 * not being called from the SCSI EH.
 	 	 */
 		qc->scsidone = scsi_finish_command;
-		ata_qc_complete(qc, AC_ERR_OTHER);
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
 	}
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
@@ -631,15 +632,18 @@ static inline int ahci_host_intr(struct 
 	ci = readl(port_mmio + PORT_CMD_ISSUE);
 	if (likely((ci & 0x1) == 0)) {
 		if (qc) {
-			ata_qc_complete(qc, 0);
+			assert(qc->err_mask == 0);
+			ata_qc_complete(qc);
 			qc = NULL;
 		}
 	}
 
 	if (status & PORT_IRQ_FATAL) {
 		ahci_intr_error(ap, status);
-		if (qc)
-			ata_qc_complete(qc, AC_ERR_OTHER);
+		if (qc) {
+			qc->err_mask |= AC_ERR_OTHER;
+			ata_qc_complete(qc);
+		}
 	}
 
 	return 1;
--- linux/drivers/scsi/pdc_adma.c	2005-11-10 11:03:04.000000000 +0800
+++ err_mask2/drivers/scsi/pdc_adma.c	2005-11-10 12:30:16.000000000 +0800
@@ -464,14 +464,12 @@ static inline unsigned int adma_intr_pkt
 			continue;
 		qc = ata_qc_from_tag(ap, ap->active_tag);
 		if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
-			unsigned int err_mask = 0;
-
 			if ((status & (aPERR | aPSD | aUIRQ)))
-				err_mask = AC_ERR_OTHER;
+				qc->err_mask |= AC_ERR_OTHER;
 			else if (pp->pkt[0] != cDONE)
-				err_mask = AC_ERR_OTHER;
+				qc->err_mask |= AC_ERR_OTHER;
 
-			ata_qc_complete(qc, err_mask);
+			ata_qc_complete(qc);
 		}
 	}
 	return handled;
@@ -501,7 +499,8 @@ static inline unsigned int adma_intr_mmi
 		
 				/* complete taskfile transaction */
 				pp->state = adma_state_idle;
-				ata_qc_complete(qc, ac_err_mask(status));
+				qc->err_mask |= ac_err_mask(status);
+				ata_qc_complete(qc);
 				handled = 1;
 			}
 		}
--- linux/drivers/scsi/sata_mv.c	2005-11-10 11:03:04.000000000 +0800
+++ err_mask2/drivers/scsi/sata_mv.c	2005-11-10 11:35:19.000000000 +0800
@@ -1136,7 +1136,8 @@ static void mv_host_intr(struct ata_host
 				VPRINTK("port %u IRQ found for qc, "
 					"ata_status 0x%x\n", port,ata_status);
 				/* mark qc status appropriately */
-				ata_qc_complete(qc, err_mask);
+				qc->err_mask |= err_mask;
+				ata_qc_complete(qc);
 			}
 		}
 	}
@@ -1312,7 +1313,8 @@ static void mv_eng_timeout(struct ata_po
 	 	 */
 		spin_lock_irqsave(&ap->host_set->lock, flags);
 		qc->scsidone = scsi_finish_command;
-		ata_qc_complete(qc, AC_ERR_OTHER);
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
 }
--- linux/drivers/scsi/sata_promise.c	2005-11-10 11:03:04.000000000 +0800
+++ err_mask2/drivers/scsi/sata_promise.c	2005-11-10 12:43:52.000000000 +0800
@@ -401,7 +401,8 @@ static void pdc_eng_timeout(struct ata_p
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 		drv_stat = ata_wait_idle(ap);
-		ata_qc_complete(qc, __ac_err_mask(drv_stat));
+		qc->err_mask |= __ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -410,7 +411,8 @@ static void pdc_eng_timeout(struct ata_p
 		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat);
 
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 
@@ -422,21 +424,21 @@ out:
 static inline unsigned int pdc_host_intr( struct ata_port *ap,
                                           struct ata_queued_cmd *qc)
 {
-	unsigned int handled = 0, err_mask = 0;
+	unsigned int handled = 0;
 	u32 tmp;
 	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_GLOBAL_CTL;
 
 	tmp = readl(mmio);
 	if (tmp & PDC_ERR_MASK) {
-		err_mask = AC_ERR_DEV;
+		qc->err_mask |= AC_ERR_DEV;
 		pdc_reset_port(ap);
 	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
-		err_mask |= ac_err_mask(ata_wait_idle(ap));
-		ata_qc_complete(qc, err_mask);
+		qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+		ata_qc_complete(qc);
 		handled = 1;
 		break;
 
--- linux/drivers/scsi/sata_qstor.c	2005-11-10 11:03:04.000000000 +0800
+++ err_mask2/drivers/scsi/sata_qstor.c	2005-11-10 11:38:29.000000000 +0800
@@ -406,8 +406,8 @@ static inline unsigned int qs_intr_pkt(s
 					case 3: /* device error */
 						pp->state = qs_state_idle;
 						qs_enter_reg_mode(qc->ap);
-						ata_qc_complete(qc,
-							ac_err_mask(sDST));
+						qc->err_mask |= ac_err_mask(sDST);
+						ata_qc_complete(qc);
 						break;
 					default:
 						break;
@@ -444,7 +444,8 @@ static inline unsigned int qs_intr_mmio(
 
 				/* complete taskfile transaction */
 				pp->state = qs_state_idle;
-				ata_qc_complete(qc, ac_err_mask(status));
+				qc->err_mask |= ac_err_mask(status);
+				ata_qc_complete(qc);
 				handled = 1;
 			}
 		}
--- linux/drivers/scsi/sata_sil24.c	2005-11-10 11:03:04.000000000 +0800
+++ err_mask2/drivers/scsi/sata_sil24.c	2005-11-10 11:41:38.000000000 +0800
@@ -518,7 +518,8 @@ static void sil24_eng_timeout(struct ata
 	 */
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->scsidone = scsi_finish_command;
-	ata_qc_complete(qc, AC_ERR_OTHER);
+	qc->err_mask |= AC_ERR_OTHER;
+	ata_qc_complete(qc);
 
 	sil24_reset_controller(ap);
 }
@@ -529,7 +530,6 @@ static void sil24_error_intr(struct ata_
 	struct sil24_port_priv *pp = ap->private_data;
 	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
 	u32 irq_stat, cmd_err, sstatus, serror;
-	unsigned int err_mask;
 
 	irq_stat = readl(port + PORT_IRQ_STAT);
 	writel(irq_stat, port + PORT_IRQ_STAT);		/* clear irq */
@@ -557,18 +557,18 @@ static void sil24_error_intr(struct ata_
 		 * Device is reporting error, tf registers are valid.
 		 */
 		sil24_update_tf(ap);
-		err_mask = ac_err_mask(pp->tf.command);
+		qc->err_mask |= ac_err_mask(pp->tf.command);
 	} else {
 		/*
 		 * Other errors.  libata currently doesn't have any
 		 * mechanism to report these errors.  Just turn on
 		 * ATA_ERR.
 		 */
-		err_mask = AC_ERR_OTHER;
+		qc->err_mask |= AC_ERR_OTHER;
 	}
 
 	if (qc)
-		ata_qc_complete(qc, err_mask);
+		ata_qc_complete(qc);
 
 	sil24_reset_controller(ap);
 }
@@ -592,8 +592,10 @@ static inline void sil24_host_intr(struc
 		 */
 		sil24_update_tf(ap);
 
-		if (qc)
-			ata_qc_complete(qc, ac_err_mask(pp->tf.command));
+		if (qc) {
+			qc->err_mask |= ac_err_mask(pp->tf.command);
+			ata_qc_complete(qc);
+		}
 	} else
 		sil24_error_intr(ap, slot_stat);
 }
--- linux/drivers/scsi/sata_sx4.c	2005-11-10 11:03:04.000000000 +0800
+++ err_mask2/drivers/scsi/sata_sx4.c	2005-11-10 11:43:38.000000000 +0800
@@ -718,7 +718,8 @@ static inline unsigned int pdc20621_host
 			VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
-			ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap)));
+			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+			ata_qc_complete(qc);
 			pdc20621_pop_hdma(qc);
 		}
 
@@ -756,7 +757,8 @@ static inline unsigned int pdc20621_host
 			VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
-			ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap)));
+			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+			ata_qc_complete(qc);
 			pdc20621_pop_hdma(qc);
 		}
 		handled = 1;
@@ -766,7 +768,8 @@ static inline unsigned int pdc20621_host
 
 		status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
-		ata_qc_complete(qc, ac_err_mask(status));
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
 		handled = 1;
 
 	} else {
@@ -881,7 +884,8 @@ static void pdc_eng_timeout(struct ata_p
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
-		ata_qc_complete(qc, __ac_err_mask(ata_wait_idle(ap)));
+		qc->err_mask |= __ac_err_mask(ata_wait_idle(ap));
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -890,7 +894,8 @@ static void pdc_eng_timeout(struct ata_p
 		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat);
 
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 

[PATCH/RFC 3/4] libata: determine the err_mask when the error is found

[Albert Lee]

Patch 3/4:
  determine the err_mask when the error is found

Changes:
  - move "qc->err_mask |= AC_ERR_ATA_BUS" to where the error is found
  - add "assert(qc->err_mask)" to ata_pio_error() to make sure qc->err_mask was available when we enter the error state


For your review and advice, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- err_mask2/drivers/scsi/libata-core.c	2005-11-10 11:28:51.000000000 +0800
+++ err_mask3/drivers/scsi/libata-core.c	2005-11-10 15:09:52.000000000 +0800
@@ -2772,6 +2772,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;
 			ap->hsm_task_state = HSM_ST_TMOUT;
 			return 0;
 		}
@@ -2822,6 +2823,7 @@ static int ata_pio_complete (struct ata_
 
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
 		ap->hsm_task_state = HSM_ST_ERR;
 		return 0;
 	}
@@ -3137,6 +3139,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;
 	ap->hsm_task_state = HSM_ST_ERR;
 }
 
@@ -3186,6 +3189,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;
 			ap->hsm_task_state = HSM_ST_ERR;
 			return;
 		}
@@ -3203,9 +3207,13 @@ static void ata_pio_error(struct ata_por
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	assert(qc != NULL);
 
+	/* make sure qc->err_mask was available to 
+	 * know about what's going wrong and recover
+	 */
+	assert(qc->err_mask);
+
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	qc->err_mask |= AC_ERR_ATA_BUS;
 	ata_poll_qc_complete(qc);
 }
 

[PATCH/RFC 4/4] libata: determine the err_mask directly in atapi_packet_task()

[Albert Lee]

Patch 4/4:
  determine the err_mask directly in atapi_packet_task()

Changes:
  - set qc->err_mask directly when we found the error
  - remove the code to determine err_mask from the device status

For your review and advice, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- err_mask3/drivers/scsi/libata-core.c	2005-11-10 15:09:52.000000000 +0800
+++ err_mask4/drivers/scsi/libata-core.c	2005-11-10 15:10:13.000000000 +0800
@@ -4053,13 +4053,17 @@ 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))
-		goto err_out_status;
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
+		goto err_out;
+	}
 
 	/* make sure DRQ is set */
 	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
+	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
 		goto err_out;
+	}
 
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
@@ -4091,10 +4095,7 @@ static void atapi_packet_task(void *_dat
 
 	return;
 
-err_out_status:
-	status = ata_chk_status(ap);
 err_out:
-	qc->err_mask |= __ac_err_mask(status);
 	ata_poll_qc_complete(qc);
 }
 

Re: [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised)

[Jeff Garzik]

Albert Lee wrote:
> Dear all,
> 
> Description:
>   Move the err_mask parameter of the complete functions to ata_queued_cmd.
>   This can make the err_mask more accessible outside the complete functions;
>   also can make the HSM easier to determine and pass err_mask between states.
> 
> patch 1/4: minor patch to pave the road for patch 2 and 3
> patch 2/4: move err_mask to ata_queued_cmd
> patch 3/4: determine the err_mask when the error is found
> patch 4/4: determine the err_mask directly in atapi_packet_task()
> 
> Revised patch against the mainline tree 
> (e4d76e1c0b15590f2ad9bba89426c2520cd22ca6).

These patches look good, and I'm finally ready to apply them to the 
'upstream' branch.

Would you be kind enough to rediff and resend?  Since I waited a while 
(my fault!), the second patch doesn't seem to apply anymore:


[jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt
4 patch(es) to process.

Applying 'libata: minor patch before moving err_mask'

Wrote tree 6db1429964448401102718bb7ebcb2a7b2efae14
Committed: be7766250023702e65fab822ecf6a8b805028036

Applying 'libata: move err_mask to ata_queued_cmd'

error: patch failed: drivers/scsi/libata-core.c:3424
error: drivers/scsi/libata-core.c: patch does not apply
error: patch failed: drivers/scsi/ahci.c:631
error: drivers/scsi/ahci.c: patch does not apply
error: patch failed: drivers/scsi/sata_mv.c:1136
error: drivers/scsi/sata_mv.c: patch does not apply
error: patch failed: drivers/scsi/sata_sil24.c:557
error: drivers/scsi/sata_sil24.c: patch does not apply

[PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2)

[Albert Lee]

Dear all,

Description:
  Move the err_mask parameter of the complete functions to ata_queued_cmd.
  This can make the err_mask more accessible outside the complete functions;
  also can make the HSM easier to determine and pass err_mask between states.

patch 1/4: minor patch to pave the road for patch 2 and 3
patch 2/4: move err_mask to ata_queued_cmd
patch 3/4: determine the err_mask when the error is found
patch 4/4: determine the err_mask directly in atapi_packet_task()

Revised patch against the upstream branch of libata-dev tree 
(4ef679e6caf1261b6380a610a705a90d7e2738c6).

For your review, thanks.

Albert

[PATCH 1/4] libata: minor patch before moving err_mask

[Albert Lee]

Patch 1/4:
  minor patch before moving the err_mask

Changes:
  - add qc to ata_pio_poll()
  - reorder the initialization of qc in ata_pio_complete()

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

===================

--- upstream/drivers/scsi/libata-core.c	2005-12-05 10:13:48.000000000 +0800
+++ err_mask1/drivers/scsi/libata-core.c	2005-12-05 12:32:33.000000000 +0800
@@ -2802,10 +2802,14 @@ void ata_poll_qc_complete(struct ata_que
 
 static unsigned long ata_pio_poll(struct ata_port *ap)
 {
+	struct ata_queued_cmd *qc;
 	u8 status;
 	unsigned int poll_state = HSM_ST_UNKNOWN;
 	unsigned int reg_state = HSM_ST_UNKNOWN;
 
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+
 	switch (ap->hsm_task_state) {
 	case HSM_ST:
 	case HSM_ST_POLL:
@@ -2870,15 +2874,15 @@ static int ata_pio_complete (struct ata_
 		}
 	}
 
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
 		ap->hsm_task_state = HSM_ST_ERR;
 		return 0;
 	}
 
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	assert(qc != NULL);
-
 	ap->hsm_task_state = HSM_ST_IDLE;
 
 	ata_poll_qc_complete(qc, 0);

[PATCH 2/4] libata: move err_mask to ata_queued_cmd

[Albert Lee]

Patch 2/4:
  move err_mask to ata_queued_cmd

Changes:
  - remove err_mask from the parameter list of the complete functions
  - move err_mask to ata_queued_cmd
  - initialize qc->err_mask when needed
  - for each function call to ata_qc_complete(), replace the err_mask parameter with qc->err_mask.

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

===============

--- upstream/include/linux/libata.h	2005-12-05 10:13:59.000000000 +0800
+++ err_mask2/include/linux/libata.h	2005-12-05 10:30:34.000000000 +0800
@@ -194,7 +194,7 @@ struct ata_port;
 struct ata_queued_cmd;
 
 /* typedefs */
-typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc, unsigned int err_mask);
+typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
 
 struct ata_ioports {
 	unsigned long		cmd_addr;
@@ -279,6 +279,8 @@ struct ata_queued_cmd {
 	/* DO NOT iterate over __sg manually, use ata_for_each_sg() */
 	struct scatterlist	*__sg;
 
+	unsigned int		err_mask;
+
 	ata_qc_cb_t		complete_fn;
 
 	struct completion	*waiting;
@@ -475,7 +477,7 @@ extern void ata_bmdma_start (struct ata_
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
 extern u8   ata_bmdma_status(struct ata_port *ap);
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
-extern void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask);
+extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eng_timeout(struct ata_port *ap);
 extern void ata_scsi_simulate(u16 *id, struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
@@ -667,6 +669,7 @@ static inline void ata_qc_reinit(struct 
 	qc->cursect = qc->cursg = qc->cursg_ofs = 0;
 	qc->nsect = 0;
 	qc->nbytes = qc->curbytes = 0;
+	qc->err_mask = 0;
 
 	ata_tf_init(qc->ap, &qc->tf, qc->dev->devno);
 }
--- upstream/drivers/scsi/libata.h	2005-12-05 10:13:48.000000000 +0800
+++ err_mask2/drivers/scsi/libata.h	2005-12-05 10:26:14.000000000 +0800
@@ -39,7 +39,7 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask);
+extern int ata_qc_complete_noop(struct ata_queued_cmd *qc);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
--- err_mask1/drivers/scsi/libata-core.c	2005-12-05 12:32:33.000000000 +0800
+++ err_mask2/drivers/scsi/libata-core.c	2005-12-05 12:37:14.000000000 +0800
@@ -1053,9 +1053,9 @@ static int ata_qc_wait_err(struct ata_qu
 
 	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
 		/* timeout handling */
-		unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
+		qc->err_mask |= ac_err_mask(ata_chk_status(qc->ap));
 
-		if (!err_mask) {
+		if (!qc->err_mask) {
 			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
 			       qc->ap->id, qc->tf.command);
 		} else {
@@ -1064,7 +1064,7 @@ static int ata_qc_wait_err(struct ata_qu
 			rc = -EIO;
 		}
 
-		ata_qc_complete(qc, err_mask);
+		ata_qc_complete(qc);
 	}
 
 	return rc;
@@ -1175,6 +1175,7 @@ retry:
 				qc->cursg_ofs = 0;
 				qc->cursect = 0;
 				qc->nsect = 1;
+				qc->err_mask = 0;
 				goto retry;
 			}
 		}
@@ -2777,7 +2778,7 @@ skip_map:
  *	None.  (grabs host lock)
  */
 
-void ata_poll_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+void ata_poll_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	unsigned long flags;
@@ -2785,7 +2786,7 @@ void ata_poll_qc_complete(struct ata_que
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	ap->flags &= ~ATA_FLAG_NOINTR;
 	ata_irq_on(ap);
-	ata_qc_complete(qc, err_mask);
+	ata_qc_complete(qc);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 }
 
@@ -2885,7 +2886,8 @@ static int ata_pio_complete (struct ata_
 
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	ata_poll_qc_complete(qc, 0);
+	assert(qc->err_mask == 0);
+	ata_poll_qc_complete(qc);
 
 	/* another command may start at this point */
 
@@ -3261,7 +3263,8 @@ static void ata_pio_error(struct ata_por
 
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	ata_poll_qc_complete(qc, AC_ERR_ATA_BUS);
+	qc->err_mask |= AC_ERR_ATA_BUS;
+	ata_poll_qc_complete(qc);
 }
 
 static void ata_pio_task(void *_data)
@@ -3363,7 +3366,8 @@ static void ata_qc_timeout(struct ata_qu
 		       ap->id, qc->tf.command, drv_stat, host_stat);
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 
@@ -3462,7 +3466,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask)
+int ata_qc_complete_noop(struct ata_queued_cmd *qc)
 {
 	return 0;
 }
@@ -3521,7 +3525,7 @@ void ata_qc_free(struct ata_queued_cmd *
  *	spin_lock_irqsave(host_set lock)
  */
 
-void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+void ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	int rc;
 
@@ -3538,7 +3542,7 @@ void ata_qc_complete(struct ata_queued_c
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 
 	/* call completion callback */
-	rc = qc->complete_fn(qc, err_mask);
+	rc = qc->complete_fn(qc);
 
 	/* if callback indicates not to complete command (non-zero),
 	 * return immediately
@@ -3976,7 +3980,8 @@ inline unsigned int ata_host_intr (struc
 		ap->ops->irq_clear(ap);
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc, ac_err_mask(status));
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -4111,7 +4116,8 @@ static void atapi_packet_task(void *_dat
 err_out_status:
 	status = ata_chk_status(ap);
 err_out:
-	ata_poll_qc_complete(qc, __ac_err_mask(status));
+	qc->err_mask |= __ac_err_mask(status);
+	ata_poll_qc_complete(qc);
 }
 
 
--- upstream/drivers/scsi/libata-scsi.c	2005-12-05 10:13:48.000000000 +0800
+++ err_mask2/drivers/scsi/libata-scsi.c	2005-12-05 12:51:30.000000000 +0800
@@ -1203,12 +1203,11 @@ nothing_to_do:
 	return 1;
 }
 
-static int ata_scsi_qc_complete(struct ata_queued_cmd *qc,
-				unsigned int err_mask)
+static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
- 	int need_sense = (err_mask != 0);
+ 	int need_sense = (qc->err_mask != 0);
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
 	 * user mandated it or if there's an error.  Note that if we
@@ -1955,9 +1954,9 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
 	done(cmd);
 }
 
-static int atapi_sense_complete(struct ata_queued_cmd *qc,unsigned int err_mask)
+static int atapi_sense_complete(struct ata_queued_cmd *qc)
 {
-	if (err_mask && ((err_mask & AC_ERR_DEV) == 0))
+	if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
 		/* FIXME: not quite right; we don't want the
 		 * translation of taskfile registers into
 		 * a sense descriptors, since that's only
@@ -2015,15 +2014,18 @@ static void atapi_request_sense(struct a
 
 	qc->complete_fn = atapi_sense_complete;
 
-	if (ata_qc_issue(qc))
-		ata_qc_complete(qc, AC_ERR_OTHER);
+	if (ata_qc_issue(qc)) {
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
+	}
 
 	DPRINTK("EXIT\n");
 }
 
-static int atapi_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask)
+static int atapi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
+	unsigned int err_mask = qc->err_mask;
 
 	VPRINTK("ENTER, err_mask 0x%X\n", err_mask);
 
--- upstream/drivers/scsi/ahci.c	2005-12-05 10:13:48.000000000 +0800
+++ err_mask2/drivers/scsi/ahci.c	2005-12-05 10:36:41.000000000 +0800
@@ -643,7 +643,8 @@ static void ahci_eng_timeout(struct ata_
 	 	 * not being called from the SCSI EH.
 	 	 */
 		qc->scsidone = scsi_finish_command;
-		ata_qc_complete(qc, AC_ERR_OTHER);
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
 	}
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
@@ -664,7 +665,8 @@ static inline int ahci_host_intr(struct 
 	ci = readl(port_mmio + PORT_CMD_ISSUE);
 	if (likely((ci & 0x1) == 0)) {
 		if (qc) {
-			ata_qc_complete(qc, 0);
+			assert(qc->err_mask == 0);
+			ata_qc_complete(qc);
 			qc = NULL;
 		}
 	}
@@ -681,8 +683,10 @@ static inline int ahci_host_intr(struct 
 		/* command processing has stopped due to error; restart */
 		ahci_restart_port(ap, status);
 
-		if (qc)
-			ata_qc_complete(qc, err_mask);
+		if (qc) {
+			qc->err_mask |= AC_ERR_OTHER;
+			ata_qc_complete(qc);
+		}
 	}
 
 	return 1;
--- upstream/drivers/scsi/pdc_adma.c	2005-12-05 10:13:48.000000000 +0800
+++ err_mask2/drivers/scsi/pdc_adma.c	2005-12-05 13:27:44.000000000 +0800
@@ -464,14 +464,12 @@ static inline unsigned int adma_intr_pkt
 			continue;
 		qc = ata_qc_from_tag(ap, ap->active_tag);
 		if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
-			unsigned int err_mask = 0;
-
 			if ((status & (aPERR | aPSD | aUIRQ)))
-				err_mask = AC_ERR_OTHER;
+				qc->err_mask |= AC_ERR_OTHER;
 			else if (pp->pkt[0] != cDONE)
-				err_mask = AC_ERR_OTHER;
+				qc->err_mask |= AC_ERR_OTHER;
 
-			ata_qc_complete(qc, err_mask);
+			ata_qc_complete(qc);
 		}
 	}
 	return handled;
@@ -501,7 +499,8 @@ static inline unsigned int adma_intr_mmi
 		
 				/* complete taskfile transaction */
 				pp->state = adma_state_idle;
-				ata_qc_complete(qc, ac_err_mask(status));
+				qc->err_mask |= ac_err_mask(status);
+				ata_qc_complete(qc);
 				handled = 1;
 			}
 		}
--- upstream/drivers/scsi/sata_mv.c	2005-12-05 10:13:49.000000000 +0800
+++ err_mask2/drivers/scsi/sata_mv.c	2005-12-05 13:45:41.000000000 +0800
@@ -1242,8 +1242,10 @@ static void mv_host_intr(struct ata_host
 				VPRINTK("port %u IRQ found for qc, "
 					"ata_status 0x%x\n", port,ata_status);
 				/* mark qc status appropriately */
-				if (!(qc->tf.ctl & ATA_NIEN))
-					ata_qc_complete(qc, err_mask);
+				if (!(qc->tf.ctl & ATA_NIEN)) {
+					qc->err_mask |= err_mask;
+					ata_qc_complete(qc);
+				}
 			}
 		}
 	}
@@ -1864,7 +1866,8 @@ static void mv_eng_timeout(struct ata_po
 	 	 */
 		spin_lock_irqsave(&ap->host_set->lock, flags);
 		qc->scsidone = scsi_finish_command;
-		ata_qc_complete(qc, AC_ERR_OTHER);
+		qc->err_mask |= AC_ERR_OTHER;
+		ata_qc_complete(qc);
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
 }
--- upstream/drivers/scsi/sata_promise.c	2005-12-05 10:13:49.000000000 +0800
+++ err_mask2/drivers/scsi/sata_promise.c	2005-12-05 10:43:12.000000000 +0800
@@ -401,7 +401,8 @@ static void pdc_eng_timeout(struct ata_p
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 		drv_stat = ata_wait_idle(ap);
-		ata_qc_complete(qc, __ac_err_mask(drv_stat));
+		qc->err_mask |= __ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -410,7 +411,8 @@ static void pdc_eng_timeout(struct ata_p
 		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat);
 
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 
@@ -422,21 +424,21 @@ out:
 static inline unsigned int pdc_host_intr( struct ata_port *ap,
                                           struct ata_queued_cmd *qc)
 {
-	unsigned int handled = 0, err_mask = 0;
+	unsigned int handled = 0;
 	u32 tmp;
 	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_GLOBAL_CTL;
 
 	tmp = readl(mmio);
 	if (tmp & PDC_ERR_MASK) {
-		err_mask = AC_ERR_DEV;
+		qc->err_mask |= AC_ERR_DEV;
 		pdc_reset_port(ap);
 	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
-		err_mask |= ac_err_mask(ata_wait_idle(ap));
-		ata_qc_complete(qc, err_mask);
+		qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+		ata_qc_complete(qc);
 		handled = 1;
 		break;
 
--- upstream/drivers/scsi/sata_qstor.c	2005-12-05 10:13:49.000000000 +0800
+++ err_mask2/drivers/scsi/sata_qstor.c	2005-12-05 10:45:09.000000000 +0800
@@ -409,8 +409,8 @@ static inline unsigned int qs_intr_pkt(s
 					case 3: /* device error */
 						pp->state = qs_state_idle;
 						qs_enter_reg_mode(qc->ap);
-						ata_qc_complete(qc,
-							ac_err_mask(sDST));
+						qc->err_mask |= ac_err_mask(sDST);
+						ata_qc_complete(qc);
 						break;
 					default:
 						break;
@@ -447,7 +447,8 @@ static inline unsigned int qs_intr_mmio(
 
 				/* complete taskfile transaction */
 				pp->state = qs_state_idle;
-				ata_qc_complete(qc, ac_err_mask(status));
+				qc->err_mask |= ac_err_mask(status);
+				ata_qc_complete(qc);
 				handled = 1;
 			}
 		}
--- upstream/drivers/scsi/sata_sil24.c	2005-12-05 10:13:49.000000000 +0800
+++ err_mask2/drivers/scsi/sata_sil24.c	2005-12-05 13:10:36.000000000 +0800
@@ -654,7 +654,8 @@ static void sil24_eng_timeout(struct ata
 	 */
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->scsidone = scsi_finish_command;
-	ata_qc_complete(qc, AC_ERR_OTHER);
+	qc->err_mask |= AC_ERR_OTHER;
+	ata_qc_complete(qc);
 
 	sil24_reset_controller(ap);
 }
@@ -711,8 +712,10 @@ static void sil24_error_intr(struct ata_
 		sil24_reset_controller(ap);
 	}
 
-	if (qc)
-		ata_qc_complete(qc, err_mask);
+	if (qc) {
+		qc->err_mask |= err_mask;
+		ata_qc_complete(qc);
+	}
 }
 
 static inline void sil24_host_intr(struct ata_port *ap)
@@ -734,8 +737,10 @@ static inline void sil24_host_intr(struc
 		 */
 		sil24_update_tf(ap);
 
-		if (qc)
-			ata_qc_complete(qc, ac_err_mask(pp->tf.command));
+		if (qc) {
+			qc->err_mask |= ac_err_mask(pp->tf.command);
+			ata_qc_complete(qc);
+		}
 	} else
 		sil24_error_intr(ap, slot_stat);
 }
--- upstream/drivers/scsi/sata_sx4.c	2005-12-05 10:13:49.000000000 +0800
+++ err_mask2/drivers/scsi/sata_sx4.c	2005-12-05 10:51:16.000000000 +0800
@@ -718,7 +718,8 @@ static inline unsigned int pdc20621_host
 			VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
-			ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap)));
+			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+			ata_qc_complete(qc);
 			pdc20621_pop_hdma(qc);
 		}
 
@@ -756,7 +757,8 @@ static inline unsigned int pdc20621_host
 			VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
-			ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap)));
+			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
+			ata_qc_complete(qc);
 			pdc20621_pop_hdma(qc);
 		}
 		handled = 1;
@@ -766,7 +768,8 @@ static inline unsigned int pdc20621_host
 
 		status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
-		ata_qc_complete(qc, ac_err_mask(status));
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
 		handled = 1;
 
 	} else {
@@ -881,7 +884,8 @@ static void pdc_eng_timeout(struct ata_p
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
 		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
-		ata_qc_complete(qc, __ac_err_mask(ata_wait_idle(ap)));
+		qc->err_mask |= __ac_err_mask(ata_wait_idle(ap));
+		ata_qc_complete(qc);
 		break;
 
 	default:
@@ -890,7 +894,8 @@ static void pdc_eng_timeout(struct ata_p
 		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat);
 
-		ata_qc_complete(qc, ac_err_mask(drv_stat));
+		qc->err_mask |= ac_err_mask(drv_stat);
+		ata_qc_complete(qc);
 		break;
 	}
 

[PATCH 3/4] libata: determine the err_mask when the error is found

[Albert Lee]

Patch 3/4:
  determine the err_mask when the error is found

Changes:
  - move "qc->err_mask |= AC_ERR_ATA_BUS" to where the error is found
  - add "assert(qc->err_mask)" to ata_pio_error() to make sure qc->err_mask was available when we enter the error state


For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- err_mask2/drivers/scsi/libata-core.c	2005-12-05 12:37:14.000000000 +0800
+++ err_mask3/drivers/scsi/libata-core.c	2005-12-05 12:42:06.000000000 +0800
@@ -2830,6 +2830,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;
 			ap->hsm_task_state = HSM_ST_TMOUT;
 			return 0;
 		}
@@ -2880,6 +2881,7 @@ static int ata_pio_complete (struct ata_
 
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
+		qc->err_mask |= __ac_err_mask(drv_stat);
 		ap->hsm_task_state = HSM_ST_ERR;
 		return 0;
 	}
@@ -3195,6 +3197,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;
 	ap->hsm_task_state = HSM_ST_ERR;
 }
 
@@ -3244,6 +3247,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;
 			ap->hsm_task_state = HSM_ST_ERR;
 			return;
 		}
@@ -3261,9 +3265,13 @@ static void ata_pio_error(struct ata_por
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	assert(qc != NULL);
 
+	/* make sure qc->err_mask is available to 
+	 * know what's wrong and recover
+	 */
+	assert(qc->err_mask);
+
 	ap->hsm_task_state = HSM_ST_IDLE;
 
-	qc->err_mask |= AC_ERR_ATA_BUS;
 	ata_poll_qc_complete(qc);
 }
 

[PATCH 4/4] libata: determine the err_mask directly in atapi_packet_task()

[Albert Lee]

Patch 4/4:
  determine the err_mask directly in atapi_packet_task()

Changes:
  - set qc->err_mask directly when we found the error
  - remove the code to determine err_mask from device status

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- err_mask3/drivers/scsi/libata-core.c	2005-12-05 12:42:06.000000000 +0800
+++ err_mask4/drivers/scsi/libata-core.c	2005-12-05 12:44:28.000000000 +0800
@@ -4083,13 +4083,17 @@ 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))
-		goto err_out_status;
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
+		goto err_out;
+	}
 
 	/* make sure DRQ is set */
 	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
+	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
+		qc->err_mask |= AC_ERR_ATA_BUS;
 		goto err_out;
+	}
 
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
@@ -4121,10 +4125,7 @@ static void atapi_packet_task(void *_dat
 
 	return;
 
-err_out_status:
-	status = ata_chk_status(ap);
 err_out:
-	qc->err_mask |= __ac_err_mask(status);
 	ata_poll_qc_complete(qc);
 }
 

[PATCH/RFC 1/1] libata: err_mask misc fix

[Albert Lee]

Desc:
  Misc follow-up patch for the err_mask patch 1-4.

Changes:
  1. ata_pio_complete(): 
     It seems unnecessary to wait for the clearing of the DRQ bit.
     (Waiting for BSY=0 should be enough.
     ata_ok() also checks the correctness of the status bits later.)

  2. ata_pio_block(): 
     - added error checking, before transfering data.
     - minor comments fix

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- err_mask4/drivers/scsi/libata-core.c	2005-12-05 12:44:28.000000000 +0800
+++ pio_state5/drivers/scsi/libata-core.c	2005-12-06 11:17:14.000000000 +0800
@@ -2865,11 +2865,11 @@ static int ata_pio_complete (struct ata_
 	 * msecs, then chk-status again.  If still busy, fall back to
 	 * HSM_ST_POLL state.
 	 */
-	drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
-	if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
+	drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
+	if (drv_stat & ATA_BUSY) {
 		msleep(2);
-		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
-		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
+		drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
+		if (drv_stat & ATA_BUSY) {
 			ap->hsm_task_state = HSM_ST_LAST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
 			return 0;
@@ -3236,8 +3236,16 @@ static void ata_pio_block(struct ata_por
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	assert(qc != NULL);
 
+	/* check error */
+	if (status & (ATA_ERR | ATA_DF)) {
+		qc->err_mask |= AC_ERR_DEV;
+		ap->hsm_task_state = HSM_ST_ERR;
+		return;
+	}
+
+	/* transfer data if any */
 	if (is_atapi_taskfile(&qc->tf)) {
-		/* no more data to transfer or unsupported ATAPI command */
+		/* DRQ=0 means no more data to transfer */
 		if ((status & ATA_DRQ) == 0) {
 			ap->hsm_task_state = HSM_ST_LAST;
 			return;

Re: [PATCH/RFC 1/1] libata: err_mask misc fix

[Jeff Garzik]

applied patches 1-4, and this fix, to 'upstream'