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

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

[Adam Belay]

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

--- a/drivers/pcmcia/rsrc_mgr.c	2004-08-05 13:05:45.000000000 +0000
+++ b/drivers/pcmcia/rsrc_mgr.c	2004-08-05 21:31:32.000000000 +0000
@@ -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);

Re: [PATCH] pcmcia driver model support [4/5]

[Adam Belay]

On Fri, Aug 06, 2004 at 11:43:20AM +0100, Russell King wrote:
> On Thu, Aug 05, 2004 at 10:28:20PM +0000, Adam Belay wrote:
> > 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.

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

--- a/drivers/pcmcia/cs.c	2004-08-05 21:28:48.000000000 +0000
+++ b/drivers/pcmcia/cs.c	2004-08-06 09:52:47.000000000 +0000
@@ -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;

Re: [PATCH] pcmcia driver model support [4/5]

[Russell King]

On Thu, Aug 05, 2004 at 10:28:20PM +0000, Adam Belay wrote:
> 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.

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

Re: [PATCH] pcmcia driver model support [4/5]

[Adam Belay]

On Fri, Aug 06, 2004 at 10:35:38AM +0000, Adam Belay wrote:
> On Fri, Aug 06, 2004 at 11:43:20AM +0100, Russell King wrote:
> > On Thu, Aug 05, 2004 at 10:28:20PM +0000, Adam Belay wrote:
> > > 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.
> 
> > 
> > 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.

--- a/drivers/pcmcia/cs.c	2004-08-05 21:28:48.000000000 +0000
+++ b/drivers/pcmcia/cs.c	2004-08-06 10:42:34.000000000 +0000
@@ -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;
 		}
 

Re: [PATCH] pcmcia driver model support [4/5]

[Russell King]

On Thu, Aug 05, 2004 at 10:28:20PM +0000, Adam Belay wrote:
>...
> 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?

--- ../bk/linux-2.6-pcmcia/drivers/pcmcia/rsrc_mgr.c	Sun Aug 22 15:42:36 2004
+++ drivers/pcmcia/rsrc_mgr.c	Mon Aug 23 18:01:10 2004
@@ -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);


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

Re: [PATCH] pcmcia driver model support [4/5]

[Russell King]

On Mon, Aug 23, 2004 at 07:05:39PM +0100, Russell King wrote:
> On Thu, Aug 05, 2004 at 10:28:20PM +0000, Adam Belay wrote:
> >...
> > 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.)

--- linux-2.6-pcmcia/drivers/pcmcia/rsrc_mgr.c	Sun Aug 22 15:42:36 2004
+++ linux/drivers/pcmcia/rsrc_mgr.c	Mon Aug 23 19:11:40 2004
@@ -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);


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