Re: [Bugme-new] [Bug 6092] New: drivers/scsi/pcmcia/aha152x_stub.c: aha152x_resume(): variable used before set

["Jürgen E. Fischer" <fischer@linux-buechse.de>]

Hi James,

On Sun, Feb 19, 2006 at 08:27:17 -0600, James Bottomley wrote:
> >         HOSTDATA(shpnt)->in_intr++;
> >  
> > +       if( HOSTDATA(shpnt)->service==0 )  {
> > +               DO_UNLOCK(flags);
> > +               return;
> > +       }
> 
> Looks a bit wrong.  If that ever triggered, you'd exit your task handler
> with in_intr raised, which would cause a panic the next time
> is_complete() was called.

ouch, but thanks for the understatement. ;)


> The driver still seems to have an awful lot of locking confusion between
> the host lock and its own internal lock (stored in the host structure).
> I think there are several races and other nasties that could be cleaned
> up simply by moving to using the host lock everywhere.

done.

patch:
- drop private lock and use host_lock instead.

Signed-off-by: Juergen E. Fischer <fischer@linux-buechse.de>

--- aha152x.c	2006-02-19 15:54:24.315291253 +0100
+++ linux-2.6/drivers/scsi/aha152x.c	2006-02-19 16:00:45.817015449 +0100
@@ -465,9 +465,6 @@
 	Scsi_Cmnd *done_SC;
 		/* command that was completed */
 
-	spinlock_t lock;
-		/* host lock */
-
 #if defined(AHA152X_DEBUG)
 	const char *locker;
 		/* which function has the lock */
@@ -563,7 +560,7 @@
 #define DONE_SC			(HOSTDATA(shpnt)->done_SC)
 #define ISSUE_SC		(HOSTDATA(shpnt)->issue_SC)
 #define DISCONNECTED_SC		(HOSTDATA(shpnt)->disconnected_SC)
-#define QLOCK			(HOSTDATA(shpnt)->lock)
+#define QLOCK			((shpnt)->host_lock)
 #define QLOCKER			(HOSTDATA(shpnt)->locker)
 #define QLOCKERL		(HOSTDATA(shpnt)->lockerl)
 
@@ -802,7 +799,6 @@
 		HOSTIOPORT1 = setup->io_port-0x10;
 	}
 
-	spin_lock_init(&QLOCK);
 	RECONNECT   = setup->reconnect;
 	SYNCHRONOUS = setup->synchronous;
 	PARITY      = setup->parity;
@@ -2539,6 +2535,13 @@
 
 	DO_LOCK(flags);
 
+	if( HOSTDATA(shpnt)->service==0 )  {
+		DO_UNLOCK(flags);
+		return;
+	}
+
+	HOSTDATA(shpnt)->service = 0;
+
 	if(HOSTDATA(shpnt)->in_intr) {
 		DO_UNLOCK(flags);
 		/* aha152x_error never returns.. */
@@ -2546,13 +2549,6 @@
 	}
 	HOSTDATA(shpnt)->in_intr++;
 
-	if (HOSTDATA(shpnt)->service == 0) {
-		DO_UNLOCK(flags);
-		return;
-	}
-
-	HOSTDATA(shpnt)->service = 0;
-
 
 	/*
 	 * loop while there are interrupt conditions pending