[PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Tejun Heo]

 Hello, Jeff.
 Hello, Jens.

 This is the fource, no, fourth (dang, that starwars) of six misc
updates to ncq.

 This patch rewrites __ata_qc_complete() to make it look more sane.
Also, remove spurious clearing of ATA_QCFLAG_ACTIVE from
ata_qc_complete().

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

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-06-26 21:20:43.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-06-26 21:20:47.000000000 +0900
@@ -3069,7 +3069,8 @@ static int ata_qc_complete_noop(struct a
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	unsigned int tag, do_clear = 0;
+	struct completion *waiting = qc->waiting;
+	unsigned int tag = ata_qc_to_tag(qc);
 
 	if (likely(qc->flags & ATA_QCFLAG_ACTIVE)) {
 		assert(ap->queue_depth);
@@ -3077,26 +3078,19 @@ static void __ata_qc_complete(struct ata
 
 		if (!ap->queue_depth)
 			ap->flags &= ~ATA_FLAG_NCQ_QUEUED;
-	}
-
-	qc->flags = 0;
-	tag = ata_qc_to_tag(qc);
-	if (likely(ata_tag_valid(tag))) {
 		if (tag == ap->active_tag)
 			ap->active_tag = ATA_TAG_POISON;
-		do_clear = 1;
 	}
 
-	if (qc->waiting) {
-		struct completion *waiting = qc->waiting;
-		qc->waiting = NULL;
-		complete(waiting);
-	}
+	qc->flags = 0;
+	qc->waiting = NULL;
+	ap->qactive &= ~(1 << tag);
 
-	if (likely(do_clear))
-		ap->qactive &= ~(1 << tag);
 	if (ap->cmd_waiters)
 		wake_up(&ap->cmd_wait_queue);
+
+	if (waiting)
+		complete(waiting);
 }
 
 /**
@@ -3151,7 +3145,6 @@ void ata_qc_complete(struct ata_queued_c
 		return;
 
 	__ata_qc_complete(qc);
-	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 
 	VPRINTK("EXIT\n");
 }

Re: [PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Jeff Garzik]

Tejun Heo wrote:
>  Hello, Jeff.
>  Hello, Jens.
> 
>  This is the fource, no, fourth (dang, that starwars) of six misc
> updates to ncq.
> 
>  This patch rewrites __ata_qc_complete() to make it look more sane.
> Also, remove spurious clearing of ATA_QCFLAG_ACTIVE from
> ata_qc_complete().
> 
>  Signed-off-by: Tejun Heo <htejun@gmail.com>

I'll have to stare at this a bit more.  This has big potential for 
creating races if not perfect.

Re: [PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Jens Axboe]

On Sun, Jun 26 2005, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Hello, Jeff.
> > Hello, Jens.
> >
> > This is the fource, no, fourth (dang, that starwars) of six misc
> >updates to ncq.
> >
> > This patch rewrites __ata_qc_complete() to make it look more sane.
> >Also, remove spurious clearing of ATA_QCFLAG_ACTIVE from
> >ata_qc_complete().
> >
> > Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> I'll have to stare at this a bit more.  This has big potential for 
> creating races if not perfect.

It looks good to me.

-- 
Jens Axboe

Re: [PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Albert Lee]

Tejun Heo wrote:

>Also, remove spurious clearing of ATA_QCFLAG_ACTIVE from
>ata_qc_complete().
> 
Hi Tejun:

Clearing the ATA_QCFLAG_ACTIVE in ata_qc_complete() is needed. We have to
prevent the interrupt handler from racing with the error handler in ATAPI.
(Please see the following link for the detail:
http://marc.theaimsgroup.com/?l=linux-ide&m=111476734016857&w=2)

Albert

Re: [PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Tejun Heo]

Albert Lee wrote:
> 
> Tejun Heo wrote:
> 
>> Also, remove spurious clearing of ATA_QCFLAG_ACTIVE from
>> ata_qc_complete().
>>
> Hi Tejun:
> 
> Clearing the ATA_QCFLAG_ACTIVE in ata_qc_complete() is needed. We have to
> prevent the interrupt handler from racing with the error handler in ATAPI.
> (Please see the following link for the detail:
> http://marc.theaimsgroup.com/?l=linux-ide&m=111476734016857&w=2)
> 
> Albert
> 

  Hello, Albert.

  Ah... I see.  Sorry, I didn't check the ATAPI code (Is any SATA ATAPI 
device selling in retail shops?  I want one for testing but cannot find 
one).  I'll remove that removal ;-) and add some comment there to 
clarify things.  it also means that ATAPI handling of my NCQ helpers is 
broken

  BTW, now that I've read how libata defers command completion to EH 
using qc_complete return value, I have a question.  While I was writing 
NCQ EH, the biggest problem was that we're called differently from 
several places and EH conventions differ in each case.

  1. from libata core: we don't have associated SCSI command and EH 
won't be entered.
  2. from SCSI midlayer (SCSI special cmds): we have associated SCSI 
command but EH won't be entered on failure (scsidone is overrided).
  3. from blk via SCSI (SCSI normal cmds): we have associated SCSI 
command and EH will be entered on failure.

  I couldn't defer qc-completion on error interrupt to EH, as I don't 
know whether it will be entered or not.  So, NCQ helpers always 
qc-completes commands and NCQ EH helper deals with what's left.  It 
seems that the current ATAPI code doesn't account for these differences 
when handling errors.  Is EH-entrance guaranteed on ATAPI failures?

  TIA.

-- 
tejun

Re: [PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Jeff Garzik]

Tejun Heo wrote:
>  Ah... I see.  Sorry, I didn't check the ATAPI code (Is any SATA ATAPI 
> device selling in retail shops?  I want one for testing but cannot find 
> one).  I'll remove that removal ;-) and add some comment there to 
> clarify things.  it also means that ATAPI handling of my NCQ helpers is 
> broken


Yep, SATA ATAPI is selling in retail shops now.

	Jeff

Re: [PATCH] libata-dev-2.6-ncq: rewrite __ata_qc_complete

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>  Ah... I see.  Sorry, I didn't check the ATAPI code (Is any SATA ATAPI 
>> device selling in retail shops?  I want one for testing but cannot 
>> find one).  I'll remove that removal ;-) and add some comment there to 
>> clarify things.  it also means that ATAPI handling of my NCQ helpers 
>> is broken
> 
> Yep, SATA ATAPI is selling in retail shops now.
> 
>     Jeff

  Yay, just found Plextor PX-716SA in an online shop.  I think it's the 
only model available here. :-)

  Thanks.

-- 
tejun