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

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

[Andrew Morton]

bugme-daemon@bugzilla.kernel.org wrote:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=6092
> 
>            Summary: drivers/scsi/pcmcia/aha152x_stub.c: aha152x_resume():
>                     variable used before set
>     Kernel Version: 2.6.16-rc4 and earlier
>             Status: NEW
>           Severity: high
>              Owner: scsi_drivers-aha152x@kernel-bugs.osdl.org
>          Submitter: bunk@stusta.de
> 
> 
> David Binderman <dcb314@hotmail.com> reported the following bug:
> 
> I just tried to compile the Linux Kernel version 2.6.11.12
> with the most excellent Intel C compiler. It said
> 
> drivers/scsi/pcmcia/aha152x_stub.c(313): remark #592: variable "tmp" is used
> before its value is set
>        tmp.device->host = info->host;
>        ^
> 
> This is clearly broken code, since the field tmp.device has not been
> initialised, and so isn't pointing to anything.
> 

(good luck - I've brought this to the scsi list a few times.  Unmaintained)

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

[Jürgen E. Fischer]

Hi there,

On Fri, Feb 17, 2006 at 21:10:49 -0800, Andrew Morton wrote:
> > I just tried to compile the Linux Kernel version 2.6.11.12
> > with the most excellent Intel C compiler. It said
> > 
> > drivers/scsi/pcmcia/aha152x_stub.c(313): remark #592: variable "tmp" is used
> > before its value is set
> >        tmp.device->host = info->host;
> >        ^
> > 
> > This is clearly broken code, since the field tmp.device has not been
> > initialised, and so isn't pointing to anything.

Fix below.

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


diff -ur orig/linux-2.6/drivers/scsi/aha152x.c linux-2.6/drivers/scsi/aha152x.c
--- orig/linux-2.6/drivers/scsi/aha152x.c	2006-02-18 13:04:12.132570132 +0100
+++ linux-2.6/drivers/scsi/aha152x.c	2006-02-18 22:49:58.121862877 +0100
@@ -1259,16 +1259,15 @@
  * Reset the bus
  *
  */
-static int aha152x_bus_reset(Scsi_Cmnd *SCpnt)
+static int aha152x_bus_reset_host(struct Scsi_Host *shpnt)
 {
-	struct Scsi_Host *shpnt = SCpnt->device->host;
 	unsigned long flags;
 
 	DO_LOCK(flags);
 
 #if defined(AHA152X_DEBUG)
 	if(HOSTDATA(shpnt)->debug & debug_eh) {
-		printk(DEBUG_LEAD "aha152x_bus_reset(%p)", CMDINFO(SCpnt), SCpnt);
+		printk(KERN_DEBUG "scsi%d: bus reset", shpnt->host_no);
 		show_queues(shpnt);
 	}
 #endif
@@ -1276,14 +1275,14 @@
 	free_hard_reset_SCs(shpnt, &ISSUE_SC);
 	free_hard_reset_SCs(shpnt, &DISCONNECTED_SC);
 
-	DPRINTK(debug_eh, DEBUG_LEAD "resetting bus\n", CMDINFO(SCpnt));
+	DPRINTK(debug_eh, KERN_DEBUG "scsi%d: resetting bus\n", shpnt->host_no);
 
 	SETPORT(SCSISEQ, SCSIRSTO);
 	mdelay(256);
 	SETPORT(SCSISEQ, 0);
 	mdelay(DELAY);
 
-	DPRINTK(debug_eh, DEBUG_LEAD "bus resetted\n", CMDINFO(SCpnt));
+	DPRINTK(debug_eh, KERN_DEBUG "scsi%d: bus resetted\n", shpnt->host_no);
 
 	setup_expected_interrupts(shpnt);
 	if(HOSTDATA(shpnt)->commands==0)
@@ -1294,6 +1293,14 @@
 	return SUCCESS;
 }
 
+/*
+ * Reset the bus
+ *
+ */
+static int aha152x_bus_reset(Scsi_Cmnd *SCpnt)
+{
+	return aha152x_bus_reset_host(SCpnt->device->host);
+}
 
 /*
  *  Restore default values to the AIC-6260 registers and reset the fifos
@@ -1336,23 +1343,28 @@
  * Reset the host (bus and controller)
  *
  */
-int aha152x_host_reset(Scsi_Cmnd * SCpnt)
+int aha152x_host_reset_host(struct Scsi_Host *shpnt)
 {
-#if defined(AHA152X_DEBUG)
-	struct Scsi_Host *shpnt = SCpnt->device->host;
-#endif
+	DPRINTK(debug_eh, KERN_DEBUG "scsi%d: host reset\n", shpnt->host_no);
 
-	DPRINTK(debug_eh, DEBUG_LEAD "aha152x_host_reset(%p)\n", CMDINFO(SCpnt), SCpnt);
+	aha152x_bus_reset_host(shpnt);
 
-	aha152x_bus_reset(SCpnt);
-
-	DPRINTK(debug_eh, DEBUG_LEAD "resetting ports\n", CMDINFO(SCpnt));
-	reset_ports(SCpnt->device->host);
+	DPRINTK(debug_eh, KERN_DEBUG "scsi%d: resetting ports\n", shpnt->host_no);
+	reset_ports(shpnt);
 
 	return SUCCESS;
 }
 
 /*
+ * Reset the host (bus and controller)
+ * 
+ */
+static int aha152x_host_reset(Scsi_Cmnd *SCpnt)
+{
+	return aha152x_host_reset_host(SCpnt->device->host);
+}
+
+/*
  * Return the "logical geometry"
  *
  */
@@ -1430,22 +1442,18 @@
 {
 	int i;
 	for (i = 0; i<ARRAY_SIZE(aha152x_host); i++) {
-		struct Scsi_Host *shpnt = aha152x_host[i];
-		if (shpnt && HOSTDATA(shpnt)->service) {
-			HOSTDATA(shpnt)->service=0;
-			is_complete(shpnt);
-		}
+		is_complete(aha152x_host[i]);
 	}
 }
 
 /*
- *    Interrupts handler
+ * Interrupt handler
  *
  */
-
 static irqreturn_t intr(int irqno, void *dev_id, struct pt_regs *regs)
 {
 	struct Scsi_Host *shpnt = lookup_irq(irqno);
+	unsigned long flags;
 	unsigned char rev, dmacntrl0;
 
 	if (!shpnt) {
@@ -1471,23 +1479,23 @@
 	if ((rev == 0xFF) && (dmacntrl0 == 0xFF))
 		return IRQ_NONE;
 
+	if( TESTLO(DMASTAT, INTSTAT) )
+		return IRQ_NONE;	
+
 	/* no more interrupts from the controller, while we're busy.
 	   INTEN is restored by the BH handler */
 	CLRBITS(DMACNTRL0, INTEN);
 
-#if 0
-	/* check if there is already something to be
-           serviced; should not happen */
-	if(HOSTDATA(shpnt)->service) {
-		printk(KERN_ERR "aha152x%d: lost interrupt (%d)\n", HOSTNO, HOSTDATA(shpnt)->service);
-	        show_queues(shpnt);
+	DO_LOCK(flags);
+	if( HOSTDATA(shpnt)->service==0 ) {
+		HOSTDATA(shpnt)->service=1;
+
+		/* Poke the BH handler */
+		INIT_WORK(&aha152x_tq, (void *) run, NULL);
+		schedule_work(&aha152x_tq);
 	}
-#endif
-	
-	/* Poke the BH handler */
-	HOSTDATA(shpnt)->service++;
-	INIT_WORK(&aha152x_tq, (void *) run, NULL);
-	schedule_work(&aha152x_tq);
+	DO_UNLOCK(flags);
+
 	return IRQ_HANDLED;
 }
 
@@ -2526,7 +2534,11 @@
 	unsigned long flags;
 	int pending;
 
+	if(!shpnt)
+		return;
+
 	DO_LOCK(flags);
+
 	if(HOSTDATA(shpnt)->in_intr) {
 		DO_UNLOCK(flags);
 		/* aha152x_error never returns.. */
@@ -2534,6 +2546,14 @@
 	}
 	HOSTDATA(shpnt)->in_intr++;
 
+	if( HOSTDATA(shpnt)->service==0 )  {
+		DO_UNLOCK(flags);
+		return;
+	}
+
+	HOSTDATA(shpnt)->service = 0;
+
+
 	/*
 	 * loop while there are interrupt conditions pending
 	 *
diff -ur orig/linux-2.6/drivers/scsi/aha152x.h linux-2.6/drivers/scsi/aha152x.h
--- orig/linux-2.6/drivers/scsi/aha152x.h	2005-06-17 21:48:29.000000000 +0200
+++ linux-2.6/drivers/scsi/aha152x.h	2006-02-18 16:40:48.383590058 +0100
@@ -332,6 +332,6 @@
 
 struct Scsi_Host *aha152x_probe_one(struct aha152x_setup *);
 void aha152x_release(struct Scsi_Host *);
-int aha152x_host_reset(Scsi_Cmnd *);
+int aha152x_host_reset_host(struct Scsi_Host *);
 
 #endif /* _AHA152X_H */
diff -ur orig/linux-2.6/drivers/scsi/pcmcia/aha152x_stub.c linux-2.6/drivers/scsi/pcmcia/aha152x_stub.c
--- orig/linux-2.6/drivers/scsi/pcmcia/aha152x_stub.c	2006-02-18 12:52:23.902417669 +0100
+++ linux-2.6/drivers/scsi/pcmcia/aha152x_stub.c	2006-02-18 20:52:18.154711475 +0100
@@ -302,10 +302,8 @@
 	/* Fall through... */
     case CS_EVENT_CARD_RESET:
 	if (link->state & DEV_CONFIG) {
-	    Scsi_Cmnd tmp;
 	    pcmcia_request_configuration(link->handle, &link->conf);
-	    tmp.device->host = info->host;
-	    aha152x_host_reset(&tmp);
+	    aha152x_host_reset_host(info->host);
 	}
 	break;
     }

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

[Andrew Morton]

Jürgen E. Fischer <fischer@linux-buechse.de> wrote:
>
> On Fri, Feb 17, 2006 at 21:10:49 -0800, Andrew Morton wrote:
>  > > I just tried to compile the Linux Kernel version 2.6.11.12
>  > > with the most excellent Intel C compiler. It said
>  > > 
>  > > drivers/scsi/pcmcia/aha152x_stub.c(313): remark #592: variable "tmp" is used
>  > > before its value is set
>  > >        tmp.device->host = info->host;
>  > >        ^
>  > > 
>  > > This is clearly broken code, since the field tmp.device has not been
>  > > initialised, and so isn't pointing to anything.
> 
>  Fix below.

Thanks.

Could we please have a more complete description of this change?  It's
obviosly doing more than fixing a used-uninitialised bug..
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jürgen E. Fischer]

Hi Andrew,

On Sat, Feb 18, 2006 at 14:14:23 -0800, Andrew Morton wrote:
> >  Fix below.
 
> Thanks.
 
> Could we please have a more complete description of this change?  It's
> obviosly doing more than fixing a used-uninitialised bug..

After fixing the problem in question, I discoved that the driver still
didn't work with the pcmcia card I picked up.  run/is_complete was
sometimes not called at all or called multiple times on one interrupt.

I'm not really sure why (or why that worked before).  It's now ensured
that run() is only added to the task queue once per interrupt (by
protecting 'service' with spinlocks).

Additionally interrupt sharing didn't seem to work as the interrupt
handler didn't check if a interrupt condition was at all pending.


In short:
- change interface of the reset functions from Scsi_Cmnd to Scsi_Host.  
- add functions with the original interface and rename the new
  functions to reflect the new interface.
- call these from the pcmcia driver, thereby avoiding the need to
  construct a (broken) Scsi_Cmnd from a Scsi_Host.
- just run the bh if the interrupt is from the controller and if so
  ensure that it's only called once per interrupt.


Jürgen
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andrew Morton]

Jürgen E. Fischer <fischer@linux-buechse.de> wrote:
>
> Hi Andrew,
> 
> On Sat, Feb 18, 2006 at 14:14:23 -0800, Andrew Morton wrote:
> > >  Fix below.
>  
> > Thanks.
>  
> > Could we please have a more complete description of this change?  It's
> > obviosly doing more than fixing a used-uninitialised bug..
> 
> After fixing the problem in question, I discoved that the driver still
> didn't work with the pcmcia card I picked up.  run/is_complete was
> sometimes not called at all or called multiple times on one interrupt.
> 
> I'm not really sure why (or why that worked before).  It's now ensured
> that run() is only added to the task queue once per interrupt (by
> protecting 'service' with spinlocks).
> 
> Additionally interrupt sharing didn't seem to work as the interrupt
> handler didn't check if a interrupt condition was at all pending.
> 

So are you saying that with this patch, the driver works OK?

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jürgen E. Fischer]

Hi Andrew,

On Sat, Feb 18, 2006 at 15:46:14 -0800, Andrew Morton wrote:
> So are you saying that with this patch, the driver works OK?

Exactly.  I tested using a scanner and a tape drive.


Jürgen
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Bottomley]

On Sun, 2006-02-19 at 00:31 +0100, Jürgen E. Fischer wrote:
> - just run the bh if the interrupt is from the controller and if so
>   ensure that it's only called once per interrupt.

This is the bit that looks like the actual fix.  It basically uses
service as a flag to debounce the interrupt.

This:

>         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.

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.

James


-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jürgen E. Fischer]

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

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

[James Bottomley]

On Sun, 2006-02-19 at 16:08 +0100, Jürgen E. Fischer wrote:
> > 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 */
> -

Actually, unfortunately, it doesn't look to be quite as simple as
that ... there are places (in ahd542x_queue_internal) where both locks
are currently held.  Perhaps I should just put the complete fix of the
previous patch in for 2.6.16 and then the locking changes can be worked
on post 2.6.16 (as in two separate patches)

James


-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html