Skip to contentSkip to conversation

pcmcia driver model support [4/5]

PATCH In review Adam Belay 2 participants 6 messages Aug 6, 2004 – Aug 23, 2004 Pcmcia

lore
Share & tools
lore

Adam Belay

author
#0 lore raw

PATCH 1/1 pcmcia driver model support [4/5]

[PCMCIA] fix eject lockup

It is not safe to use the skt_sem in pcmcia_validate_mem. This patch fixes a real world bug, and without it many systems will fail to shutdown properly. When pcmcia-cs calls DS_EJECT_CARD, it creates a CS_EVENT_EJECTION_REQUEST event. The event is then eventually reported to the ds.c client. DS then informs userspace of the ejection request and waits for userspace to reply with whether the request was successful. pcmcia-cs, in turn, calls DS_GET_FIRST_TUPLE while verifying the ejection request. Unfortunately, at this point the skt_sem semaphore is already held by pcmcia_eject_card. This results in the ds event code waiting forever for skt_sem to become available.

drivers/pcmcia/rsrc_mgr.c +0 -4
@@ -520,12 +520,8 @@
 
 void pcmcia_validate_mem(struct pcmcia_socket *s)
 {
-	down(&s->skt_sem);
-
 	if (probe_mem && s->state & SOCKET_PRESENT)
 		validate_mem(s);
-
-	up(&s->skt_sem);
 }
 
 EXPORT_SYMBOL(pcmcia_validate_mem);
#1 lore raw
Quoted 3 lines · Adam Belay

It is not safe to use the skt_sem in pcmcia_validate_mem. This patch fixes a real world bug, and without it many systems will fail to shutdown properly.

However, we need to take this semaphore here to prevent the socket state changing. It sounds from your description that we're hitting yet another stupid recursion bug in PCMCIA...

It sounds like we shouldn't be holding skt_sem when we wait for userspace to reply to the ejection request.

Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core
author
#2 lore raw
Quoted 8 lines · Russell King
Quoted 3 lines · Adam Belay

It is not safe to use the skt_sem in pcmcia_validate_mem. This patch fixes a real world bug, and without it many systems will fail to shutdown properly.

However, we need to take this semaphore here to prevent the socket state changing. It sounds from your description that we're hitting yet another stupid recursion bug in PCMCIA...

It's worth noting that we don't hold skt_sem in pcmcia_get_first_tuple (and possibly others), but we probably should be. This may have been to prevent recursion bugs.

Quoted 3 lines

It sounds like we shouldn't be holding skt_sem when we wait for userspace to reply to the ejection request.

The situation is rather complicated. pcmcia_eject_card itself has to hold skt_sem to ensure the socket state remains correct. We could always release the semaphore while sending the event, and then grab it again. Of course we would have to check if the socket is still present a second time in the same function. How does this look (untested)?

drivers/pcmcia/cs.c +6 -0
@@ -2056,11 +2056,17 @@
 			break;
 		}
 
+		up(&skt->skt_sem);
 		ret = send_event(skt, CS_EVENT_EJECTION_REQUEST, CS_EVENT_PRI_LOW);
 		if (ret != 0) {
 			ret = -EINVAL;
 			break;
 		}
+		down(&skt->sem);
+		if (!(skt->state & SOCKET_PRESENT)) {
+			ret = -ENODEV;
+			break;
+		}
 
 		socket_remove(skt);
 		ret = 0;
author
#3 lore raw
Quoted 15 lines · Adam Belay
Quoted 8 lines · Russell King
Quoted 3 lines · Adam Belay

It is not safe to use the skt_sem in pcmcia_validate_mem. This patch fixes a real world bug, and without it many systems will fail to shutdown properly.

However, we need to take this semaphore here to prevent the socket state changing. It sounds from your description that we're hitting yet another stupid recursion bug in PCMCIA...

It's worth noting that we don't hold skt_sem in pcmcia_get_first_tuple (and possibly others), but we probably should be. This may have been to prevent recursion bugs.

Quoted 1 line
Quoted 8 lines
Quoted 2 lines

It sounds like we shouldn't be holding skt_sem when we wait for userspace to reply to the ejection request.

The situation is rather complicated. pcmcia_eject_card itself has to hold skt_sem to ensure the socket state remains correct. We could always release the semaphore while sending the event, and then grab it again. Of course we would have to check if the socket is still present a second time in the same function. How does this look (untested)?

Sorry, the last patch was incorrect.

drivers/pcmcia/cs.c +7 -2
@@ -2056,9 +2056,14 @@
 			break;
 		}
 
+		up(&skt->skt_sem);
 		ret = send_event(skt, CS_EVENT_EJECTION_REQUEST, CS_EVENT_PRI_LOW);
-		if (ret != 0) {
-			ret = -EINVAL;
+		if (ret != 0)
+			return -EINVAL;
+		down(&skt->skt_sem);
+
+		if (!(skt->state & SOCKET_PRESENT)) {
+			ret = -ENODEV;
 			break;
 		}
#4 lore raw
In reply to Adam Belay
Quoted 5 lines · Adam Belay

...
The event is then eventually reported to the ds.c client. DS then informs userspace of the ejection request and waits for userspace to reply with whether the request was successful. pcmcia-cs, in turn, calls DS_GET_FIRST_TUPLE while verifying the ejection request.

The alternative is that we avoid taking the lock when we know that we don't need to re-validate the regions. How does this look?

pcmcia/rsrc_mgr.c renamed from bk/linux-2.6-pcmcia/drivers/pcmcia/rsrc_mgr.c +38 -24
@@ -88,6 +88,9 @@
 };
 
 static DECLARE_MUTEX(rsrc_sem);
+static unsigned int rsrc_mem_probe;
+#define MEM_PROBE_LOW	(1 << 0)
+#define MEM_PROBE_HIGH	(1 << 1)
 
 #ifdef CONFIG_PCMCIA_PROBE
 

@@ -451,24 +454,22 @@
     return do_mem_probe(m->base, m->num, s);
 }
 
-static void validate_mem(struct pcmcia_socket *s)
+static void validate_mem(struct pcmcia_socket *s, unsigned int probe_mask)
 {
     resource_map_t *m, mm;
     static u_char order[] = { 0xd0, 0xe0, 0xc0, 0xf0 };
     static int hi = 0, lo = 0;
     u_long b, i, ok = 0;
-    int force_low = !(s->features & SS_CAP_PAGE_REGS);
 
-    down(&rsrc_sem);
     /* We do up to four passes through the list */
-    if (!force_low) {
-	if (hi++ || (inv_probe(mem_db.next, s) > 0))
-	    goto out;
+    if (probe_mask & MEM_PROBE_HIGH) {
+	if (inv_probe(mem_db.next, s) > 0)
+	    return;
 	printk(KERN_NOTICE "cs: warning: no high memory space "
 	       "available!\n");
     }
-    if (lo++)
-	goto out;
+    if ((probe_mask & MEM_PROBE_LOW) == 0)
+	return;
     for (m = mem_db.next; m != &mem_db; m = mm.next) {
 	mm = *m;
 	/* Only probe < 1 MB */

@@ -488,38 +489,51 @@
 	    }
 	}
     }
- out:
-    up(&rsrc_sem);
 }
 
 #else /* CONFIG_PCMCIA_PROBE */
 
-static void validate_mem(struct pcmcia_socket *s)
+static void validate_mem(struct pcmcia_socket *s, unsigned int probe_mask)
 {
-    resource_map_t *m, mm;
-    static int done = 0;
+	resource_map_t *m, mm;
     
-    if (done++ == 0) {
-	down(&rsrc_sem);
 	for (m = mem_db.next; m != &mem_db; m = mm.next) {
-	    mm = *m;
-	    if (do_mem_probe(mm.base, mm.num, s))
-		break;
+		mm = *m;
+		if (do_mem_probe(mm.base, mm.num, s))
+			break;
 	}
-	up(&rsrc_sem);
-    }
 }
 
 #endif /* CONFIG_PCMCIA_PROBE */
 
+/*
+ * Locking note: this is the only place where we take
+ * both rsrc_sem and skt_sem.
+ */
 void pcmcia_validate_mem(struct pcmcia_socket *s)
 {
-	down(&s->skt_sem);
+	if (probe_mem) {
+		unsigned int probe_mask;
+
+		down(&rsrc_sem);
+
+		probe_mask = PROBE_MEM_LOW;
+		if (s->features & SS_CAP_PAGE_REGS)
+			probe_mask = PROBE_MEM_HIGH;
 
-	if (probe_mem && s->state & SOCKET_PRESENT)
-		validate_mem(s);
+		if (probe_mask & ~rsrc_mem_probe) {
+			rsrc_mem_probe |= probe_mask;
 
-	up(&s->skt_sem);
+			down(&s->skt_sem);
+
+			if (s->state & SOCKET_PRESENT)
+				validate_mem(s, probe_mask);
+
+			up(&s->skt_sem);
+		}
+
+		up(&rsrc_sem);
+	}
 }
 
 EXPORT_SYMBOL(pcmcia_validate_mem);
Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core
#5 lore raw
Quoted 9 lines · Russell King
Quoted 5 lines · Adam Belay

...
The event is then eventually reported to the ds.c client. DS then informs userspace of the ejection request and waits for userspace to reply with whether the request was successful. pcmcia-cs, in turn, calls DS_GET_FIRST_TUPLE while verifying the ejection request.

The alternative is that we avoid taking the lock when we know that we don't need to re-validate the regions. How does this look?

Terrible. 8) This one probably builds.

(Note: I'm expecting that this patch will have offsets, but should still apply, build and should solve your problem.)

drivers/pcmcia/rsrc_mgr.c +38 -24
@@ -88,6 +88,9 @@
 };
 
 static DECLARE_MUTEX(rsrc_sem);
+static unsigned int rsrc_mem_probe;
+#define MEM_PROBE_LOW	(1 << 0)
+#define MEM_PROBE_HIGH	(1 << 1)
 
 #ifdef CONFIG_PCMCIA_PROBE
 

@@ -451,24 +454,22 @@
     return do_mem_probe(m->base, m->num, s);
 }
 
-static void validate_mem(struct pcmcia_socket *s)
+static void validate_mem(struct pcmcia_socket *s, unsigned int probe_mask)
 {
     resource_map_t *m, mm;
     static u_char order[] = { 0xd0, 0xe0, 0xc0, 0xf0 };
     static int hi = 0, lo = 0;
     u_long b, i, ok = 0;
-    int force_low = !(s->features & SS_CAP_PAGE_REGS);
 
-    down(&rsrc_sem);
     /* We do up to four passes through the list */
-    if (!force_low) {
-	if (hi++ || (inv_probe(mem_db.next, s) > 0))
-	    goto out;
+    if (probe_mask & MEM_PROBE_HIGH) {
+	if (inv_probe(mem_db.next, s) > 0)
+	    return;
 	printk(KERN_NOTICE "cs: warning: no high memory space "
 	       "available!\n");
     }
-    if (lo++)
-	goto out;
+    if ((probe_mask & MEM_PROBE_LOW) == 0)
+	return;
     for (m = mem_db.next; m != &mem_db; m = mm.next) {
 	mm = *m;
 	/* Only probe < 1 MB */

@@ -488,38 +489,51 @@
 	    }
 	}
     }
- out:
-    up(&rsrc_sem);
 }
 
 #else /* CONFIG_PCMCIA_PROBE */
 
-static void validate_mem(struct pcmcia_socket *s)
+static void validate_mem(struct pcmcia_socket *s, unsigned int probe_mask)
 {
-    resource_map_t *m, mm;
-    static int done = 0;
+	resource_map_t *m, mm;
     
-    if (done++ == 0) {
-	down(&rsrc_sem);
 	for (m = mem_db.next; m != &mem_db; m = mm.next) {
-	    mm = *m;
-	    if (do_mem_probe(mm.base, mm.num, s))
-		break;
+		mm = *m;
+		if (do_mem_probe(mm.base, mm.num, s))
+			break;
 	}
-	up(&rsrc_sem);
-    }
 }
 
 #endif /* CONFIG_PCMCIA_PROBE */
 
+/*
+ * Locking note: this is the only place where we take
+ * both rsrc_sem and skt_sem.
+ */
 void pcmcia_validate_mem(struct pcmcia_socket *s)
 {
-	down(&s->skt_sem);
+	if (probe_mem) {
+		unsigned int probe_mask;
+
+		down(&rsrc_sem);
+
+		probe_mask = MEM_PROBE_LOW;
+		if (s->features & SS_CAP_PAGE_REGS)
+			probe_mask = MEM_PROBE_HIGH;
 
-	if (probe_mem && s->state & SOCKET_PRESENT)
-		validate_mem(s);
+		if (probe_mask & ~rsrc_mem_probe) {
+			rsrc_mem_probe |= probe_mask;
 
-	up(&s->skt_sem);
+			down(&s->skt_sem);
+
+			if (s->state & SOCKET_PRESENT)
+				validate_mem(s, probe_mask);
+
+			up(&s->skt_sem);
+		}
+
+		up(&rsrc_sem);
+	}
 }
 
 EXPORT_SYMBOL(pcmcia_validate_mem);
Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core