Subject: Re: lock/unlock asymmetry
To: None <tech-kern@NetBSD.org>
From: David Young <dyoung@pobox.com>
List: tech-kern
Date: 07/09/2007 13:03:21
--NtwzykIc2mflq5ck
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jul 09, 2007 at 10:32:56AM -0700, Bill Stouder-Studenmund wrote:
> On Sun, Jul 08, 2007 at 02:24:00AM +0200, Hauke Fath wrote:
> > At 2:14 Uhr +0200 8.7.2007, Adam Hamsik wrote:
> > >     324 	/* Now look at channel B. */
> > >     325 	cs = zsc->zsc_cs[1];
> > >		!!!Here cs become Channel B not A  so when we unlock it
> > >		B is unlocked
> > 
> > Argh!! Time for bed.
> > 
> > Thanks, and sorry for the noise...
> 
> We should add a comment noting this.

Another way is to introduce variables csa and csb for channels A and
B, and assign to each just once.  See the attached (untested) patch.
I see an opportunity in zsc_intr_hard() to extract some duplicate code
into a subroutine. :-)

Dave

-- 
David Young             OJC Technologies
dyoung@ojctech.com      Urbana, IL * (217) 278-3933 ext 24

--NtwzykIc2mflq5ck
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=csa-csb

Index: z8530sc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/z8530sc.c,v
retrieving revision 1.23
diff -p -u -u -p -r1.23 z8530sc.c
--- z8530sc.c	4 Mar 2007 06:02:04 -0000	1.23
+++ z8530sc.c	9 Jul 2007 17:58:49 -0000
@@ -288,17 +288,18 @@ zsc_intr_hard(arg)
 	void *arg;
 {
 	struct zsc_softc *zsc = arg;
-	struct zs_chanstate *cs;
+	struct zs_chanstate *csa, *csb;
 	u_char rr3;
 
 	/* First look at channel A. */
-	cs = zsc->zsc_cs[0];
+	csa = zsc->zsc_cs[0];
+	csb = zsc->zsc_cs[1];
 
 	/* Lock both channels */
-	simple_lock(&cs->cs_lock);
-	simple_lock(&zsc->zsc_cs[1]->cs_lock);
+	simple_lock(&csa->cs_lock);
+	simple_lock(&csb->cs_lock);
 	/* Note: only channel A has an RR3 */
-	rr3 = zs_read_reg(cs, 3);
+	rr3 = zs_read_reg(csa, 3);
 
 	/*
 	 * Clear interrupt first to avoid a race condition.
@@ -309,31 +310,30 @@ zsc_intr_hard(arg)
 	 * on this interrupt level (i.e. sun3x floppy disk).
 	 */
 	if (rr3 & (ZSRR3_IP_A_RX | ZSRR3_IP_A_TX | ZSRR3_IP_A_STAT)) {
-		zs_write_csr(cs, ZSWR0_CLR_INTR);
+		zs_write_csr(csa, ZSWR0_CLR_INTR);
 		if (rr3 & ZSRR3_IP_A_RX)
-			(*cs->cs_ops->zsop_rxint)(cs);
+			(*csa->cs_ops->zsop_rxint)(csa);
 		if (rr3 & ZSRR3_IP_A_STAT)
-			(*cs->cs_ops->zsop_stint)(cs, 0);
+			(*csa->cs_ops->zsop_stint)(csa, 0);
 		if (rr3 & ZSRR3_IP_A_TX)
-			(*cs->cs_ops->zsop_txint)(cs);
+			(*csa->cs_ops->zsop_txint)(csa);
 	}
 
 	/* Done with channel A */
-	simple_unlock(&cs->cs_lock);
+	simple_unlock(&csa->cs_lock);
 
 	/* Now look at channel B. */
-	cs = zsc->zsc_cs[1];
 	if (rr3 & (ZSRR3_IP_B_RX | ZSRR3_IP_B_TX | ZSRR3_IP_B_STAT)) {
-		zs_write_csr(cs, ZSWR0_CLR_INTR);
+		zs_write_csr(csb, ZSWR0_CLR_INTR);
 		if (rr3 & ZSRR3_IP_B_RX)
-			(*cs->cs_ops->zsop_rxint)(cs);
+			(*csb->cs_ops->zsop_rxint)(csb);
 		if (rr3 & ZSRR3_IP_B_STAT)
-			(*cs->cs_ops->zsop_stint)(cs, 0);
+			(*csb->cs_ops->zsop_stint)(csb, 0);
 		if (rr3 & ZSRR3_IP_B_TX)
-			(*cs->cs_ops->zsop_txint)(cs);
+			(*csb->cs_ops->zsop_txint)(csb);
 	}
 
-	simple_unlock(&cs->cs_lock);
+	simple_unlock(&csb->cs_lock);
 
 	/* Note: caller will check cs_x->cs_softreq and DTRT. */
 	return (rr3);

--NtwzykIc2mflq5ck--