[PATCH] tmscsim: host_lock use in LLD

[PATCH] tmscsim: host_lock use in LLD

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1307 bytes --]

Hi

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.

BTW, I noticed, that other drivers have this bug (?) too, e.g.:

in *NCR5380.c::NCR5380_information_transfer() (called with lock held, irqs 
disabled)
 				while (NCR5380_read(STATUS_REG) & SR_REQ);

But just now, looking at dc395x.c I see at places, symmetric to those 
from tmscsim, that I am trying to fix with this patch, a comment from 
Kurt:
/* KG: This should not be needed any more! */
and those loops are commented out. Kurt (or anybody else), please, 
explain?

Thanks
Guennadi
---
Guennadi Liakhovetski

[-- Attachment #2: Type: TEXT/PLAIN, Size: 6611 bytes --]

diff -ur --exclude=CVS vanilla/linux-2.6.6/drivers/scsi/scsiiom.c linux-2.6.6-bk6-tmscsim/drivers/scsi/scsiiom.c
--- vanilla/linux-2.6.6/drivers/scsi/scsiiom.c	Thu Jun 17 23:47:03 2004
+++ linux-2.6.6-bk6-tmscsim/drivers/scsi/scsiiom.c	Fri Jun 18 23:03:38 2004
@@ -227,7 +227,6 @@
 #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 @@
     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_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 @@
 	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 @@
 
 	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 @@
 
 	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 @@
     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;
diff -ur --exclude=CVS vanilla/linux-2.6.6/drivers/scsi/tmscsim.c linux-2.6.6-bk6-tmscsim/drivers/scsi/tmscsim.c
--- vanilla/linux-2.6.6/drivers/scsi/tmscsim.c	Thu Jun 17 23:47:03 2004
+++ linux-2.6.6-bk6-tmscsim/drivers/scsi/tmscsim.c	Fri Jun 18 23:02:20 2004
@@ -283,12 +283,6 @@
 };
 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_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 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 @@
 
 	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);

Re: [PATCH] tmscsim: host_lock use in LLD

[James Bottomley]

On Fri, 2004-06-18 at 16:28, Guennadi Liakhovetski wrote:
> 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.

This patch is rejecting in this part:

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);
 

As best as I can tell, DC390_IFLAGS has never been part of the tmscsim.c
file ... I think this must be against some modification of the driver
you have in your tree.

James

Re: [PATCH] tmscsim: host_lock use in LLD

[Guennadi Liakhovetski]

On Sun, 20 Jun 2004, James Bottomley wrote:

> On Fri, 2004-06-18 at 16:28, Guennadi Liakhovetski wrote:
>> 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.
>
> This patch is rejecting in this part:
>
> 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);
>
>
> As best as I can tell, DC390_IFLAGS has never been part of the tmscsim.c
> file ... I think this must be against some modification of the driver
> you have in your tree.

Sorry, James. It's been a pretty long Sunday, and I do have a bit of a 
headache, so, what I see might quite well differ from what everybody else 
sees, but it does look like DC390_IFLAGS is still a part of tmscsim.c as 
of 2.6.7... And rejects are due to my another patch, sent to the list, 
cc'ed to you, Christoph and Kurt on the 31st May, and I did receive it 
back from the list. So, it could have happen that you didn't receive it 
personally, but it did come through to the list. Tell me if I should 
re-send it. Actually, according to my calculations, there are 3 more 
(apart from these 2) outstanding patches for tmscsim.

BTW, what is your preferred way of receiving patches, which depend on each 
other? Should I (in the future) wait until the previous patch appears 
(where?) in a global tree, or collect several patches and send them 
together, or is what I am doing ATM ok (just send patches in as they 
appear, "on the queue")?

Sorry for the inconvenience.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: host_lock use in LLD

[Guennadi Liakhovetski]

James

For your convenience, outstanding (submitted but not yet applied) patches 
are at http://home.arcor.de/g.liakhovetski/scsi/outstanding/

Thanks
Guennadi
---
Guennadi Liakhovetski