Re: ATA driver races with interrupts

My previous patch makes things more reliable, but it's not good enough...

Attached is an updated version of the patch that clears ATA_EXPECT_INTR 
earlier (in the interrupt handler), which seems to stabilize things better.

I'm definitely not convinced of its correctness, since I'm not sure 
which part of the code is racing against interrupts despite the previous 
version of the patch, but I can no longer cause any failures.

Repeating the problem should be trivial with an unpatched -current with 
PREEMPTION enabled and hardware similar to mine - ASUS K8V Deluxe, two 
SATA disks on the VIA 6420:

...
atapci1: <VIA 6420 SATA150 controller> port 
0xd000-0xd0ff,0xd400-0xd40f,0xd800-0xd803,0xe000-0xe007,0xe400-0xe403,0xe800-0xe807 
irq 20 at device 15.0 on pci0
ata5: at 0xe800 on atapci1
ata6: at 0xe000 on atapci1
...
ad10: 152627MB <SAMSUNG SP1614C> [310101/16/63] at ata5-master SATA150
ad12: 152627MB <SAMSUNG SP1614C> [310101/16/63] at ata6-master SATA150

Partitions from ad10 and ad12 were mounted on /sata1 and /sata2, 
respectively, both containing tens of gigabytes of files.

By issuing

$ find /sata1 -type f -exec md5 '{}' \; & find /sata2 -type f -exec md5 
'{}' \; &

the system would lock up or detach one of the disks in less than 5 seconds.

With the attached version of my patch, it completed successfully.


Index: ata-all.h
===================================================================
RCS file: /data/freebsd/src/sys/dev/ata/ata-all.h,v
retrieving revision 1.79
diff -u -r1.79 ata-all.h
--- ata-all.h	30 Apr 2004 16:21:34 -0000	1.79
+++ ata-all.h	2 Aug 2004 15:54:50 -0000
_at__at_ -339,6 +339,7 _at__at_
 #define		ATA_48BIT_ACTIVE	0x10
 #define		ATA_IMMEDIATE_MODE	0x20
 #define		ATA_HWGONE		0x40
+#define		ATA_EXPECT_INTR		0x80
 
     struct ata_device		device[2];	/* devices on this channel */
 #define		MASTER			0x00
Index: ata-lowlevel.c
===================================================================
RCS file: /data/freebsd/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	2 Aug 2004 16:34:00 -0000
_at__at_ -81,6 +81,7 _at__at_
     }
 
     /* record the request as running */
+    ch->flags &= ~ATA_EXPECT_INTR;
     ch->running = request;
 
     ATA_DEBUG_RQ(request, "transaction");
_at__at_ -140,6 +141,7 _at__at_
 	}
 	
 	/* return and wait for interrupt */
+	ch->flags |= ATA_EXPECT_INTR;
 	return ATA_OP_CONTINUES;
 
     /* ATA DMA data transfer commands */
_at__at_ -169,6 +171,7 _at__at_
 	}
 
 	/* return and wait for interrupt */
+	ch->flags |= ATA_EXPECT_INTR;
 	return ATA_OP_CONTINUES;
 
     /* ATAPI PIO commands */
_at__at_ -225,6 +228,7 _at__at_
 			   ATA_PROTO_ATAPI_12 ? 6 : 8);
 
 	/* return and wait for interrupt */
+	ch->flags |= ATA_EXPECT_INTR;
 	return ATA_OP_CONTINUES;
 
     case ATA_R_ATAPI|ATA_R_DMA:
_at__at_ -290,6 +294,7 _at__at_
 	}
 
 	/* return and wait for interrupt */
+	ch->flags |= ATA_EXPECT_INTR;
 	return ATA_OP_CONTINUES;
     }
 
_at__at_ -308,7 +313,7 _at__at_
     int length;
 
     /* ignore this interrupt if there is no running request */
-    if (!request) 
+    if (!request || !(ch->flags & ATA_EXPECT_INTR))
 	return;
 
     ATA_DEBUG_RQ(request, "interrupt");
_at__at_ -524,6 +529,8 _at__at_
 	break;
     }
 
+    ch->flags &= ~ATA_EXPECT_INTR;
+
     /* if we timed out the unlocking of the ATA channel is done later */
     if (!(request->flags & ATA_R_TIMEOUT)) {
 	ch->running = NULL;