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