>Number:         4566
>Category:       kern
>Synopsis:       pcmcia uses splraise(), and it uses it incorrectly.
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people (Kernel Bug People)
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Nov 23 10:20:03 1997
>Last-Modified:
>Originator:     Christos Zoulas
>Organization:
Astron
>Release:        NetBSD-1.3A
>Environment:


>Description:
	pcmcia.c uses splraise(IPL_XXX) instead of splraise(imask[IPL_XXX])
	which is the right usage for the i386. In any case, it
	should not be using splraise() in the first place since it
	is not an MI interface. It should not compare directly the
	IPL_XXX numbers to determine priority because the numbering vs.
	priority ordering of IPL numbers is not consistant across ports.

>How-To-Repeat:
	Look at the code.

>Fix:
	The following ugly fix, treats IPL_{SERIAL,NET,BIO} specially
	since these are the only IPL's used by the device drivers.
	Please note that IPL_SERIAL is not present in all the ports,
	so the pcmcia code will not compile on ports that do not define it.

Index: pcmcia.c
===================================================================
RCS file: /a/cvsroot/src/sys/dev/pcmcia/pcmcia.c,v
retrieving revision 1.3
diff -u -r1.3 pcmcia.c
--- pcmcia.c	1997/10/19 14:04:29	1.3
+++ pcmcia.c	1997/11/23 18:04:48
@@ -72,6 +72,10 @@
 
 int	pcmcia_card_intr __P((void *));
 
+static void pcmcia_badspl __P((int)) __attribute__((__noreturn__));
+static int pcmcia_splcmp __P((int, int));
+static int pcmcia_splset __P((int));
+
 struct cfdriver pcmcia_cd = {
 	NULL, "pcmcia", DV_DULL
 };
@@ -80,6 +84,89 @@
 	sizeof(struct pcmcia_softc), pcmcia_match, pcmcia_attach
 };
 
+/*
+ * XXX:	Centralize error checking for bad spl's
+ */
+static void
+pcmcia_badspl(s)
+	int s;
+{
+	panic("Unsupported spl value %d", s);
+}
+
+/*
+ * XXX: Unfortunately IPL_XXX constants are not numbered consistantly
+ * 	across ports, so we need to do the comparison manually.
+ *	We assume IPL_SERIAL is highest priority followed by IPL_NET
+ *	and followed by IPL_BIO.
+ *	return -1 if s1 has lower priority than s2
+ *		0 if s1 has equal priority to s2
+ *		1 if s1 has higher priority than s2
+ */
+static int
+pcmcia_splcmp(s1, s2)
+	int s1, s2;
+{
+	switch (s1) {
+	case IPL_BIO:
+		switch (s2) {
+		case IPL_BIO:
+			return 0;
+		case IPL_NET:
+		case IPL_SERIAL:
+			return -1;
+		default:
+			pcmcia_badspl(s2);
+		}
+
+	case IPL_NET:
+		switch (s2) {
+		case IPL_BIO:
+			return 1;
+		case IPL_NET:
+			return 0;
+		case IPL_SERIAL:
+			return -1;
+		default:
+			pcmcia_badspl(s2);
+		}
+
+	case IPL_SERIAL:
+		switch (s2) {
+		case IPL_BIO:
+		case IPL_NET:
+			return 1;
+		case IPL_SERIAL:
+			return 0;
+		default:
+			pcmcia_badspl(s2);
+		}
+	default:
+		pcmcia_badspl(s1);
+	}
+}
+
+/*
+ * XXX: We don't have access to a function that takes an IPL_XXX number
+ *	and sets the interrupt level, so we bake our own.
+ */
+static int
+pcmcia_splset(s)
+	int s;
+{
+	switch (s) {
+	case IPL_BIO:
+		return splbio();
+	case IPL_NET:
+		return splnet();
+	case IPL_SERIAL:
+		return splserial();
+	default:
+		pcmcia_badspl(s);
+	}
+}
+	
+
 int
 pcmcia_ccr_read(pf, ccr)
 	struct pcmcia_function *pf;
@@ -519,31 +606,28 @@
 		int s, ihcnt, hiipl, reg;
 		struct pcmcia_function *pf2;
 
-		/* XXX splraise() use needs to go away! */
-
 		/*
 		 * mask all the ipl's which are already used by this card,
 		 * and find the highest ipl number (lowest priority)
 		 */
 
 		ihcnt = 0;
-		s = 0;		/* this is only here to keep the compipler
-				   happy */
-		hiipl = 0;	/* this is only here to keep the compipler
-				   happy */
+		s = 0;		/* Appease gcc */
+		hiipl = 0;	/* Appease gcc */
 
 		for (pf2 = pf->sc->card.pf_head.sqh_first; pf2 != NULL;
 		     pf2 = pf2->pf_list.sqe_next) {
 			if (pf2->ih_fct) {
 				if (ihcnt == 0) {
-					s = splraise(pf2->ih_ipl);
+					s = pcmcia_splset(pf2->ih_ipl);
 					hiipl = pf2->ih_ipl;
-					ihcnt++;
 				} else {
-					splraise(pf2->ih_ipl);
-					if (pf2->ih_ipl > hiipl)
+					if (pcmcia_splcmp(pf2->ih_ipl, hiipl) <= 0)
 						hiipl = pf2->ih_ipl;
+					else
+						(void)pcmcia_splset(pf2->ih_ipl);
 				}
+				ihcnt++;
 			}
 		}
 
@@ -566,7 +650,7 @@
 
 			pf->sc->ih = pcmcia_chip_intr_establish(pf->sc->pct,
 			    pf->sc->pch, pf, ipl, pcmcia_card_intr, pf->sc);
-		} else if (ipl > hiipl) {
+		} else if (pcmcia_splcmp(ipl, hiipl) < 0) {
 #ifdef DIAGNOSTIC
 			if (pf->sc->ih == NULL)
 				panic("functions have ih, but the card does not");
@@ -618,10 +702,8 @@
 		 */
 
 		ihcnt = 0;
-		s = 0;		/* this is only here to keep the compipler
-				   happy */
-		hiipl = 0;	/* this is only here to keep the compipler
-				   happy */
+		s = 0;		/* Appease gcc */
+		hiipl = 0;	/* Appease gcc */
 
 		for (pf2 = pf->sc->card.pf_head.sqh_first; pf2 != NULL;
 		     pf2 = pf2->pf_list.sqe_next) {
@@ -630,14 +712,15 @@
 
 			if (pf2->ih_fct) {
 				if (ihcnt == 0) {
-					s = splraise(pf2->ih_ipl);
 					hiipl = pf2->ih_ipl;
-					ihcnt++;
+					s = pcmcia_splset(pf2->ih_ipl);
 				} else {
-					splraise(pf2->ih_ipl);
-					if (pf2->ih_ipl > hiipl)
+					if (pcmcia_splcmp(pf2->ih_ipl, hiipl) <= 0)
 						hiipl = pf2->ih_ipl;
+					else
+						(void)pcmcia_splset(pf2->ih_ipl);
 				}
+				ihcnt++;
 			}
 		}
 
@@ -664,7 +747,7 @@
 			pcmcia_chip_intr_disestablish(pf->sc->pct, pf->sc->pch,
 			    pf->sc->ih);
 			pf->sc->ih = NULL;
-		} else if (pf->ih_ipl > hiipl) {
+		} else if (pcmcia_splcmp(pf->ih_ipl, hiipl) < 0) {
 #ifdef DIAGNOSTIC
 			if (pf->sc->ih == NULL)
 				panic("changing ih ipl, but card has no ih");
>Audit-Trail:
>Unformatted: