Re: Playing with SATA NCQ

Jeff Garzik
2005-05-27T07:32:58Z

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

Jens Axboe wrote:
> I double checked this. If you agree to move the setting of QCFLAG_ACTIVE
> _after_ successful ap->ops->qc_issue(qc) and clear it _after_
> __ata_qc_complete(qc) then I can just use that bit and kill
> ATA_QCFLAG_ACCOUNT.
>
> What do you think?

Fine with me.

Keep in mind that the attached patch was applied recently...

Jeff

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1933 bytes --]

author Albert Lee <albertcc@tw.ibm.com> Fri, 29 Apr 2005 17:34:59 +0800
committer Jeff Garzik <jgarzik@pobox.com> Mon, 16 May 2005 02:46:59 -0400

[PATCH] libata: Prevent the interrupt handler from completing a command twice

Problem:
During the libata CD-ROM stress test, sometimes the "BUG: timeout
without command" error is seen.

Root cause:
Unexpected interrupt occurs after the ata_qc_complete() is called,
but before the SCSI error handler. The interrupt handler is invoked
before the SCSI error handler, and it clears the command by calling
ata_qc_complete() again. Later when the SCSI error handler is run,
the ata_queued_cmd is already gone, causing the "BUG: timeout without
command" error.

Changes:
- Use the ATA_QCFLAG_ACTIVE flag to prevent the interrupt handler
from completing the command twice, before the scsi_error_handler.

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

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2539,7 +2539,7 @@ static void atapi_request_sense(struct a
ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
qc->dma_dir = DMA_FROM_DEVICE;

- memset(&qc->cdb, 0, sizeof(ap->cdb_len));
+ memset(&qc->cdb, 0, ap->cdb_len);
qc->cdb[0] = REQUEST_SENSE;
qc->cdb[4] = SCSI_SENSE_BUFFERSIZE;

@@ -2811,6 +2811,7 @@ void ata_qc_complete(struct ata_queued_c

/* call completion callback */
rc = qc->complete_fn(qc, drv_stat);
+ qc->flags &= ~ATA_QCFLAG_ACTIVE;

/* if callback indicates not to complete command (non-zero),
* return immediately
@@ -3229,7 +3230,8 @@ irqreturn_t ata_interrupt (int irq, void
struct ata_queued_cmd *qc;

qc = ata_qc_from_tag(ap, ap->active_tag);
- if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+ if (qc && (!(qc->tf.ctl & ATA_NIEN)) &&
+ (qc->flags & ATA_QCFLAG_ACTIVE))
handled |= ata_host_intr(ap, qc);
}
}