Re: ATA driver races with interrupts

Ville-Pertti Keinonen wrote:
> Søren Schmidt wrote:
> 
>> If the controller doesn't have a bit saying if the interrupt is for 
>> us, then its impossible to close the race completely (without the 
>> above measures in place). However some devices use the DMA interrupt 
>> bits even in PIO mode (ie HPT does this) but I have no docs on the 
>> VIA's on that, but its worth a try at least...
> 
> The current situation seems to me like ata_generic_intr can't work for 
> *any* controller sharing interrupts, even with DMA on a controller that 
> sets ATA_BMSTAT_INTERRUPT correctly, since if an interrupt occurs before 
> ATA_DMA_ACTIVE is set, ATA_BMSTAT_PORT is never even checked.

Oh, I dont mean the ATA_BMSTAT* bits, I mean real interrupt status bits 
in the controller that you can poll on interrupt to see what exactly 
caused the event (check the Promise support fx).

> IMHO ch->running != NULL is an insufficient condition based on which to 
> assume that an interrupt is intended for a specific channel...

It was newer meant for that purpose really, but checking it was a nice 
way of sorting of some trouble.

> Something similar to my patch + disabling interrupts to avoid a race 
> between the interrupt and ATA_EXPECT_INTR being set should at least 
> improve the situation.  What races would that leave open?

We dont want to disable interrupts, ever.

I have one change in a tree here that you could try, but as long as the 
hardware doesn't support proper interrupt status its impossible to close 
the race window completely. Please remember that this is part of a 
bigger patchset, so I might have edited it too much, YMMV,,


-- 
-Søren


Index: ata-lowlevel.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/ata/ata-lowlevel.c,v
retrieving revision 1.40
diff -u -r1.40 ata-lowlevel.c
--- ata-lowlevel.c	24 Jul 2004 19:03:28 -0000	1.40
+++ ata-lowlevel.c	3 Aug 2004 10:25:19 -0000
_at__at_ -80,9 +80,6 _at__at_
 	return ATA_OP_FINISHED;
     }
 
-    /* record the request as running */
-    ch->running = request;
-
     ATA_DEBUG_RQ(request, "transaction");
 
     /* disable ATAPI DMA writes if HW doesn't support it */
_at__at_ -139,7 +136,8 _at__at_
 	    }
 	}
 	
-	/* return and wait for interrupt */
+	/* record the request as running and return for interrupt */
+	ch->running = request;
 	return ATA_OP_CONTINUES;
 
     /* ATA DMA data transfer commands */
_at__at_ -168,7 +166,8 _at__at_
 	    break;
 	}
 
-	/* return and wait for interrupt */
+	/* record the request as running and return for interrupt */
+	ch->running = request;
 	return ATA_OP_CONTINUES;
 
     /* ATAPI PIO commands */
_at__at_ -192,8 +191,10 _at__at_
 	}
 
 	/* command interrupt device ? just return and wait for interrupt */
-	if ((request->device->param->config & ATA_DRQ_MASK) == ATA_DRQ_INTR)
+	if ((request->device->param->config & ATA_DRQ_MASK) == ATA_DRQ_INTR) {
+	    ch->running = request;
 	    return ATA_OP_CONTINUES;
+	}
 
 	/* wait for ready to write ATAPI command block */
 	{
_at__at_ -224,7 +225,8 _at__at_
 			   (request->device->param->config & ATA_PROTO_MASK) ==
 			   ATA_PROTO_ATAPI_12 ? 6 : 8);
 
-	/* return and wait for interrupt */
+	/* record the request as running and return for interrupt */
+	ch->running = request;
 	return ATA_OP_CONTINUES;
 
     case ATA_R_ATAPI|ATA_R_DMA:
_at__at_ -289,14 +291,14 _at__at_
 	    break;
 	}
 
-	/* return and wait for interrupt */
+	/* record the request as running and return for interrupt */
+	ch->running = request;
 	return ATA_OP_CONTINUES;
     }
 
     /* request finish here */
     if (ch->dma->flags & ATA_DMA_ACTIVE)
 	ch->dma->unload(ch);
-    ch->running = NULL;
     return ATA_OP_FINISHED;
 }