public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Make ppp_async callable from hard interrupt
@ 2003-12-21  0:43 Paul Mackerras
  2003-12-21  1:01 ` Greg KH
  2003-12-25 10:08 ` Marcel Sebek
  0 siblings, 2 replies; 7+ messages in thread
From: Paul Mackerras @ 2003-12-21  0:43 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

Since there are serial drivers (particularly the USB serial driver)
that call the line discipline receive_buf and write_wakeup routines at
hard interrupt level, I have changed the ppp_async code to cope with
that.  It now uses a tasklet so that it calls the generic PPP code at
soft interrupt level even if its receive_buf and write_wakeup entries
are called at hard interrupt level.

This patch has been lightly tested here with a keyspan USB serial
adaptor and also with the built-in modem on my tibook, which uses the
pmac_zilog driver (which hooks into the drivers/serial
infrastructure).

Paul.

diff -urN linux-2.5/drivers/net/ppp_async.c pmac-2.5/drivers/net/ppp_async.c
--- linux-2.5/drivers/net/ppp_async.c	2003-09-05 07:43:11.000000000 +1000
+++ pmac-2.5/drivers/net/ppp_async.c	2003-12-20 21:13:39.000000000 +1100
@@ -16,8 +16,6 @@
  * Part of the code in this driver was inspired by the old async-only
  * PPP driver, written by Michael Callahan and Al Longyear, and
  * subsequently hacked by Paul Mackerras.
- *
- * ==FILEVERSION 20020125==
  */
 
 #include <linux/module.h>
@@ -61,6 +59,9 @@
 
 	struct sk_buff	*rpkt;
 	int		lcp_fcs;
+	struct sk_buff_head rqueue;
+
+	struct tasklet_struct tsk;
 
 	atomic_t	refcnt;
 	struct semaphore dead_sem;
@@ -74,8 +75,9 @@
 #define XMIT_BUSY	2
 
 /* State bits */
-#define SC_TOSS		0x20000000
-#define SC_ESCAPE	0x40000000
+#define SC_TOSS		1
+#define SC_ESCAPE	2
+#define SC_PREV_ERROR	4
 
 /* Bits in rbits */
 #define SC_RCV_BITS	(SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
@@ -97,6 +99,8 @@
 			    char *flags, int count);
 static int ppp_async_ioctl(struct ppp_channel *chan, unsigned int cmd,
 			   unsigned long arg);
+static void ppp_async_process(unsigned long arg);
+
 static void async_lcp_peek(struct asyncppp *ap, unsigned char *data,
 			   int len, int inbound);
 
@@ -165,6 +169,9 @@
 	ap->olim = ap->obuf;
 	ap->lcp_fcs = -1;
 
+	skb_queue_head_init(&ap->rqueue);
+	tasklet_init(&ap->tsk, ppp_async_process, (unsigned long) ap);
+
 	atomic_set(&ap->refcnt, 1);
 	init_MUTEX_LOCKED(&ap->dead_sem);
 
@@ -214,10 +221,12 @@
 	 */
 	if (!atomic_dec_and_test(&ap->refcnt))
 		down(&ap->dead_sem);
+	tasklet_kill(&ap->tsk);
 
 	ppp_unregister_channel(&ap->chan);
 	if (ap->rpkt != 0)
 		kfree_skb(ap->rpkt);
+	skb_queue_purge(&ap->rqueue);
 	if (ap->tpkt != 0)
 		kfree_skb(ap->tpkt);
 	kfree(ap);
@@ -316,17 +325,24 @@
 	return 65535;
 }
 
+/*
+ * This can now be called from hard interrupt level as well
+ * as soft interrupt level or mainline.
+ */
 static void
 ppp_asynctty_receive(struct tty_struct *tty, const unsigned char *buf,
-		  char *flags, int count)
+		  char *cflags, int count)
 {
 	struct asyncppp *ap = ap_get(tty);
+	unsigned long flags;
 
 	if (ap == 0)
 		return;
-	spin_lock_bh(&ap->recv_lock);
-	ppp_async_input(ap, buf, flags, count);
-	spin_unlock_bh(&ap->recv_lock);
+	spin_lock_irqsave(&ap->recv_lock, flags);
+	ppp_async_input(ap, buf, cflags, count);
+	spin_unlock_irqrestore(&ap->recv_lock, flags);
+	if (skb_queue_len(&ap->rqueue))
+		tasklet_schedule(&ap->tsk);
 	ap_put(ap);
 	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags)
 	    && tty->driver->unthrottle)
@@ -341,8 +357,8 @@
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	if (ap == 0)
 		return;
-	if (ppp_async_push(ap))
-		ppp_output_wakeup(&ap->chan);
+	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
+	tasklet_schedule(&ap->tsk);
 	ap_put(ap);
 }
 
@@ -396,9 +412,9 @@
 		if (get_user(val, (int *) arg))
 			break;
 		ap->flags = val & ~SC_RCV_BITS;
-		spin_lock_bh(&ap->recv_lock);
+		spin_lock_irq(&ap->recv_lock);
 		ap->rbits = val & SC_RCV_BITS;
-		spin_unlock_bh(&ap->recv_lock);
+		spin_unlock_irq(&ap->recv_lock);
 		err = 0;
 		break;
 
@@ -460,6 +476,28 @@
 }
 
 /*
+ * This is called at softirq level to deliver received packets
+ * to the ppp_generic code, and to tell the ppp_generic code
+ * if we can accept more output now.
+ */
+static void ppp_async_process(unsigned long arg)
+{
+	struct asyncppp *ap = (struct asyncppp *) arg;
+	struct sk_buff *skb;
+
+	/* process received packets */
+	while ((skb = skb_dequeue(&ap->rqueue)) != NULL) {
+		if (skb->cb[0])
+			ppp_input_error(&ap->chan, 0);
+		ppp_input(&ap->chan, skb);
+	}
+
+	/* try to push more stuff out */
+	if (test_bit(XMIT_WAKEUP, &ap->xmit_flags) && ppp_async_push(ap))
+		ppp_output_wakeup(&ap->chan);
+}
+
+/*
  * Procedures for encapsulation and framing.
  */
 
@@ -641,7 +679,6 @@
 	struct tty_struct *tty = ap->tty;
 	int tty_stuffed = 0;
 
-	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
 	/*
 	 * We can get called recursively here if the tty write
 	 * function calls our wakeup function.  This can happen
@@ -752,22 +789,19 @@
 }
 
 /* called when a flag is seen - do end-of-packet processing */
-static inline void
+static void
 process_input_packet(struct asyncppp *ap)
 {
 	struct sk_buff *skb;
 	unsigned char *p;
 	unsigned int len, fcs, proto;
-	int code = 0;
+
+	if (ap->state & (SC_TOSS | SC_ESCAPE))
+		goto err;
 
 	skb = ap->rpkt;
-	ap->rpkt = 0;
-	if ((ap->state & (SC_TOSS | SC_ESCAPE)) || skb == 0) {
-		ap->state &= ~(SC_TOSS | SC_ESCAPE);
-		if (skb != 0)
-			kfree_skb(skb);
-		return;
-	}
+	if (skb == NULL)
+		return;		/* 0-length packet */
 
 	/* check the FCS */
 	p = skb->data;
@@ -801,20 +835,18 @@
 			async_lcp_peek(ap, p, skb->len, 1);
 	}
 
-	/* all OK, give it to the generic layer */
-	ppp_input(&ap->chan, skb);
+	/* queue the frame to be processed */
+	skb->cb[0] = ap->state;
+	skb_queue_tail(&ap->rqueue, skb);
+	ap->rpkt = 0;
+	ap->state = 0;
 	return;
 
  err:
-	kfree_skb(skb);
-	ppp_input_error(&ap->chan, code);
-}
-
-static inline void
-input_error(struct asyncppp *ap, int code)
-{
-	ap->state |= SC_TOSS;
-	ppp_input_error(&ap->chan, code);
+	/* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */
+	ap->state = SC_PREV_ERROR;
+	if (skb)
+		skb_trim(skb, 0);
 }
 
 /* called when the tty driver has data for us. */
@@ -856,7 +888,7 @@
 		}
 		if (f != 0) {
 			/* start tossing */
-			input_error(ap, f);
+			ap->state |= SC_TOSS;
 
 		} else if (n > 0 && (ap->state & SC_TOSS) == 0) {
 			/* stuff the chars in the skb */
@@ -872,7 +904,7 @@
 			}
 			if (n > skb_tailroom(skb)) {
 				/* packet overflowed MRU */
-				input_error(ap, 1);
+				ap->state |= SC_TOSS;
 			} else {
 				sp = skb_put(skb, n);
 				memcpy(sp, buf, n);
@@ -909,7 +941,7 @@
 
  nomem:
 	printk(KERN_ERR "PPPasync: no memory (input pkt)\n");
-	input_error(ap, 0);
+	ap->state |= SC_TOSS;
 }
 
 /*

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Make ppp_async callable from hard interrupt
  2003-12-21  0:43 [PATCH] Make ppp_async callable from hard interrupt Paul Mackerras
@ 2003-12-21  1:01 ` Greg KH
  2003-12-25 10:08 ` Marcel Sebek
  1 sibling, 0 replies; 7+ messages in thread
From: Greg KH @ 2003-12-21  1:01 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: akpm, linux-kernel

On Sun, Dec 21, 2003 at 11:43:33AM +1100, Paul Mackerras wrote:
> Since there are serial drivers (particularly the USB serial driver)
> that call the line discipline receive_buf and write_wakeup routines at
> hard interrupt level, I have changed the ppp_async code to cope with
> that.  It now uses a tasklet so that it calls the generic PPP code at
> soft interrupt level even if its receive_buf and write_wakeup entries
> are called at hard interrupt level.

Ah, nice.  Does this mean I can take the "make usb serial drivers use a
tasklet to handle received data" entry off of my todo list now?  :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Make ppp_async callable from hard interrupt
  2003-12-21  0:43 [PATCH] Make ppp_async callable from hard interrupt Paul Mackerras
  2003-12-21  1:01 ` Greg KH
@ 2003-12-25 10:08 ` Marcel Sebek
  2003-12-25 10:22   ` Andrew Morton
  2003-12-25 22:29   ` [PATCH] " Paul Mackerras
  1 sibling, 2 replies; 7+ messages in thread
From: Marcel Sebek @ 2003-12-25 10:08 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: akpm, linux-kernel

On Sun, Dec 21, 2003 at 11:43:33AM +1100, Paul Mackerras wrote:
>  /* called when a flag is seen - do end-of-packet processing */
> -static inline void
> +static void
>  process_input_packet(struct asyncppp *ap)
>  {
>  	struct sk_buff *skb;
>  	unsigned char *p;
>  	unsigned int len, fcs, proto;
> -	int code = 0;
> +
> +	if (ap->state & (SC_TOSS | SC_ESCAPE))
> +		goto err;
If this is true, skb will be used uninitialized.

>  
>  	skb = ap->rpkt;
> -	ap->rpkt = 0;
> -	if ((ap->state & (SC_TOSS | SC_ESCAPE)) || skb == 0) {
> -		ap->state &= ~(SC_TOSS | SC_ESCAPE);
> -		if (skb != 0)
> -			kfree_skb(skb);
> -		return;
> -	}
> +	if (skb == NULL)
> +		return;		/* 0-length packet */


-- 
Marcel Sebek
jabber: sebek@jabber.cz                     ICQ: 279852819
linux user number: 307850                 GPG ID: 5F88735E
GPG FP: 0F01 BAB8 3148 94DB B95D  1FCA 8B63 CA06 5F88 735E


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Make ppp_async callable from hard interrupt
  2003-12-25 10:08 ` Marcel Sebek
@ 2003-12-25 10:22   ` Andrew Morton
  2003-12-25 20:59     ` Kiko Piris
  2003-12-25 22:29   ` [PATCH] " Paul Mackerras
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2003-12-25 10:22 UTC (permalink / raw)
  To: Marcel Sebek; +Cc: paulus, linux-kernel

sebek64@post.cz (Marcel Sebek) wrote:
>
> On Sun, Dec 21, 2003 at 11:43:33AM +1100, Paul Mackerras wrote:
>  >  /* called when a flag is seen - do end-of-packet processing */
>  > -static inline void
>  > +static void
>  >  process_input_packet(struct asyncppp *ap)
>  >  {
>  >  	struct sk_buff *skb;
>  >  	unsigned char *p;
>  >  	unsigned int len, fcs, proto;
>  > -	int code = 0;
>  > +
>  > +	if (ap->state & (SC_TOSS | SC_ESCAPE))
>  > +		goto err;
>  If this is true, skb will be used uninitialized.

True.    Here's an updated patch.


diff -puN drivers/net/ppp_async.c~ppp_async-locking-fix drivers/net/ppp_async.c
--- 25/drivers/net/ppp_async.c~ppp_async-locking-fix	2003-12-25 02:22:14.000000000 -0800
+++ 25-akpm/drivers/net/ppp_async.c	2003-12-25 02:22:14.000000000 -0800
@@ -16,8 +16,6 @@
  * Part of the code in this driver was inspired by the old async-only
  * PPP driver, written by Michael Callahan and Al Longyear, and
  * subsequently hacked by Paul Mackerras.
- *
- * ==FILEVERSION 20020125==
  */
 
 #include <linux/module.h>
@@ -61,6 +59,9 @@ struct asyncppp {
 
 	struct sk_buff	*rpkt;
 	int		lcp_fcs;
+	struct sk_buff_head rqueue;
+
+	struct tasklet_struct tsk;
 
 	atomic_t	refcnt;
 	struct semaphore dead_sem;
@@ -74,8 +75,9 @@ struct asyncppp {
 #define XMIT_BUSY	2
 
 /* State bits */
-#define SC_TOSS		0x20000000
-#define SC_ESCAPE	0x40000000
+#define SC_TOSS		1
+#define SC_ESCAPE	2
+#define SC_PREV_ERROR	4
 
 /* Bits in rbits */
 #define SC_RCV_BITS	(SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
@@ -97,6 +99,8 @@ static void ppp_async_input(struct async
 			    char *flags, int count);
 static int ppp_async_ioctl(struct ppp_channel *chan, unsigned int cmd,
 			   unsigned long arg);
+static void ppp_async_process(unsigned long arg);
+
 static void async_lcp_peek(struct asyncppp *ap, unsigned char *data,
 			   int len, int inbound);
 
@@ -165,6 +169,9 @@ ppp_asynctty_open(struct tty_struct *tty
 	ap->olim = ap->obuf;
 	ap->lcp_fcs = -1;
 
+	skb_queue_head_init(&ap->rqueue);
+	tasklet_init(&ap->tsk, ppp_async_process, (unsigned long) ap);
+
 	atomic_set(&ap->refcnt, 1);
 	init_MUTEX_LOCKED(&ap->dead_sem);
 
@@ -214,10 +221,12 @@ ppp_asynctty_close(struct tty_struct *tt
 	 */
 	if (!atomic_dec_and_test(&ap->refcnt))
 		down(&ap->dead_sem);
+	tasklet_kill(&ap->tsk);
 
 	ppp_unregister_channel(&ap->chan);
 	if (ap->rpkt != 0)
 		kfree_skb(ap->rpkt);
+	skb_queue_purge(&ap->rqueue);
 	if (ap->tpkt != 0)
 		kfree_skb(ap->tpkt);
 	kfree(ap);
@@ -316,17 +325,24 @@ ppp_asynctty_room(struct tty_struct *tty
 	return 65535;
 }
 
+/*
+ * This can now be called from hard interrupt level as well
+ * as soft interrupt level or mainline.
+ */
 static void
 ppp_asynctty_receive(struct tty_struct *tty, const unsigned char *buf,
-		  char *flags, int count)
+		  char *cflags, int count)
 {
 	struct asyncppp *ap = ap_get(tty);
+	unsigned long flags;
 
 	if (ap == 0)
 		return;
-	spin_lock_bh(&ap->recv_lock);
-	ppp_async_input(ap, buf, flags, count);
-	spin_unlock_bh(&ap->recv_lock);
+	spin_lock_irqsave(&ap->recv_lock, flags);
+	ppp_async_input(ap, buf, cflags, count);
+	spin_unlock_irqrestore(&ap->recv_lock, flags);
+	if (skb_queue_len(&ap->rqueue))
+		tasklet_schedule(&ap->tsk);
 	ap_put(ap);
 	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags)
 	    && tty->driver->unthrottle)
@@ -341,8 +357,8 @@ ppp_asynctty_wakeup(struct tty_struct *t
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	if (ap == 0)
 		return;
-	if (ppp_async_push(ap))
-		ppp_output_wakeup(&ap->chan);
+	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
+	tasklet_schedule(&ap->tsk);
 	ap_put(ap);
 }
 
@@ -396,9 +412,9 @@ ppp_async_ioctl(struct ppp_channel *chan
 		if (get_user(val, (int *) arg))
 			break;
 		ap->flags = val & ~SC_RCV_BITS;
-		spin_lock_bh(&ap->recv_lock);
+		spin_lock_irq(&ap->recv_lock);
 		ap->rbits = val & SC_RCV_BITS;
-		spin_unlock_bh(&ap->recv_lock);
+		spin_unlock_irq(&ap->recv_lock);
 		err = 0;
 		break;
 
@@ -460,6 +476,28 @@ ppp_async_ioctl(struct ppp_channel *chan
 }
 
 /*
+ * This is called at softirq level to deliver received packets
+ * to the ppp_generic code, and to tell the ppp_generic code
+ * if we can accept more output now.
+ */
+static void ppp_async_process(unsigned long arg)
+{
+	struct asyncppp *ap = (struct asyncppp *) arg;
+	struct sk_buff *skb;
+
+	/* process received packets */
+	while ((skb = skb_dequeue(&ap->rqueue)) != NULL) {
+		if (skb->cb[0])
+			ppp_input_error(&ap->chan, 0);
+		ppp_input(&ap->chan, skb);
+	}
+
+	/* try to push more stuff out */
+	if (test_bit(XMIT_WAKEUP, &ap->xmit_flags) && ppp_async_push(ap))
+		ppp_output_wakeup(&ap->chan);
+}
+
+/*
  * Procedures for encapsulation and framing.
  */
 
@@ -641,7 +679,6 @@ ppp_async_push(struct asyncppp *ap)
 	struct tty_struct *tty = ap->tty;
 	int tty_stuffed = 0;
 
-	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
 	/*
 	 * We can get called recursively here if the tty write
 	 * function calls our wakeup function.  This can happen
@@ -752,22 +789,19 @@ scan_ordinary(struct asyncppp *ap, const
 }
 
 /* called when a flag is seen - do end-of-packet processing */
-static inline void
+static void
 process_input_packet(struct asyncppp *ap)
 {
-	struct sk_buff *skb;
+	struct sk_buff *skb = NULL;
 	unsigned char *p;
 	unsigned int len, fcs, proto;
-	int code = 0;
+
+	if (ap->state & (SC_TOSS | SC_ESCAPE))
+		goto err;
 
 	skb = ap->rpkt;
-	ap->rpkt = 0;
-	if ((ap->state & (SC_TOSS | SC_ESCAPE)) || skb == 0) {
-		ap->state &= ~(SC_TOSS | SC_ESCAPE);
-		if (skb != 0)
-			kfree_skb(skb);
-		return;
-	}
+	if (skb == NULL)
+		return;		/* 0-length packet */
 
 	/* check the FCS */
 	p = skb->data;
@@ -801,20 +835,18 @@ process_input_packet(struct asyncppp *ap
 			async_lcp_peek(ap, p, skb->len, 1);
 	}
 
-	/* all OK, give it to the generic layer */
-	ppp_input(&ap->chan, skb);
+	/* queue the frame to be processed */
+	skb->cb[0] = ap->state;
+	skb_queue_tail(&ap->rqueue, skb);
+	ap->rpkt = 0;
+	ap->state = 0;
 	return;
 
  err:
-	kfree_skb(skb);
-	ppp_input_error(&ap->chan, code);
-}
-
-static inline void
-input_error(struct asyncppp *ap, int code)
-{
-	ap->state |= SC_TOSS;
-	ppp_input_error(&ap->chan, code);
+	/* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */
+	ap->state = SC_PREV_ERROR;
+	if (skb)
+		skb_trim(skb, 0);
 }
 
 /* called when the tty driver has data for us. */
@@ -856,7 +888,7 @@ ppp_async_input(struct asyncppp *ap, con
 		}
 		if (f != 0) {
 			/* start tossing */
-			input_error(ap, f);
+			ap->state |= SC_TOSS;
 
 		} else if (n > 0 && (ap->state & SC_TOSS) == 0) {
 			/* stuff the chars in the skb */
@@ -872,7 +904,7 @@ ppp_async_input(struct asyncppp *ap, con
 			}
 			if (n > skb_tailroom(skb)) {
 				/* packet overflowed MRU */
-				input_error(ap, 1);
+				ap->state |= SC_TOSS;
 			} else {
 				sp = skb_put(skb, n);
 				memcpy(sp, buf, n);
@@ -909,7 +941,7 @@ ppp_async_input(struct asyncppp *ap, con
 
  nomem:
 	printk(KERN_ERR "PPPasync: no memory (input pkt)\n");
-	input_error(ap, 0);
+	ap->state |= SC_TOSS;
 }
 
 /*

_


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Make ppp_async callable from hard interrupt
  2003-12-25 10:22   ` Andrew Morton
@ 2003-12-25 20:59     ` Kiko Piris
  2003-12-25 22:09       ` Martin Schlemmer
  0 siblings, 1 reply; 7+ messages in thread
From: Kiko Piris @ 2003-12-25 20:59 UTC (permalink / raw)
  To: linux-kernel

On 25/12/2003 at 02:22, Andrew Morton wrote:

> (Marcel Sebek) wrote:

> >  If this is true, skb will be used uninitialized.
> 
> True.    Here's an updated patch.

I applied the first patch to 2.6.0-ben1 (benH's tree) and had a couple
of hard freezes on my iBook. Not on the first ppp connection since boot,
but around 4th or 5th).

I _suspect_ (with absolutely no evidence) this was the same problem as
the ppp oopses pointed by other people with 2.6.0-mm1.

But I was under X and nothing was written to the logs (ie. no info at
all about the error), so I couldn't tell if the culprit was ppp_async.

And this was the very first time I used my modem (rs-232 with usb-serial
/ ftdi_sio) with 2.6, so I had too little info to post anything here.


But the fact is that with this lastest patch (w/ the correction pointed
by Marcel Sebek, everything seems to work ok here (about half a dozen
connections w/o problems).

Just wanted to add some info in case it could be useful to track this
issue.

Feel free to ask any additional info, I'll be glad if I can help in any
way.

A big thank you to everyone and season's greetings

-- 
Kiko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Make ppp_async callable from hard interrupt
  2003-12-25 20:59     ` Kiko Piris
@ 2003-12-25 22:09       ` Martin Schlemmer
  0 siblings, 0 replies; 7+ messages in thread
From: Martin Schlemmer @ 2003-12-25 22:09 UTC (permalink / raw)
  To: Linux Kernel Mailing Lists

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Thu, 2003-12-25 at 22:59, Kiko Piris wrote:
> On 25/12/2003 at 02:22, Andrew Morton wrote:
> 
> > (Marcel Sebek) wrote:
> 
> > >  If this is true, skb will be used uninitialized.
> > 
> > True.    Here's an updated patch.
> 

> I _suspect_ (with absolutely no evidence) this was the same problem as
> the ppp oopses pointed by other people with 2.6.0-mm1.
> 

Yep.  Andrew, the oops's I got that was similar to those of users of
-mm1 is now gone.


Cheers,

-- 
Martin Schlemmer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Make ppp_async callable from hard interrupt
  2003-12-25 10:08 ` Marcel Sebek
  2003-12-25 10:22   ` Andrew Morton
@ 2003-12-25 22:29   ` Paul Mackerras
  1 sibling, 0 replies; 7+ messages in thread
From: Paul Mackerras @ 2003-12-25 22:29 UTC (permalink / raw)
  To: Marcel Sebek; +Cc: akpm, linux-kernel

Marcel Sebek writes:

> > +	if (ap->state & (SC_TOSS | SC_ESCAPE))
> > +		goto err;
> If this is true, skb will be used uninitialized.

True.  Thanks for finding that.  The solution is of course to move the
line that does skb = ap->rpkt before that test.

Here is a new patch (which is slightly different from the one Andrew
did, since Andrew's version won't reset the skb to 0 length on error).

Regards,
Paul.

--- linux-2.5/drivers/net/ppp_async.c	2003-09-05 07:43:11.000000000 +1000
+++ pmac-2.5/drivers/net/ppp_async.c	2003-12-26 09:24:12.317339048 +1100
@@ -16,8 +16,6 @@
  * Part of the code in this driver was inspired by the old async-only
  * PPP driver, written by Michael Callahan and Al Longyear, and
  * subsequently hacked by Paul Mackerras.
- *
- * ==FILEVERSION 20020125==
  */
 
 #include <linux/module.h>
@@ -61,6 +59,9 @@
 
 	struct sk_buff	*rpkt;
 	int		lcp_fcs;
+	struct sk_buff_head rqueue;
+
+	struct tasklet_struct tsk;
 
 	atomic_t	refcnt;
 	struct semaphore dead_sem;
@@ -74,8 +75,9 @@
 #define XMIT_BUSY	2
 
 /* State bits */
-#define SC_TOSS		0x20000000
-#define SC_ESCAPE	0x40000000
+#define SC_TOSS		1
+#define SC_ESCAPE	2
+#define SC_PREV_ERROR	4
 
 /* Bits in rbits */
 #define SC_RCV_BITS	(SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
@@ -97,6 +99,8 @@
 			    char *flags, int count);
 static int ppp_async_ioctl(struct ppp_channel *chan, unsigned int cmd,
 			   unsigned long arg);
+static void ppp_async_process(unsigned long arg);
+
 static void async_lcp_peek(struct asyncppp *ap, unsigned char *data,
 			   int len, int inbound);
 
@@ -165,6 +169,9 @@
 	ap->olim = ap->obuf;
 	ap->lcp_fcs = -1;
 
+	skb_queue_head_init(&ap->rqueue);
+	tasklet_init(&ap->tsk, ppp_async_process, (unsigned long) ap);
+
 	atomic_set(&ap->refcnt, 1);
 	init_MUTEX_LOCKED(&ap->dead_sem);
 
@@ -214,10 +221,12 @@
 	 */
 	if (!atomic_dec_and_test(&ap->refcnt))
 		down(&ap->dead_sem);
+	tasklet_kill(&ap->tsk);
 
 	ppp_unregister_channel(&ap->chan);
 	if (ap->rpkt != 0)
 		kfree_skb(ap->rpkt);
+	skb_queue_purge(&ap->rqueue);
 	if (ap->tpkt != 0)
 		kfree_skb(ap->tpkt);
 	kfree(ap);
@@ -316,17 +325,24 @@
 	return 65535;
 }
 
+/*
+ * This can now be called from hard interrupt level as well
+ * as soft interrupt level or mainline.
+ */
 static void
 ppp_asynctty_receive(struct tty_struct *tty, const unsigned char *buf,
-		  char *flags, int count)
+		  char *cflags, int count)
 {
 	struct asyncppp *ap = ap_get(tty);
+	unsigned long flags;
 
 	if (ap == 0)
 		return;
-	spin_lock_bh(&ap->recv_lock);
-	ppp_async_input(ap, buf, flags, count);
-	spin_unlock_bh(&ap->recv_lock);
+	spin_lock_irqsave(&ap->recv_lock, flags);
+	ppp_async_input(ap, buf, cflags, count);
+	spin_unlock_irqrestore(&ap->recv_lock, flags);
+	if (skb_queue_len(&ap->rqueue))
+		tasklet_schedule(&ap->tsk);
 	ap_put(ap);
 	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags)
 	    && tty->driver->unthrottle)
@@ -341,8 +357,8 @@
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	if (ap == 0)
 		return;
-	if (ppp_async_push(ap))
-		ppp_output_wakeup(&ap->chan);
+	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
+	tasklet_schedule(&ap->tsk);
 	ap_put(ap);
 }
 
@@ -396,9 +412,9 @@
 		if (get_user(val, (int *) arg))
 			break;
 		ap->flags = val & ~SC_RCV_BITS;
-		spin_lock_bh(&ap->recv_lock);
+		spin_lock_irq(&ap->recv_lock);
 		ap->rbits = val & SC_RCV_BITS;
-		spin_unlock_bh(&ap->recv_lock);
+		spin_unlock_irq(&ap->recv_lock);
 		err = 0;
 		break;
 
@@ -460,6 +476,28 @@
 }
 
 /*
+ * This is called at softirq level to deliver received packets
+ * to the ppp_generic code, and to tell the ppp_generic code
+ * if we can accept more output now.
+ */
+static void ppp_async_process(unsigned long arg)
+{
+	struct asyncppp *ap = (struct asyncppp *) arg;
+	struct sk_buff *skb;
+
+	/* process received packets */
+	while ((skb = skb_dequeue(&ap->rqueue)) != NULL) {
+		if (skb->cb[0])
+			ppp_input_error(&ap->chan, 0);
+		ppp_input(&ap->chan, skb);
+	}
+
+	/* try to push more stuff out */
+	if (test_bit(XMIT_WAKEUP, &ap->xmit_flags) && ppp_async_push(ap))
+		ppp_output_wakeup(&ap->chan);
+}
+
+/*
  * Procedures for encapsulation and framing.
  */
 
@@ -641,7 +679,6 @@
 	struct tty_struct *tty = ap->tty;
 	int tty_stuffed = 0;
 
-	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
 	/*
 	 * We can get called recursively here if the tty write
 	 * function calls our wakeup function.  This can happen
@@ -752,22 +789,19 @@
 }
 
 /* called when a flag is seen - do end-of-packet processing */
-static inline void
+static void
 process_input_packet(struct asyncppp *ap)
 {
 	struct sk_buff *skb;
 	unsigned char *p;
 	unsigned int len, fcs, proto;
-	int code = 0;
 
 	skb = ap->rpkt;
-	ap->rpkt = 0;
-	if ((ap->state & (SC_TOSS | SC_ESCAPE)) || skb == 0) {
-		ap->state &= ~(SC_TOSS | SC_ESCAPE);
-		if (skb != 0)
-			kfree_skb(skb);
-		return;
-	}
+	if (ap->state & (SC_TOSS | SC_ESCAPE))
+		goto err;
+
+	if (skb == NULL)
+		return;		/* 0-length packet */
 
 	/* check the FCS */
 	p = skb->data;
@@ -801,20 +835,18 @@
 			async_lcp_peek(ap, p, skb->len, 1);
 	}
 
-	/* all OK, give it to the generic layer */
-	ppp_input(&ap->chan, skb);
+	/* queue the frame to be processed */
+	skb->cb[0] = ap->state;
+	skb_queue_tail(&ap->rqueue, skb);
+	ap->rpkt = 0;
+	ap->state = 0;
 	return;
 
  err:
-	kfree_skb(skb);
-	ppp_input_error(&ap->chan, code);
-}
-
-static inline void
-input_error(struct asyncppp *ap, int code)
-{
-	ap->state |= SC_TOSS;
-	ppp_input_error(&ap->chan, code);
+	/* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */
+	ap->state = SC_PREV_ERROR;
+	if (skb)
+		skb_trim(skb, 0);
 }
 
 /* called when the tty driver has data for us. */
@@ -856,7 +888,7 @@
 		}
 		if (f != 0) {
 			/* start tossing */
-			input_error(ap, f);
+			ap->state |= SC_TOSS;
 
 		} else if (n > 0 && (ap->state & SC_TOSS) == 0) {
 			/* stuff the chars in the skb */
@@ -872,7 +904,7 @@
 			}
 			if (n > skb_tailroom(skb)) {
 				/* packet overflowed MRU */
-				input_error(ap, 1);
+				ap->state |= SC_TOSS;
 			} else {
 				sp = skb_put(skb, n);
 				memcpy(sp, buf, n);
@@ -909,7 +941,7 @@
 
  nomem:
 	printk(KERN_ERR "PPPasync: no memory (input pkt)\n");
-	input_error(ap, 0);
+	ap->state |= SC_TOSS;
 }
 
 /*

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2003-12-25 22:25 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-12-21  0:43 [PATCH] Make ppp_async callable from hard interrupt Paul Mackerras
2003-12-21  1:01 ` Greg KH
2003-12-25 10:08 ` Marcel Sebek
2003-12-25 10:22   ` Andrew Morton
2003-12-25 20:59     ` Kiko Piris
2003-12-25 22:09       ` Martin Schlemmer
2003-12-25 22:29   ` [PATCH] " Paul Mackerras

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox