[RFC] aic7xxx - ahc_done check SCB_ACTIVE for tagged transactions

[RFC] aic7xxx - ahc_done check SCB_ACTIVE for tagged transactions

[David Milburn]

Hannes,

Does ahc_done need to check the SCB_ACTIVE flag only if the SCB
is not in the untagged queue before panic?

If the driver is in error recovery, you may end panic'ing
on a TUR that is in the untagged queue.

Attempting to queue an ABORT message
CDB: 0x0 0x0 0x0 0x0 0x0 0x0
SCB 3 done'd twice

This patch is included in Adaptec's 6.3.11 driver on their 
website.

Thank you,
David

--- scsi-misc-2.6.git/drivers/scsi/aic7xxx/aic7xxx_osm.c.abort
+++ scsi-misc-2.6.git/drivers/scsi/aic7xxx/aic7xxx_osm.c
@@ -1658,9 +1658,12 @@ ahc_done(struct ahc_softc *ahc, struct s
 		untagged_q = &(ahc->untagged_queues[target_offset]);
 		TAILQ_REMOVE(untagged_q, scb, links.tqe);
 		BUG_ON(!TAILQ_EMPTY(untagged_q));
-	}
-
-	if ((scb->flags & SCB_ACTIVE) == 0) {
+	} else if ((scb->flags & SCB_ACTIVE) == 0) {
+		/*
+		 * Transactions aborted from the untagged queue may
+		 * not have been dispatched to the controller, so
+		 * only check the SCB_ACTIVE flag for tagged transactions.
+		 */
 		printf("SCB %d done'd twice\n", scb->hscb->tag);
 		ahc_dump_card_state(ahc);
 		panic("Stopping for safety");

Re: [RFC] aic7xxx - ahc_done check SCB_ACTIVE for tagged transactions

[Hannes Reinecke]

David Milburn wrote:
> Hannes,
> 
> Does ahc_done need to check the SCB_ACTIVE flag only if the SCB
> is not in the untagged queue before panic?
> 
> If the driver is in error recovery, you may end panic'ing
> on a TUR that is in the untagged queue.
> 
> Attempting to queue an ABORT message
> CDB: 0x0 0x0 0x0 0x0 0x0 0x0
> SCB 3 done'd twice
> 
> This patch is included in Adaptec's 6.3.11 driver on their 
> website.
> 
> Thank you,
> David
> 
> --- scsi-misc-2.6.git/drivers/scsi/aic7xxx/aic7xxx_osm.c.abort
> +++ scsi-misc-2.6.git/drivers/scsi/aic7xxx/aic7xxx_osm.c
> @@ -1658,9 +1658,12 @@ ahc_done(struct ahc_softc *ahc, struct s
>  		untagged_q = &(ahc->untagged_queues[target_offset]);
>  		TAILQ_REMOVE(untagged_q, scb, links.tqe);
>  		BUG_ON(!TAILQ_EMPTY(untagged_q));
> -	}
> -
> -	if ((scb->flags & SCB_ACTIVE) == 0) {
> +	} else if ((scb->flags & SCB_ACTIVE) == 0) {
> +		/*
> +		 * Transactions aborted from the untagged queue may
> +		 * not have been dispatched to the controller, so
> +		 * only check the SCB_ACTIVE flag for tagged transactions.
> +		 */
>  		printf("SCB %d done'd twice\n", scb->hscb->tag);
>  		ahc_dump_card_state(ahc);
>  		panic("Stopping for safety");
> 
Yes, this looks correct. The SCB_ACTIVE flag is reset when doing an ahc_scb_free()
at the very end of ahc_done(). And seeing that we are re-using scbs for error recovery
we might indeed end up with an scb with SCB_ACTIVE is set.

But I'll do some more investigation.
Do you have any setup to exercise this?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] aic7xxx - ahc_done check SCB_ACTIVE for tagged transactions

[David Milburn]

Hannes Reinecke wrote:
> David Milburn wrote:
> 
>>Hannes,
>>
>>Does ahc_done need to check the SCB_ACTIVE flag only if the SCB
>>is not in the untagged queue before panic?
>>
>>If the driver is in error recovery, you may end panic'ing
>>on a TUR that is in the untagged queue.
>>
>>Attempting to queue an ABORT message
>>CDB: 0x0 0x0 0x0 0x0 0x0 0x0
>>SCB 3 done'd twice
>>
>>This patch is included in Adaptec's 6.3.11 driver on their 
>>website.
>>
>>Thank you,
>>David
>>
>>--- scsi-misc-2.6.git/drivers/scsi/aic7xxx/aic7xxx_osm.c.abort
>>+++ scsi-misc-2.6.git/drivers/scsi/aic7xxx/aic7xxx_osm.c
>>@@ -1658,9 +1658,12 @@ ahc_done(struct ahc_softc *ahc, struct s
>> 		untagged_q = &(ahc->untagged_queues[target_offset]);
>> 		TAILQ_REMOVE(untagged_q, scb, links.tqe);
>> 		BUG_ON(!TAILQ_EMPTY(untagged_q));
>>-	}
>>-
>>-	if ((scb->flags & SCB_ACTIVE) == 0) {
>>+	} else if ((scb->flags & SCB_ACTIVE) == 0) {
>>+		/*
>>+		 * Transactions aborted from the untagged queue may
>>+		 * not have been dispatched to the controller, so
>>+		 * only check the SCB_ACTIVE flag for tagged transactions.
>>+		 */
>> 		printf("SCB %d done'd twice\n", scb->hscb->tag);
>> 		ahc_dump_card_state(ahc);
>> 		panic("Stopping for safety");
>>
> 
> Yes, this looks correct. The SCB_ACTIVE flag is reset when doing an ahc_scb_free()
> at the very end of ahc_done(). And seeing that we are re-using scbs for error recovery
> we might indeed end up with an scb with SCB_ACTIVE is set.
> 
> But I'll do some more investigation.
> Do you have any setup to exercise this?

Hannes,

I don't have a hands on setup, but, I verified the fix
by providing a rhel4 test kernel to prevent the panic
above.

James committed to scsi-misc-2.6.git

cddb3e38076639aacf2182e4b164e45ee8bbe6d8

Thanks,
David


> 
> Cheers,
> 
> Hannes