ATAPI error handling work

This is a work-in-progress, attempting to fix the death of libata
whenever there is a SCSI error thrown to us... IOW this fixes libata
EH to properly fake autosensing, rather than kicking over to the SCSI EH thread for that purpose.

drivers/scsi/libata-core.c +7 -0

@@ -3174,17 +3106,35 @@ out:
 void ata_eng_timeout(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
+	struct ata_host_set *host_set = ap->host_set;
+	unsigned long flags;
+	int printed_message = 0;
 
 	DPRINTK("ENTER\n");
 
+	spin_lock_irqsave(&host_set->lock, flags);
+	while (ap->flags & ATA_FLAG_IN_EH) {
+		spin_unlock_irqrestore(&host_set->lock, flags);
+

drivers/scsi/libata-core.c +2 -1

 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
+	if (qc)
+		ata_qc_timeout(qc);

drivers/scsi/libata-scsi.c +16 -2

@@ -225,7 +225,7 @@ void ata_to_sense_error(struct ata_queue
 	};
 	int i = 0;
 
-	cmd->result = SAM_STAT_CHECK_CONDITION;
+	cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
 	/*
 	 *	Is this an error we can process/parse

@@ -1468,7 +1468,7 @@ unsigned int ata_scsiop_report_luns(stru
 void ata_scsi_badcmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *), u8 asc, u8 ascq)
 {
 	DPRINTK("ENTER\n");
-	cmd->result = SAM_STAT_CHECK_CONDITION;
+	cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
 	cmd->sense_buffer[0] = 0x70;
 	cmd->sense_buffer[2] = ILLEGAL_REQUEST;

@@ -1479,28 +1479,81 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
 	done(cmd);
 }
 
+static int atapi_qc_complete_sense(struct ata_queued_cmd *qc, u8 drv_stat)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+
+	if (ata_ok(drv_stat))
+		cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
+	else
+		ata_to_sense_error(qc, drv_stat);
+
+	qc->ap->flags &= ~ATA_FLAG_IN_EH;
+	qc->scsidone(cmd);
+	return 0;
+}
+

drivers/scsi/libata-scsi.c +27 -0

+
+	ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
+	qc->dma_dir = DMA_FROM_DEVICE;
+
+	memset(&qc->cdb, 0, ap->cdb_len);
+	qc->cdb[0] = REQUEST_SENSE;
+	qc->cdb[4] = SCSI_SENSE_BUFFERSIZE;
+
+	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	qc->tf.command = ATA_CMD_PACKET;
+
+	qc->tf.protocol = ATA_PROT_ATAPI;
+	qc->tf.lbam = (8 * 1024) & 0xff;
+	qc->tf.lbah = (8 * 1024) >> 8;
+	qc->nbytes = SCSI_SENSE_BUFFERSIZE;
+
+	qc->complete_fn = atapi_qc_complete_sense;
+
+	rc = ata_qc_issue(qc);
+
+	if (rc) {
+		/* FIXME: complete failing command with an error */
+		ata_port_disable(ap);
+	}
+
+	/* FIXME: add timer for timeout of this command */
+	/* control flow continues in atapi_qc_complete_sense() */

drivers/scsi/libata-scsi.c +10 -13

+	if (ap->host->eh_active)
+		return 1;
+
 	if (unlikely(drv_stat & (ATA_BUSY | ATA_DRQ)))
 		ata_to_sense_error(qc, drv_stat);
+
 	else if (unlikely(drv_stat & ATA_ERR)) {
 		DPRINTK("request check condition\n");
-
-		/* FIXME: command completion with check condition
-		 * but no sense causes the error handler to run,
-		 * which then issues REQUEST SENSE, fills in the sense 
-		 * buffer, and completes the command (for the second
-		 * time).  We need to issue REQUEST SENSE some other
-		 * way, to avoid completing the command twice.
-		 */
-		cmd->result = SAM_STAT_CHECK_CONDITION;
-
-		qc->scsidone(cmd);
-
+		ap->flags |= ATA_FLAG_IN_EH;
+		atapi_request_sense(qc);
 		return 1;
-	} else {
+	}
+
+	else {
 		u8 *scsicmd = cmd->cmnd;
 
 		if (scsicmd[0] == INQUIRY) {

@@ -1535,6 +1588,7 @@ static int atapi_qc_complete(struct ata_
 
 	return 0;
 }
+
 /**
  *	atapi_xlat - Initialize PACKET taskfile
  *	@qc: command structure to be initialized

drivers/scsi/libata.h +0 -0

Hello, Jeff.

Quoted 2 lines · Jeff Garzik

This is a work-in-progress, attempting to fix the death of libata whenever there is a SCSI error thrown to us... IOW this fixes libata EH to properly fake autosensing, rather than kicking over to the SCSI EH thread for that purpose.

Great. I just have a few questions. Oh.. BTW, against which head is
the patch generated? I couldn't get it applied cleanly to either upstream or ALL.

Quoted 14 lines

(quoted diff) +7 -0

@@ -3174,17 +3106,35 @@ out:
 void ata_eng_timeout(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
+	struct ata_host_set *host_set = ap->host_set;
+	unsigned long flags;
+	int printed_message = 0;
 
 	DPRINTK("ENTER\n");
 
+	spin_lock_irqsave(&host_set->lock, flags);
+	while (ap->flags & ATA_FLAG_IN_EH) {
+		spin_unlock_irqrestore(&host_set->lock, flags);
+

Review out: · line 3128

Quoted 10 lines

(quoted diff) +10 -0

+		if (!printed_message++)
+			printk(KERN_INFO "ata%u: waiting in timeout handler for EH\n",
+				ap->id);
+
+		msleep(250);
+
+		spin_lock_irqsave(&host_set->lock, flags);
+	}
+	spin_unlock_irqrestore(&host_set->lock, flags);
+

I'm not really sure if synchronization around reqsense with
ATA_FLAG_IN_EH is such a good idea for the following reasons.

* IMHO, tugging in request sense into the timeout window of the original command is okay (or even better).

* As long as we grab host_set lock while submitting request sense, which we already does, the synchronization can be done safely in EH proper. libata EH isn't currently ready for this but other commands are dealt the same way and fixing EH is the correct way, IMHO.

Note that we really don't have to do anything special for timing out
during request sense. Once we fix EH/interrupt handler race, it can be handled the same way without any speical casing.

Quoted 12 lines

(quoted diff) +3 -2

 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
+	if (qc)
+		ata_qc_timeout(qc);
+	else {
 		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
 		       ap->id);
 		goto out;
 	}
 
-	ata_qc_timeout(qc);
 

Quoted 2 lines

out:

	DPRINTK("EXIT\n");

I think you were trying to kill goto out and the label by moving
ata_qc_timeout() into if condition, no? Currently, the goto out seem a bit funny.

Quoted 48 lines

drivers/scsi/libata-scsi.c +25 -2

@@ -225,7 +225,7 @@ void ata_to_sense_error(struct ata_queue
 	};
 	int i = 0;
 
-	cmd->result = SAM_STAT_CHECK_CONDITION;
+	cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
 	/*
 	 *	Is this an error we can process/parse

@@ -1468,7 +1468,7 @@ unsigned int ata_scsiop_report_luns(stru
 void ata_scsi_badcmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *), u8 asc, u8 ascq)
 {
 	DPRINTK("ENTER\n");
-	cmd->result = SAM_STAT_CHECK_CONDITION;
+	cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
 	cmd->sense_buffer[0] = 0x70;
 	cmd->sense_buffer[2] = ILLEGAL_REQUEST;

@@ -1479,28 +1479,81 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
 	done(cmd);
 }
 
+static int atapi_qc_complete_sense(struct ata_queued_cmd *qc, u8 drv_stat)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+
+	if (ata_ok(drv_stat))
+		cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
+	else
+		ata_to_sense_error(qc, drv_stat);
+
+	qc->ap->flags &= ~ATA_FLAG_IN_EH;
+	qc->scsidone(cmd);
+	return 0;
+}
+
+static void atapi_request_sense(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	int rc;
+
+	DPRINTK("ATAPI request sense\n");
+
+	/* FIXME: is this needed? */

drivers/scsi/libata-scsi.c void ata_scsi_badcmd(struct scsi_cmnd *c · line 1505

Quoted 1 line

drivers/scsi/libata-scsi.c +1 -0

+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));

This is just a side note as this part seems to be just copied from
the original request_sense function. Anyways, I think this isn't needed anymore. SCSI ML seems to clear things pretty thoroughly these days.

Quoted 35 lines

drivers/scsi/libata-scsi.c +31 -0

+	ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
+	qc->dma_dir = DMA_FROM_DEVICE;
+
+	memset(&qc->cdb, 0, ap->cdb_len);
+	qc->cdb[0] = REQUEST_SENSE;
+	qc->cdb[4] = SCSI_SENSE_BUFFERSIZE;
+
+	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	qc->tf.command = ATA_CMD_PACKET;
+
+	qc->tf.protocol = ATA_PROT_ATAPI;
+	qc->tf.lbam = (8 * 1024) & 0xff;
+	qc->tf.lbah = (8 * 1024) >> 8;
+	qc->nbytes = SCSI_SENSE_BUFFERSIZE;
+
+	qc->complete_fn = atapi_qc_complete_sense;
+
+	rc = ata_qc_issue(qc);
+
+	if (rc) {
+		/* FIXME: complete failing command with an error */
+		ata_port_disable(ap);
+	}
+
+	/* FIXME: add timer for timeout of this command */
+	/* control flow continues in atapi_qc_complete_sense() */
+
+	DPRINTK("EXIT\n");
+}
+
 static int atapi_qc_complete(struct ata_queued_cmd *qc, u8 drv_stat)
 {
+	struct ata_port *ap = qc->ap;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 

drivers/scsi/libata-scsi.c void ata_scsi_badcmd(struct scsi_cmnd *c · line 1541

Quoted 1 line

drivers/scsi/libata-scsi.c +1 -0

+	rmb();

I'm having difficult time understanding this rmb(). What is it
protecting against?

Tejun Heo wrote:

Quoted 4 lines

I'm not really sure if synchronization around reqsense with
ATA_FLAG_IN_EH is such a good idea for the following reasons.

Quoted 2 lines

* IMHO, tugging in request sense into the timeout window of the original command is okay (or even better).

Not really: we have to assume that time passed, so by relying on the SCSI EH timeout rather than a timer, the timeout is suddenly effectively random. I want a definitive time length, since its likely REQUEST SENSE may need a longer timeout for some devices.

Second, the timeout for READ/WRITE commands is often smaller than it is for non-READ/WRITE commands. We want much to ensure that we have sufficient time to handle REQUEST SENSE.

Quoted 5 lines

* As long as we grab host_set lock while submitting request sense, which we already does, the synchronization can be done safely in EH proper. libata EH isn't currently ready for this but other commands are dealt the same way and fixing EH is the correct way, IMHO.

We grab the lock submitting request sense, but then we release it. ata_eng_timeout() could get called before the entire REQUEST SENSE process is completed.

Quoted 3 lines

Note that we really don't have to do anything special for timing out
during request sense. Once we fix EH/interrupt handler race, it can be handled the same way without any speical casing.

Yes, this is feasible. We can simply wait for the timer (which doesn't exist yet) or atapi_qc_complete_sense() to clear the ATA_FLAG_IN_EH bit, at which point the EH timer takes over, or will do so shortly.

Quoted 19 lines

Quoted 13 lines

    qc = ata_qc_from_tag(ap, ap->active_tag);

(quoted diff) +3 -2

-    if (!qc) {
+    if (qc)
+        ata_qc_timeout(qc);
+    else {
        printk(KERN_ERR "ata%u: BUG: timeout without command\n",
               ap->id);
        goto out;
    }
 
-    ata_qc_timeout(qc);

Quoted 2 lines

out:

    DPRINTK("EXIT\n");

Quoted 3 lines

I think you were trying to kill goto out and the label by moving
ata_qc_timeout() into if condition, no? Currently, the goto out seem a bit funny.

The goto is gone in the most recent version :)

Quoted 54 lines

Quoted 40 lines

drivers/scsi/libata-scsi.c +16 -2

@@ -225,7 +225,7 @@ void ata_to_sense_error(struct ata_queue
    };
    int i = 0;
 
-    cmd->result = SAM_STAT_CHECK_CONDITION;
+    cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
    /*
     *    Is this an error we can process/parse

@@ -1468,7 +1468,7 @@ unsigned int ata_scsiop_report_luns(stru
 void ata_scsi_badcmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *), u8 asc, u8 ascq)
 {
    DPRINTK("ENTER\n");
-    cmd->result = SAM_STAT_CHECK_CONDITION;
+    cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
    cmd->sense_buffer[0] = 0x70;
    cmd->sense_buffer[2] = ILLEGAL_REQUEST;

@@ -1479,28 +1479,81 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
    done(cmd);
 }
 
+static int atapi_qc_complete_sense(struct ata_queued_cmd *qc, u8 
 drv_stat)
+{
+    struct scsi_cmnd *cmd = qc->scsicmd;
+
+    if (ata_ok(drv_stat))
+        cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
+    else
+        ata_to_sense_error(qc, drv_stat);
+
+    qc->ap->flags &= ~ATA_FLAG_IN_EH;
+    qc->scsidone(cmd);
+    return 0;
+}
+

drivers/scsi/libata-scsi.c void ata_scsi_badcmd(struct scsi_cmnd *c · line 1506

Quoted 10 lines

drivers/scsi/libata-scsi.c +10 -0

+static void atapi_request_sense(struct ata_queued_cmd *qc)
+{
+    struct ata_port *ap = qc->ap;
+    struct scsi_cmnd *cmd = qc->scsicmd;
+    int rc;
+
+    DPRINTK("ATAPI request sense\n");
+
+    /* FIXME: is this needed? */
+    memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));

Quoted 4 lines

This is just a side note as this part seems to be just copied from
the original request_sense function. Anyways, I think this isn't needed anymore. SCSI ML seems to clear things pretty thoroughly these days.

Well, until I have the line of code pointed to me, its not important. Its not the hot path, so we can afford to do possibly-redundant work.

Quoted 36 lines

Quoted 36 lines

drivers/scsi/libata-scsi.c +31 -0

+    ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
+    qc->dma_dir = DMA_FROM_DEVICE;
+
+    memset(&qc->cdb, 0, ap->cdb_len);
+    qc->cdb[0] = REQUEST_SENSE;
+    qc->cdb[4] = SCSI_SENSE_BUFFERSIZE;
+
+    qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+    qc->tf.command = ATA_CMD_PACKET;
+
+    qc->tf.protocol = ATA_PROT_ATAPI;
+    qc->tf.lbam = (8 * 1024) & 0xff;
+    qc->tf.lbah = (8 * 1024) >> 8;
+    qc->nbytes = SCSI_SENSE_BUFFERSIZE;
+
+    qc->complete_fn = atapi_qc_complete_sense;
+
+    rc = ata_qc_issue(qc);
+
+    if (rc) {
+        /* FIXME: complete failing command with an error */
+        ata_port_disable(ap);
+    }
+
+    /* FIXME: add timer for timeout of this command */
+    /* control flow continues in atapi_qc_complete_sense() */
+
+    DPRINTK("EXIT\n");
+}
+
 static int atapi_qc_complete(struct ata_queued_cmd *qc, u8 drv_stat)
 {
+    struct ata_port *ap = qc->ap;
    struct scsi_cmnd *cmd = qc->scsicmd;
 

Quoted 7 lines

drivers/scsi/libata-scsi.c void atapi_request_sense(struct ata_port · line 1549

Quoted 1 line

drivers/scsi/libata-scsi.c +1 -0

+    rmb();

I'm having difficult time understanding this rmb(). What is it
protecting against?

Just paranoia, before reading ap->host->eh_active, which is tweaked by scsi_error.c.

As I wrote in the libata EH doc and associated thread, I don't think
doing autosensing-by-turning-qc-around is a very good idea. I hope I can make some point here.

In the other thread, you wrote that the current ATAPI sense requisition
is corrupt because it finishes scmd twice and I believe this patch was implemented to fix that. I disagreed with you in that thread and gave my rationale for that. If I was wrong in that thread, please ignore what follows.