[PATCH] tmscsim: host_lock use in LLD

While reviewing tmscsim, I noticed something, I didn't quite like /
understand. The driver takes the host_lock (with irqsave) at the entry to
the ISR, and releases it at the exit. And inside the ISR there are
potentially long busy-waits... Like

 	int ctr = 6000000; /* only try for about a second */
 	while( --ctr && !((dstate = DC390_read8 (DMA_Status)) &
 				DMA_XFER_DONE) && pSRB->SGToBeXferLen );

The attached patch is attempting to fix those places. Not sure if this is
a proper fix though. In my understanding, after looking through the code,
the host_lock is used to protect host-specific data and host-registers.
The ->queuecommand is already called with it help, so, one just,
basically, have to protect other contexts - interrupt, timer,... So, looks
mostly right.
Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

drivers/scsi/scsiiom.c

@@ -227,7 +227,6 @@ DC390_Interrupt( int irq, void *dev_id, struct pt_regs *regs)
 #if DMA_INT
     UCHAR  dstatus;
 #endif
-    DC390_IFLAGS;
 
     pACB = (PACB)dev_id;
     for (pACB2 = dc390_pACB_start; (pACB2 && pACB2 != pACB); pACB2 = pACB2->pNextACB);
@@ -244,9 +243,9 @@ DC390_Interrupt( int irq, void *dev_id, struct pt_regs *regs)
     DEBUG1(printk (KERN_DEBUG "sstatus=%02x,", sstatus));
 
 #if DMA_INT
-    DC390_LOCK_IO(pACB->pScsiHost);
+    spin_lock_irq(pACB->pScsiHost->host_lock);
     dstatus = dc390_dma_intr (pACB);
-    DC390_UNLOCK_IO(pACB->pScsiHost);
+    spin_unlock_irq(pACB->pScsiHost->host_lock);
 
     DEBUG1(printk (KERN_DEBUG "dstatus=%02x,", dstatus));
     if (! (dstatus & SCSI_INTERRUPT))
@@ -260,7 +259,7 @@ DC390_Interrupt( int irq, void *dev_id, struct pt_regs *regs)
     //DC390_write32 (DMA_ScsiBusCtrl, EN_INT_ON_PCI_ABORT);
 #endif
 
-    DC390_LOCK_IO(pACB->pScsiHost);
+    spin_lock_irq(pACB->pScsiHost->host_lock);
 
     istate = DC390_read8 (Intern_State);
     istatus = DC390_read8 (INT_Status); /* This clears Scsi_Status, Intern_State and INT_Status ! */
@@ -328,11 +327,10 @@ DC390_Interrupt( int irq, void *dev_id, struct pt_regs *regs)
 	DEBUG1(printk (KERN_INFO "DC390: [%i]%s(1) (%02x)\n", phase, dc390_p1_str[phase], sstatus));
 	stateV = (void *) dc390_phase1[phase];
 	( *stateV )( pACB, pSRB, &sstatus );
-	goto unlock;
     }
 
  unlock:
-    DC390_UNLOCK_IO(pACB->pScsiHost);
+    spin_unlock_irq(pACB->pScsiHost->host_lock);
     return IRQ_HANDLED;
 }
 
@@ -363,10 +361,20 @@ dc390_DataOut_0( PACB pACB, PSRB pSRB, PUCHAR psstatus)
 
 	if( sstatus & COUNT_2_ZERO )
 	{
-	    int ctr = 6000000; /* only try for about a second */
-	    while( --ctr && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE) && pSRB->SGToBeXferLen );
-	    if (!ctr) printk (KERN_CRIT "DC390: Deadlock in DataOut_0: DMA aborted unfinished: %06x bytes remain!!\n", DC390_read32 (DMA_Wk_ByteCntr));
-	    dc390_laststatus &= ~0xff000000; dc390_laststatus |= dstate << 24;
+	    unsigned long timeout = jiffies + HZ;
+
+	    /* Function called from the ISR with the host_lock held and interrupts disabled */
+	    if (pSRB->SGToBeXferLen)
+		while (time_before(jiffies, timeout) && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE)) {
+		    spin_unlock_irq(pACB->pScsiHost->host_lock);
+		    udelay(50);
+		    spin_lock_irq(pACB->pScsiHost->host_lock);
+		}
+	    if (!time_before(jiffies, timeout))
+		printk (KERN_CRIT "DC390: Deadlock in DataOut_0: DMA aborted unfinished: %06x bytes remain!!\n",
+			DC390_read32 (DMA_Wk_ByteCntr));
+	    dc390_laststatus &= ~0xff000000;
+	    dc390_laststatus |= dstate << 24;
 	    pSRB->TotalXferredLen += pSRB->SGToBeXferLen;
 	    pSRB->SGIndex++;
 	    if( pSRB->SGIndex < pSRB->SGcount )
@@ -418,12 +426,23 @@ dc390_DataIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus)
 
 	if( sstatus & COUNT_2_ZERO )
 	{
-	    int ctr = 6000000; /* only try for about a second */
 	    int dstate = 0;
-	    while( --ctr && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE) && pSRB->SGToBeXferLen );
-	    if (!ctr) printk (KERN_CRIT "DC390: Deadlock in DataIn_0: DMA aborted unfinished: %06x bytes remain!!\n", DC390_read32 (DMA_Wk_ByteCntr));
-	    if (!ctr) printk (KERN_CRIT "DC390: DataIn_0: DMA State: %i\n", dstate);
-	    dc390_laststatus &= ~0xff000000; dc390_laststatus |= dstate << 24;
+	    unsigned long timeout = jiffies + HZ;
+
+	    /* Function called from the ISR with the host_lock held and interrupts disabled */
+	    if (pSRB->SGToBeXferLen)
+		while (time_before(jiffies, timeout) && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE)) {
+		    spin_unlock_irq(pACB->pScsiHost->host_lock);
+		    udelay(50);
+		    spin_lock_irq(pACB->pScsiHost->host_lock);
+		}
+	    if (!time_before(jiffies, timeout)) {
+		printk (KERN_CRIT "DC390: Deadlock in DataIn_0: DMA aborted unfinished: %06x bytes remain!!\n",
+			DC390_read32 (DMA_Wk_ByteCntr));
+		printk (KERN_CRIT "DC390: DataIn_0: DMA State: %i\n", dstate);
+	    }
+	    dc390_laststatus &= ~0xff000000;
+	    dc390_laststatus |= dstate << 24;
 	    DEBUG1(ResidCnt = ((ULONG) DC390_read8 (CtcReg_High) << 16)	\
 		+ ((ULONG) DC390_read8 (CtcReg_Mid) << 8)		\
 		+ ((ULONG) DC390_read8 (CtcReg_Low)));
@@ -1111,10 +1130,9 @@ dc390_Disconnect( PACB pACB )
     pDCB = pACB->pActiveDCB;
     if (!pDCB)
      {
-	int j = 400;
 	DEBUG0(printk(KERN_ERR "ACB:%p->ActiveDCB:%p IOPort:%04x IRQ:%02x !\n",\
 	       pACB, pDCB, pACB->IOPortBase, pACB->IRQLevel));
-	while (--j) udelay (1000);
+	mdelay(400);
 	DC390_read8 (INT_Status);	/* Reset Pending INT */
 	DC390_write8 (ScsiCmd, EN_SEL_RESEL);
 	return;

drivers/scsi/tmscsim.c

@@ -283,12 +283,6 @@ static struct pci_device_id tmscsim_pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, tmscsim_pci_tbl);
 
-#define USE_SPINLOCKS 1
-
-#define DC390_IFLAGS unsigned long iflags
-#define DC390_LOCK_IO(dev) spin_lock_irqsave (((struct Scsi_Host *)dev)->host_lock, iflags)
-#define DC390_UNLOCK_IO(dev) spin_unlock_irqrestore (((struct Scsi_Host *)dev)->host_lock, iflags)
-
 /* These macros are used for uniform access to 2.0.x and 2.1.x PCI config space*/
 
 #define PDEV pdev
@@ -863,11 +857,11 @@ static void dc390_Waiting_process ( PACB pACB )
 static void DC390_waiting_timed_out (unsigned long ptr)
 {
 	PACB pACB = (PACB)ptr;
-	DC390_IFLAGS;
+	unsigned long iflags;
 	DEBUG0(printk ("DC390: Debug: Waiting queue woken up by timer!\n"));
-	DC390_LOCK_IO(pACB->pScsiHost);
+	spin_lock_irqsave(pACB->pScsiHost->host_lock, iflags);
 	dc390_Waiting_process (pACB);
-	DC390_UNLOCK_IO(pACB->pScsiHost);
+	spin_unlock_irqrestore(pACB->pScsiHost->host_lock, iflags);
 }
 
 /***********************************************************************
@@ -2026,12 +2020,12 @@ static int __devexit dc390_shutdown (struct Scsi_Host *host)
 static void __devexit dc390_remove_one(struct pci_dev *dev)
 {
 	struct Scsi_Host *scsi_host = pci_get_drvdata(dev);
-	DC390_IFLAGS;
+	unsigned long iflags;
 	PACB pACB = (PACB) scsi_host->hostdata;
 
 	scsi_remove_host(scsi_host);
 
-	DC390_LOCK_IO(scsi_host);
+	spin_lock_irqsave(scsi_host->host_lock, iflags);
 
 	/* TO DO: We should check for outstanding commands first. */
 	dc390_shutdown(scsi_host);
@@ -2043,7 +2037,7 @@ static void __devexit dc390_remove_one(struct pci_dev *dev)
 
 	release_region(scsi_host->io_port, scsi_host->n_io_port);
 	dc390_freeDCBs(scsi_host);
-	DC390_UNLOCK_IO(scsi_host);
+	spin_unlock_irqrestore(scsi_host->host_lock, iflags);
 
 	pci_disable_device(dev);
 	scsi_host_put(scsi_host);