From owner-freebsd-current@FreeBSD.ORG Tue Aug 3 08:40:19 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5F72816A4CF for ; Tue, 3 Aug 2004 08:40:19 +0000 (GMT) Received: from spider.deepcore.dk (cpe.atm2-0-53484.0x50a6c9a6.abnxx9.customer.tele.dk [80.166.201.166]) by mx1.FreeBSD.org (Postfix) with ESMTP id E2F3E43D68 for ; Tue, 3 Aug 2004 08:40:16 +0000 (GMT) (envelope-from sos@DeepCore.dk) Received: from DeepCore.dk ([192.168.0.130]) by spider.deepcore.dk (8.12.11/8.12.10) with ESMTP id i738e6vF060814; Tue, 3 Aug 2004 10:40:11 +0200 (CEST) (envelope-from sos@DeepCore.dk) Message-ID: <410F4F66.8040201@DeepCore.dk> Date: Tue, 03 Aug 2004 10:40:06 +0200 From: =?ISO-8859-1?Q?S=F8ren_Schmidt?= User-Agent: Mozilla Thunderbird 0.5 (X11/20040329) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Ville-Pertti Keinonen References: <410E688D.7020709@will.iki.fi> <410E74F7.1070000@will.iki.fi> <20040802132802.3d7kgoow0c80ss0s@www.sweetdreamsracing.biz> <410E7B8B.3080407@will.iki.fi> <410E81B8.1000206@DeepCore.dk> <410E8594.7070600@will.iki.fi> <410EA92C.6090506@DeepCore.dk> <410F3DD0.5030104@will.iki.fi> In-Reply-To: <410F3DD0.5030104@will.iki.fi> Content-Type: multipart/mixed; boundary="------------070309020503090709080401" X-mail-scanned: by DeepCore Virus & Spam killer v1.4 cc: freebsd-current@freebsd.org Subject: Re: ATA driver races with interrupts X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Aug 2004 08:40:19 -0000 This is a multi-part message in MIME format. --------------070309020503090709080401 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit 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 --------------070309020503090709080401 Content-Type: text/plain; name="running-patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="running-patch" 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 @@ -80,9 +80,6 @@ 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 */ @@ -139,7 +136,8 @@ } } - /* 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 */ @@ -168,7 +166,8 @@ 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 */ @@ -192,8 +191,10 @@ } /* 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 */ { @@ -224,7 +225,8 @@ (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: @@ -289,14 +291,14 @@ 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; } --------------070309020503090709080401--