Badness in local_bh_enable at kernel/softirq.c:122

Badness in local_bh_enable at kernel/softirq.c:122

[Jurjen Oskam]

Hi there,

I'm using pptp-linux to connect to my ADSL-provider. I've been doing this
for several years now, without significant problems. A week ago, I
upgraded

May 27 11:35:41 calvin kernel: Badness in local_bh_enable at kernel/softirq.c:122
May 27 11:35:41 calvin kernel: Call Trace:
May 27 11:35:41 calvin kernel:  [<c011ac00>] local_bh_enable+0x60/0x80
May 27 11:35:41 calvin kernel:  [<c4cc2b54>] ppp_sync_push+0x54/0x140 [ppp_synctty]
May 27 11:35:41 calvin kernel:  [<c4cc25a5>] ppp_sync_wakeup+0x25/0x60 [ppp_synctty]
May 27 11:35:41 calvin kernel:  [<c01f20e7>] pty_unthrottle+0x47/0x60
May 27 11:35:41 calvin kernel:  [<c01eefb1>] check_unthrottle+0x31/0x40
May 27 11:35:41 calvin kernel:  [<c01ef02b>] n_tty_flush_buffer+0xb/0x60
May 27 11:35:41 calvin kernel:  [<c01f2488>] pty_flush_buffer+0x48/0x60
May 27 11:35:41 calvin kernel:  [<c01ec116>] do_tty_hangup+0x2d6/0x340
May 27 11:35:41 calvin kernel:  [<c01ed387>] release_dev+0x547/0x580
May 27 11:35:41 calvin kernel:  [<c010c038>] timer_interrupt+0xb8/0xc0
May 27 11:35:41 calvin kernel:  [<c012544d>] rcu_check_quiescent_state+0x4d/0x60
May 27 11:35:41 calvin kernel:  [<c012551e>] rcu_process_callbacks+0xbe/0xe0
May 27 11:35:41 calvin kernel:  [<c011ad7a>] tasklet_action+0x3a/0x60
May 27 11:35:41 calvin kernel:  [<c0108805>] do_IRQ+0xc5/0xe0
May 27 11:35:41 calvin kernel:  [<c0106ed8>] common_interrupt+0x18/0x20
May 27 11:35:41 calvin kernel:  [<c01ed709>] tty_release+0x9/0x20
May 27 11:35:41 calvin kernel:  [<c0145532>] __fput+0xd2/0x100
May 27 11:35:41 calvin kernel:  [<c0143ee3>] filp_close+0x43/0x80
May 27 11:35:41 calvin kernel:  [<c0118795>] put_files_struct+0x55/0xc0
May 27 11:35:41 calvin kernel:  [<c01192eb>] do_exit+0x14b/0x2e0
May 27 11:35:41 calvin kernel:  [<c0119548>] do_group_exit+0x48/0x80
May 27 11:35:41 calvin kernel:  [<c0120bf9>] get_signal_to_deliver+0x199/0x2c0
May 27 11:35:41 calvin kernel:  [<c0106a65>] do_signal+0x45/0xc0
May 27 11:35:41 calvin kernel:  [<c01134ea>] recalc_task_prio+0x8a/0x1a0
May 27 11:35:41 calvin kernel:  [<c014463d>] vfs_read+0xbd/0xe0
May 27 11:35:41 calvin kernel:  [<c014482b>] sys_read+0x2b/0x60
May 27 11:35:41 calvin kernel:  [<c0106b29>] do_notify_resume+0x49/0x50
May 27 11:35:41 calvin kernel:  [<c0106d0e>] work_notifysig+0x13/0x15
May 27 11:35:41 calvin kernel:


Each time the pppd link was retried, the "Badness in..." message appeared.

This is on a 450 MHz K6-II.
-- 
Jurjen Oskam

"Avoid putting a paging file on a fault-tolerant drive, such as a mirrored
volume or a RAID-5 volume. Paging files do not need fault-tolerance."-MS Q308417

Re: Badness in local_bh_enable at kernel/softirq.c:122

[Jurjen Oskam]

On Thu, May 27, 2004 at 07:45:10PM +0200, Jurjen Oskam wrote:
> 
> I'm using pptp-linux to connect to my ADSL-provider. I've been doing this
> for several years now, without significant problems. A week ago, I
> upgraded

... to Slackware 9.1 and installed kernel 2.6.5.

(Also, it seems to cause random text to disappear from mails sent to
linux-kernel, but I'll post a separate re
-- 
Jurjen Oskam

"Avoid putting a paging file on a fault-tolerant drive, such as a mirrored
volume or a RAID-5 volume. Paging files do not need fault-tolerance."-MS Q308417

Re: Badness in local_bh_enable at kernel/softirq.c:122

[Paul Fulghum]

On Thu, 2004-05-27 at 12:45, Jurjen Oskam wrote:
> Hi there,
> 
> I'm using pptp-linux to connect to my ADSL-provider. I've been doing this
> for several years now, without significant problems. A week ago, I
> upgraded
> 
> May 27 11:35:41 calvin kernel: Badness in local_bh_enable at kernel/softirq.c:122
> May 27 11:35:41 calvin kernel: Call Trace:
> May 27 11:35:41 calvin kernel:  [<c011ac00>] local_bh_enable+0x60/0x80
> May 27 11:35:41 calvin kernel:  [<c4cc2b54>] ppp_sync_push+0x54/0x140 [ppp_synctty]
> May 27 11:35:41 calvin kernel:  [<c4cc25a5>] ppp_sync_wakeup+0x25/0x60 [ppp_synctty]
> May 27 11:35:41 calvin kernel:  [<c01f20e7>] pty_unthrottle+0x47/0x60
> May 27 11:35:41 calvin kernel:  [<c01eefb1>] check_unthrottle+0x31/0x40
> May 27 11:35:41 calvin kernel:  [<c01ef02b>] n_tty_flush_buffer+0xb/0x60
> May 27 11:35:41 calvin kernel:  [<c01f2488>] pty_flush_buffer+0x48/0x60
> May 27 11:35:41 calvin kernel:  [<c01ec116>] do_tty_hangup+0x2d6/0x340

I've been looking at this and the culprit is in
drivers/char/tty_io.c, function do_tty_hangup():

	lock_kernel();

        ...

	/* FIXME! What are the locking issues here? This may me overdoing things..
	* this question is especially important now that we've removed the irqlock. */
	{
		unsigned long flags;

		local_irq_save(flags); // FIXME: is this safe?
		if (tty->ldisc.flush_buffer)
			tty->ldisc.flush_buffer(tty);
		if (tty->driver->flush_buffer)
			tty->driver->flush_buffer(tty);
		if ((test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) &&
		    tty->ldisc.write_wakeup)
			(tty->ldisc.write_wakeup)(tty);
		local_irq_restore(flags); // FIXME: is this safe?
	}

        ...

	unlock_kernel();

I do not see that disabling the local CPU interrupts for the
flush_buffer and write_wakeup calls is needed. In fact
doing so provokes the warning message in softirq.c
when write_wakeup is called.

All of these functions are also invoked by ioctl calls
which do not disable the local IRQ (ioctl is protected
by lock_kernel/unlock_kernel just like do_tty_hangup).

So the flush_buffer/write_wakeup calls do *not*
need IRQs disabled (in fact need them enabled to
avoid the warning as is the case on ioctl).

Since the IRQ is restored immediately after these 3
calls, it does not seem to serve a purpose for the calling
function either.

The solution, in my opinion, is to remove the IRQ
disabling surrounding these 3 functions in do_tty_hangup.


-- 
Paul Fulghum
paulkf@microgate.com

[PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Paul Fulghum]

The following patch removes unnecessary disabling of
interrupts when processing hangup for tty devices.

This was introduced way back in August 1998
with patch 2.1.115 when the original hangup processing was
changed from cli/sti to lock_kernel()/unlock_kernel().

Up to now, this did not cause any actual problems.
In 2.6.X this causes a warning if any of the called functions
(flush_buffer/write_wakeup) call spin_xxx_bh() functions.

Warning is in spin_unlock_bh (kernel/softirq.c:122)

An example is drivers/net/ppp_synctty.c write_wakeup.
This has been reported several times by different people.

The flush_buffer/write_wakeup calls are also called
when processing ioctl for tty devices, without disabling
interrupts using only lock_kernel()/unlock_kernel().

tty->ldisc.flush_buffer()
	tty_ioctl.c
		ioctl(TCSETSF) -> set_termios()
		ioctl(TCSETAF) -> set_termios()
		ioctl(TCFLSH)

tty->driver.flush_buffer()
	tty_ioctl.c
		ioctl(TCFLSH)

tty->ldisc.write_wakeup()
	tty_ioctl.c
		ioctl(TCOON)  -> start_tty()
		ioctl(TCIOFF) -> send_prio_char() -> start_tty()
		ioctl(TCION)  -> send_prio_char() -> start_tty()

So removal of the interrupt disable/enable from around
these calls in tty_io.c do_tty_hangup(), implements locking
the same as the ioctl calls.

I have tested this patch on an SMP machine with 2.6.6.

I welcome comments and testing by others, particularly those
 who have seen the softirq.c message when doing ppp
(synchronous PPP, PPPoATM etc) that use the ppp_synctty.c

Paul Fulghum
paulkf@microgate.com


--- linux-2.6.6/drivers/char/tty_io.c	2004-05-28 08:26:47.000000000 -0500
+++ linux-2.6.6-mg1/drivers/char/tty_io.c	2004-05-28 08:31:05.000000000 -0500
@@ -442,21 +442,13 @@
 	}
 	file_list_unlock();
 	
-	/* FIXME! What are the locking issues here? This may me overdoing things..
-	* this question is especially important now that we've removed the irqlock. */
-	{
-		unsigned long flags;
-
-		local_irq_save(flags); // FIXME: is this safe?
-		if (tty->ldisc.flush_buffer)
-			tty->ldisc.flush_buffer(tty);
-		if (tty->driver->flush_buffer)
-			tty->driver->flush_buffer(tty);
-		if ((test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) &&
-		    tty->ldisc.write_wakeup)
-			(tty->ldisc.write_wakeup)(tty);
-		local_irq_restore(flags); // FIXME: is this safe?
-	}
+	if (tty->ldisc.flush_buffer)
+		tty->ldisc.flush_buffer(tty);
+	if (tty->driver->flush_buffer)
+		tty->driver->flush_buffer(tty);
+	if ((test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) &&
+	    tty->ldisc.write_wakeup)
+		(tty->ldisc.write_wakeup)(tty);
 
 	wake_up_interruptible(&tty->write_wait);
 	wake_up_interruptible(&tty->read_wait);

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Jurjen Oskam]

On Fri, May 28, 2004 at 01:42:50PM -0500, Paul Fulghum wrote:

> Up to now, this did not cause any actual problems.
> In 2.6.X this causes a warning if any of the called functions
> (flush_buffer/write_wakeup) call spin_xxx_bh() functions.

It was not only a warning here, but the pptp client also didn't work again
until after rebooting. Every time the connection was retried (every 10
seconds or so) the message appeared. Perhaps this is a problem in the pptp
client, but after rebooting it worked.

As for the patch: applied to vanilla 2.6.5 (applied cleanly, offset -5
lines) and compiling now.

One problem though: I'm running 2.6 for about two weeks now (with a 24x7
pptp connection), and I encountered this problem just once. I've seen a
report that someone got this error when his connection was severed, so
I'll try to pull the phone cable a few times and see how it recovers, both
with and without your patch. I'll report back when I know more.

Thanks,
-- 
Jurjen Oskam

"Avoid putting a paging file on a fault-tolerant drive, such as a mirrored
volume or a RAID-5 volume. Paging files do not need fault-tolerance."-MS Q308417

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Paul Fulghum]

On Fri, 2004-05-28 at 15:11, Jurjen Oskam wrote:

> It was not only a warning here, but the pptp client also didn't work again
> until after rebooting. Every time the connection was retried (every 10
> seconds or so) the message appeared. Perhaps this is a problem in the pptp
> client, but after rebooting it worked.

That sounds like a different problem.
The warning itself is harmless.

I see another patch (not from me) related to closing a tty device
(also in tty_io.c) that has gone into 2.6.7-rc1
that may be related. I include that patch below


> As for the patch: applied to vanilla 2.6.5 (applied cleanly, offset -5
> lines) and compiling now.
> 
> One problem though: I'm running 2.6 for about two weeks now (with a 24x7
> pptp connection), and I encountered this problem just once. I've seen a
> report that someone got this error when his connection was severed, so
> I'll try to pull the phone cable a few times and see how it recovers, both
> with and without your patch. I'll report back when I know more.


In order to see the warning, the synctty line discipline must
be waiting to push more send data to the tty device when disconnecting.
If the line is idle, then the warning will not appear on hangup.

You can try and provoke the warning in the unpatched kernel
by causing a hangup while sending lots of data.

--
Paul Fulghum
paulkf@microgate.com


diff -Nru a/drivers/char/tty_io.c b/drivers/char/tty_io.c
--- a/drivers/char/tty_io.c	Sat May 22 22:55:13 2004
+++ b/drivers/char/tty_io.c	Sat May 22 22:55:13 2004
@@ -1267,6 +1267,18 @@
 #endif
 
 	/*
+	 * Prevent flush_to_ldisc() from rescheduling the work for later.  Then
+	 * kill any delayed work.
+	 */
+	clear_bit(TTY_DONT_FLIP, &tty->flags);
+	cancel_delayed_work(&tty->flip.work);
+
+	/*
+	 * Wait for ->hangup_work and ->flip.work handlers to terminate
+	 */
+	flush_scheduled_work();
+
+	/*
 	 * Shutdown the current line discipline, and reset it to N_TTY.
 	 * N.B. why reset ldisc when we're releasing the memory??
 	 */
@@ -1282,18 +1294,6 @@
 		module_put(o_tty->ldisc.owner);
 		o_tty->ldisc = ldiscs[N_TTY];
 	}
-	
-	/*
-	 * Prevent flush_to_ldisc() from rescheduling the work for later.  Then
-	 * kill any delayed work.
-	 */
-	clear_bit(TTY_DONT_FLIP, &tty->flags);
-	cancel_delayed_work(&tty->flip.work);
-
-	/*
-	 * Wait for ->hangup_work and ->flip.work handlers to terminate
-	 */
-	flush_scheduled_work();
 
 	/* 
 	 * The release_mem function takes care of the details of clearing

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Andrew Morton]

Paul Fulghum <paulkf@microgate.com> wrote:
>
> The following patch removes unnecessary disabling of
> interrupts when processing hangup for tty devices.

Ho hum, this has been hanging around forever.  Obviously the current
locking is pointless and the only useful locking we have in there is
lock_kernel().

The reason why a patch such as yours wasn't applied is that it was all kept
as a reminder that we suck.  Someone needs to get down and audit what's
actually happening in there.  It seems that you've now done that via
comparison with other callers, but that is not necessarily a good approach
when it comes to the tty layer ;)

We need to itemise all the affected memory storage in all impementations of
->flush_buffer() and ->write_wakeup() and then make sure that all _other_
users of those fields (whether or not they lie in the ->flush_buffer() and
->write_wakeup() codepaths) are using the same locking.  Is that something
you could do?

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Paul Fulghum]

On Fri, 2004-05-28 at 18:06, Andrew Morton wrote:
> Obviously the current locking is pointless...

Agreed

> ... and the only useful locking we have in there is lock_kernel().

For calling write_wakeup, even that is useless.

After more examination, I see write_wakeup can
be called in any context (even hard interrupt) with no
locking (external to write_wakeup) at all.

write_wakeup *must* provide all required locking internally,
because none is provided externally (in a consistent way
by all callers). write_wakeup implementations requiring
external locking are broken.
Planned or not, this is the current situation.

I'm still poking around the flush_buffer calls
but the same appears to apply here as well.


> The reason why a patch such as yours wasn't applied is that it was all kept
> as a reminder that we suck.  Someone needs to get down and audit what's
> actually happening in there.  It seems that you've now done that via
> comparison with other callers, but that is not necessarily a good approach
> when it comes to the tty layer ;)

My analysis of callers of these functions was to show that
disabling interrupts to make these calls in do_tty_hangup()
is useless, because not all callers follow this convention.
It is not an indication of the correctness
of individual tty drivers or line disciplines.

Removing the interrupt disabling won't make
individual tty drivers or line disciplines any more or
less broken. (but may expose existing problems more readily)


> We need to itemise all the affected memory storage in all impementations of
> ->flush_buffer() and ->write_wakeup() and then make sure that all _other_
> users of those fields (whether or not they lie in the ->flush_buffer() and
> ->write_wakeup() codepaths) are using the same locking.  Is that something
> you could do?

I see what you are getting at, but what question are we
trying to answer? We already know flush_buffer/write_wakeup
implementations requiring external locking
are broken as no such locking currently exists, and
interrupt disabling in do_tty_hangup does not fix this.
If external locking is not required, then interrupt
disabling in do_tty_hangup serves no purpose.

Apart from that question, auditing the correctness of internal
details of the 17 line disciplines and 64 tty drivers would be
nice but would take a very long time for one person :-)
It is probably better that:
* Broken, unmaintained drivers remain broken and are eventually culled.
* Broken, maintained drivers are identified and fixed by the maintainer.

I understand that without such an audit you may want
to let interrupt disabling in do_tty_hangup() stand in
case it is helping broken drivers or line disciplines
to limp along in some cases.


As for the softirq.c warning, this is not a problem.
Currently ppp_synctty does not allow for the fact that
write_wakeup can be called in hard interrupt context.
A patch from Paul Mackerras has already been accepted
to correct the same problem for ppp_async.c and I will
adapt his modifications to ppp_synctty.c
These modifications have the side effect of making
the interrupt enable/disable state of the write_wakeup
caller irrelevant. 

--
Paul Fulghum
paulkf@microgate.com

[PATCH] 2.6.6 synclinkmp.c

[Paul Fulghum]

Patch to drivers/char/synclinkmp.c against 2.6.6
to cleanup properly on errors during
driver initialization.

In particular, call pci_unregister_driver if
driver init fails. This is in response to a bug
report by Dave Jones.

Please apply.
--
Paul Fulghum
paulkf@microgate.com


--- linux-2.6.6/drivers/char/synclinkmp.c	2004-06-01 15:30:02.967249331 -0500
+++ linux-2.6.6-mg1/drivers/char/synclinkmp.c	2004-06-01 15:28:31.022403976 -0500
@@ -1,5 +1,5 @@
 /*
- * $Id: synclinkmp.c,v 4.19 2004/03/08 15:29:23 paulkf Exp $
+ * $Id: synclinkmp.c,v 4.20 2004/06/01 20:27:46 paulkf Exp $
  *
  * Device driver for Microgate SyncLink Multiport
  * high speed multiprotocol serial adapter.
@@ -494,7 +494,7 @@
 MODULE_PARM(dosyncppp,"1-" __MODULE_STRING(MAX_DEVICES) "i");
 
 static char *driver_name = "SyncLink MultiPort driver";
-static char *driver_version = "$Revision: 4.19 $";
+static char *driver_version = "$Revision: 4.20 $";
 
 static int synclinkmp_init_one(struct pci_dev *dev,const struct pci_device_id *ent);
 static void synclinkmp_remove_one(struct pci_dev *dev);
@@ -3781,56 +3781,7 @@
 	.tiocmset = tiocmset,
 };
 
-/* Driver initialization entry point.
- */
-
-static int __init synclinkmp_init(void)
-{
-	if (break_on_load) {
-	 	synclinkmp_get_text_ptr();
-  		BREAKPOINT();
-	}
-
- 	printk("%s %s\n", driver_name, driver_version);
-
-	synclinkmp_adapter_count = -1;
-	pci_register_driver(&synclinkmp_pci_driver);
-
-	if ( !synclinkmp_device_list ) {
-		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
-		return -ENODEV;
-	}
-
-	serial_driver = alloc_tty_driver(synclinkmp_device_count);
-	if (!serial_driver)
-		return -ENOMEM;
-
-	/* Initialize the tty_driver structure */
-
-	serial_driver->owner = THIS_MODULE;
-	serial_driver->driver_name = "synclinkmp";
-	serial_driver->name = "ttySLM";
-	serial_driver->major = ttymajor;
-	serial_driver->minor_start = 64;
-	serial_driver->type = TTY_DRIVER_TYPE_SERIAL;
-	serial_driver->subtype = SERIAL_TYPE_NORMAL;
-	serial_driver->init_termios = tty_std_termios;
-	serial_driver->init_termios.c_cflag =
-		B9600 | CS8 | CREAD | HUPCL | CLOCAL;
-	serial_driver->flags = TTY_DRIVER_REAL_RAW;
-	tty_set_operations(serial_driver, &ops);
-	if (tty_register_driver(serial_driver) < 0)
-		printk("%s(%d):Couldn't register serial driver\n",
-			__FILE__,__LINE__);
-
- 	printk("%s %s, tty major#%d\n",
-		driver_name, driver_version,
-		serial_driver->major);
-
-	return 0;
-}
-
-static void __exit synclinkmp_exit(void)
+static void synclinkmp_cleanup(void)
 {
 	unsigned long flags;
 	int rc;
@@ -3839,10 +3790,12 @@
 
 	printk("Unloading %s %s\n", driver_name, driver_version);
 
-	if ((rc = tty_unregister_driver(serial_driver)))
-		printk("%s(%d) failed to unregister tty driver err=%d\n",
-		       __FILE__,__LINE__,rc);
-	put_tty_driver(serial_driver);
+	if (serial_driver) {
+		if ((rc = tty_unregister_driver(serial_driver)))
+			printk("%s(%d) failed to unregister tty driver err=%d\n",
+			       __FILE__,__LINE__,rc);
+		put_tty_driver(serial_driver);
+	}
 
 	info = synclinkmp_device_list;
 	while(info) {
@@ -3882,6 +3835,73 @@
 	pci_unregister_driver(&synclinkmp_pci_driver);
 }
 
+/* Driver initialization entry point.
+ */
+
+static int __init synclinkmp_init(void)
+{
+	int rc;
+
+	if (break_on_load) {
+	 	synclinkmp_get_text_ptr();
+  		BREAKPOINT();
+	}
+
+ 	printk("%s %s\n", driver_name, driver_version);
+
+	synclinkmp_adapter_count = -1;
+	rc = pci_register_driver(&synclinkmp_pci_driver);
+
+	if (rc <= 0 || !synclinkmp_device_list) {
+		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
+		rc = -ENODEV;
+		goto error;
+	}
+
+	serial_driver = alloc_tty_driver(synclinkmp_device_count);
+	if (!serial_driver) {
+		rc = -ENOMEM;
+		goto error;
+	}
+
+	/* Initialize the tty_driver structure */
+
+	serial_driver->owner = THIS_MODULE;
+	serial_driver->driver_name = "synclinkmp";
+	serial_driver->name = "ttySLM";
+	serial_driver->major = ttymajor;
+	serial_driver->minor_start = 64;
+	serial_driver->type = TTY_DRIVER_TYPE_SERIAL;
+	serial_driver->subtype = SERIAL_TYPE_NORMAL;
+	serial_driver->init_termios = tty_std_termios;
+	serial_driver->init_termios.c_cflag =
+		B9600 | CS8 | CREAD | HUPCL | CLOCAL;
+	serial_driver->flags = TTY_DRIVER_REAL_RAW;
+	tty_set_operations(serial_driver, &ops);
+	if ((rc = tty_register_driver(serial_driver)) < 0) {
+		printk("%s(%d):Couldn't register serial driver\n",
+			__FILE__,__LINE__);
+		put_tty_driver(serial_driver);
+		serial_driver = NULL;
+		goto error;
+	}
+
+ 	printk("%s %s, tty major#%d\n",
+		driver_name, driver_version,
+		serial_driver->major);
+
+	return 0;
+
+error:
+	synclinkmp_cleanup();
+	return rc;
+}
+
+static void __exit synclinkmp_exit(void)
+{
+	synclinkmp_cleanup();
+}
+
 module_init(synclinkmp_init);
 module_exit(synclinkmp_exit);
 

Re: [PATCH] 2.6.6 synclinkmp.c

[Russell King]

On Tue, Jun 01, 2004 at 03:51:02PM -0500, Paul Fulghum wrote:
> In particular, call pci_unregister_driver if driver init fails. This
> is in response to a bug report by Dave Jones.

If pci_register_driver fails, the driver is not, repeat not left
registered.  Therefore it must not be unregistered after failure
to register.

-- 
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] 2.6.6 synclinkmp.c

[Paul Fulghum]

On Tue, 2004-06-01 at 15:57, Russell King wrote:
> If pci_register_driver fails, the driver is not, repeat not left
> registered.  Therefore it must not be unregistered after failure
> to register.

You are right. The specific problem I was trying to
fix is when no hardware is detected. I looked at other
PCI drivers (char/epca.c and net/eepro100.c) and which call
pci_unregister_driver if pci_register_driver returns <= 0
and indicates that pci_register_device returns the number
of pci devices detected. I now see that the two drivers I
looked at are broken. (bad luck that)

After looking at the source for pci_register_device(),
if no devices are detected, then it still returns 1.

I will rework the patches against synclink.c/synclinkmp.c
to only call pci_unregister_device() if init fails
(such as when no devices are detected)
*and* the call to pci_register_device() succeeds.

--
Paul Fulghum
paulkf@microgate.com

Re: [PATCH] 2.6.6 synclinkmp.c

[Russell King]

On Tue, Jun 01, 2004 at 04:25:30PM -0500, Paul Fulghum wrote:
> On Tue, 2004-06-01 at 15:57, Russell King wrote:
> > If pci_register_driver fails, the driver is not, repeat not left
> > registered.  Therefore it must not be unregistered after failure
> > to register.
> 
> You are right. The specific problem I was trying to
> fix is when no hardware is detected. I looked at other
> PCI drivers (char/epca.c and net/eepro100.c) and which call
> pci_unregister_driver if pci_register_driver returns <= 0
> and indicates that pci_register_device returns the number
> of pci devices detected. I now see that the two drivers I
> looked at are broken. (bad luck that)
> 
> After looking at the source for pci_register_device(),
> if no devices are detected, then it still returns 1.
> 
> I will rework the patches against synclink.c/synclinkmp.c
> to only call pci_unregister_device() if init fails
> (such as when no devices are detected)
> *and* the call to pci_register_device() succeeds.

Don't arrange for the driver to unload if it doesn't detect anything.
2.6 has dynid support so that the user can load your driver and assign
it extra PCI vendor/device IDs at run time - which won't work if you've
forced failure when nothing is found.

-- 
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] 2.6.6 synclinkmp.c

[Paul Fulghum]

Russell King wrote:

>Don't arrange for the driver to unload if it doesn't detect anything.
>2.6 has dynid support so that the user can load your driver and assign
>it extra PCI vendor/device IDs at run time - which won't work if you've
>forced failure when nothing is found.
>

That sounds reasonable.

I'll add that change and resubmit the patches for synclink.c and 
synclinkmp.c

--
Paul Fulghum
paulkf@microgate.com

Re: [PATCH] 2.6.6 synclinkmp.c

[Paul Fulghum]

On Tue, 2004-06-01 at 15:57, Russell King wrote:
> If pci_register_driver fails, the driver is not, repeat not left
> registered.  Therefore it must not be unregistered after failure
> to register.

OK, here is a corrected patch that properly distinguishes
between pci_register_driver failure and the case
of finding no hardware.


--
Paul Fulghum
paulkf@microgate.com


--- linux-2.6.6/drivers/char/synclinkmp.c	2004-06-02 09:07:40.495553141 -0500
+++ linux-2.6.6-mg1/drivers/char/synclinkmp.c	2004-06-02 09:08:05.720218567 -0500
@@ -1,5 +1,5 @@
 /*
- * $Id: synclinkmp.c,v 4.19 2004/03/08 15:29:23 paulkf Exp $
+ * $Id: synclinkmp.c,v 4.21 2004/06/02 14:07:14 paulkf Exp $
  *
  * Device driver for Microgate SyncLink Multiport
  * high speed multiprotocol serial adapter.
@@ -494,7 +494,7 @@
 MODULE_PARM(dosyncppp,"1-" __MODULE_STRING(MAX_DEVICES) "i");
 
 static char *driver_name = "SyncLink MultiPort driver";
-static char *driver_version = "$Revision: 4.19 $";
+static char *driver_version = "$Revision: 4.21 $";
 
 static int synclinkmp_init_one(struct pci_dev *dev,const struct pci_device_id *ent);
 static void synclinkmp_remove_one(struct pci_dev *dev);
@@ -3781,56 +3781,7 @@
 	.tiocmset = tiocmset,
 };
 
-/* Driver initialization entry point.
- */
-
-static int __init synclinkmp_init(void)
-{
-	if (break_on_load) {
-	 	synclinkmp_get_text_ptr();
-  		BREAKPOINT();
-	}
-
- 	printk("%s %s\n", driver_name, driver_version);
-
-	synclinkmp_adapter_count = -1;
-	pci_register_driver(&synclinkmp_pci_driver);
-
-	if ( !synclinkmp_device_list ) {
-		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
-		return -ENODEV;
-	}
-
-	serial_driver = alloc_tty_driver(synclinkmp_device_count);
-	if (!serial_driver)
-		return -ENOMEM;
-
-	/* Initialize the tty_driver structure */
-
-	serial_driver->owner = THIS_MODULE;
-	serial_driver->driver_name = "synclinkmp";
-	serial_driver->name = "ttySLM";
-	serial_driver->major = ttymajor;
-	serial_driver->minor_start = 64;
-	serial_driver->type = TTY_DRIVER_TYPE_SERIAL;
-	serial_driver->subtype = SERIAL_TYPE_NORMAL;
-	serial_driver->init_termios = tty_std_termios;
-	serial_driver->init_termios.c_cflag =
-		B9600 | CS8 | CREAD | HUPCL | CLOCAL;
-	serial_driver->flags = TTY_DRIVER_REAL_RAW;
-	tty_set_operations(serial_driver, &ops);
-	if (tty_register_driver(serial_driver) < 0)
-		printk("%s(%d):Couldn't register serial driver\n",
-			__FILE__,__LINE__);
-
- 	printk("%s %s, tty major#%d\n",
-		driver_name, driver_version,
-		serial_driver->major);
-
-	return 0;
-}
-
-static void __exit synclinkmp_exit(void)
+static void synclinkmp_cleanup(void)
 {
 	unsigned long flags;
 	int rc;
@@ -3839,10 +3790,12 @@
 
 	printk("Unloading %s %s\n", driver_name, driver_version);
 
-	if ((rc = tty_unregister_driver(serial_driver)))
-		printk("%s(%d) failed to unregister tty driver err=%d\n",
-		       __FILE__,__LINE__,rc);
-	put_tty_driver(serial_driver);
+	if (serial_driver) {
+		if ((rc = tty_unregister_driver(serial_driver)))
+			printk("%s(%d) failed to unregister tty driver err=%d\n",
+			       __FILE__,__LINE__,rc);
+		put_tty_driver(serial_driver);
+	}
 
 	info = synclinkmp_device_list;
 	while(info) {
@@ -3882,6 +3835,75 @@
 	pci_unregister_driver(&synclinkmp_pci_driver);
 }
 
+/* Driver initialization entry point.
+ */
+
+static int __init synclinkmp_init(void)
+{
+	int rc;
+
+	if (break_on_load) {
+	 	synclinkmp_get_text_ptr();
+  		BREAKPOINT();
+	}
+
+ 	printk("%s %s\n", driver_name, driver_version);
+
+	if ((rc = pci_register_driver(&synclinkmp_pci_driver)) < 0) {
+		printk("%s:failed to register PCI driver, error=%d\n",__FILE__,rc);
+		return rc;
+	}
+
+	if (!synclinkmp_device_list) {
+		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
+		rc = -ENODEV;
+		goto error;
+	}
+
+	serial_driver = alloc_tty_driver(synclinkmp_device_count);
+	if (!serial_driver) {
+		rc = -ENOMEM;
+		goto error;
+	}
+
+	/* Initialize the tty_driver structure */
+
+	serial_driver->owner = THIS_MODULE;
+	serial_driver->driver_name = "synclinkmp";
+	serial_driver->name = "ttySLM";
+	serial_driver->major = ttymajor;
+	serial_driver->minor_start = 64;
+	serial_driver->type = TTY_DRIVER_TYPE_SERIAL;
+	serial_driver->subtype = SERIAL_TYPE_NORMAL;
+	serial_driver->init_termios = tty_std_termios;
+	serial_driver->init_termios.c_cflag =
+		B9600 | CS8 | CREAD | HUPCL | CLOCAL;
+	serial_driver->flags = TTY_DRIVER_REAL_RAW;
+	tty_set_operations(serial_driver, &ops);
+	if ((rc = tty_register_driver(serial_driver)) < 0) {
+		printk("%s(%d):Couldn't register serial driver\n",
+			__FILE__,__LINE__);
+		put_tty_driver(serial_driver);
+		serial_driver = NULL;
+		goto error;
+	}
+
+ 	printk("%s %s, tty major#%d\n",
+		driver_name, driver_version,
+		serial_driver->major);
+
+	return 0;
+
+error:
+	synclinkmp_cleanup();
+	return rc;
+}
+
+static void __exit synclinkmp_exit(void)
+{
+	synclinkmp_cleanup();
+}
+
 module_init(synclinkmp_init);
 module_exit(synclinkmp_exit);
 

[PATCH] 2.6.6 synclink.c

[Paul Fulghum]

Patch to drivers/char/synclink.c against 2.6.6
to cleanup properly on errors during
driver initialization.

In particular, call pci_unregister_driver if
driver init fails. This is in response to a bug
report by Dave Jones.

Please apply.

--
Paul Fulghum
paulkf@microgate.com



--- linux-2.6.6/drivers/char/synclink.c	2004-06-01 15:30:02.903257790 -0500
+++ linux-2.6.6-mg1/drivers/char/synclink.c	2004-06-01 15:28:24.166310318 -0500
@@ -1,7 +1,7 @@
 /*
  * linux/drivers/char/synclink.c
  *
- * $Id: synclink.c,v 4.21 2004/03/08 15:29:22 paulkf Exp $
+ * $Id: synclink.c,v 4.22 2004/06/01 20:27:46 paulkf Exp $
  *
  * Device driver for Microgate SyncLink ISA and PCI
  * high speed multiprotocol serial adapters.
@@ -782,7 +782,6 @@
 void mgsl_release_resources(struct mgsl_struct *info);
 void mgsl_add_device(struct mgsl_struct *info);
 struct mgsl_struct* mgsl_allocate_device(void);
-int mgsl_enum_isa_devices(void);
 
 /*
  * DMA buffer manupulation functions.
@@ -909,7 +908,7 @@
 MODULE_PARM(txholdbufs,"1-" __MODULE_STRING(MAX_TOTAL_DEVICES) "i");
 
 static char *driver_name = "SyncLink serial driver";
-static char *driver_version = "$Revision: 4.21 $";
+static char *driver_version = "$Revision: 4.22 $";
 
 static int synclink_init_one (struct pci_dev *dev,
 				     const struct pci_device_id *ent);
@@ -4484,9 +4483,10 @@
 /*
  * perform tty device initialization
  */
-int mgsl_init_tty(void);
-int mgsl_init_tty()
+static int mgsl_init_tty(void)
 {
+	int rc;
+
 	serial_driver = alloc_tty_driver(mgsl_device_count);
 	if (!serial_driver)
 		return -ENOMEM;
@@ -4503,9 +4503,13 @@
 		B9600 | CS8 | CREAD | HUPCL | CLOCAL;
 	serial_driver->flags = TTY_DRIVER_REAL_RAW;
 	tty_set_operations(serial_driver, &mgsl_ops);
-	if (tty_register_driver(serial_driver) < 0)
+	if ((rc = tty_register_driver(serial_driver)) < 0) {
 		printk("%s(%d):Couldn't register serial driver\n",
 			__FILE__,__LINE__);
+		put_tty_driver(serial_driver);
+		serial_driver = NULL;
+		return rc;
+	}
 			
  	printk("%s %s, tty major#%d\n",
 		driver_name, driver_version,
@@ -4515,7 +4519,7 @@
 
 /* enumerate user specified ISA adapters
  */
-int mgsl_enum_isa_devices()
+static void mgsl_enum_isa_devices(void)
 {
 	struct mgsl_struct *info;
 	int i;
@@ -4546,51 +4550,9 @@
 		
 		mgsl_add_device( info );
 	}
-	
-	return 0;
-}
-
-/* mgsl_init()
- * 
- * 	Driver initialization entry point.
- * 	
- * Arguments:	None
- * Return Value:	0 if success, otherwise error code
- */
-int __init mgsl_init(void)
-{
-	int rc;
-
- 	printk("%s %s\n", driver_name, driver_version);
-	
-	mgsl_enum_isa_devices();
-	pci_register_driver(&synclink_pci_driver);
-
-	if ( !mgsl_device_list ) {
-		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
-		return -ENODEV;
-	}
-	if ((rc = mgsl_init_tty()))
-		return rc;
-	
-	return 0;
-}
-
-static int __init synclink_init(void)
-{
-/* Uncomment this to kernel debug module.
- * mgsl_get_text_ptr() leaves the .text address in eax
- * which can be used with add-symbol-file with gdb.
- */
-	if (break_on_load) {
-	 	mgsl_get_text_ptr();
-  		BREAKPOINT();
-	}
-	
-	return mgsl_init();
 }
 
-static void __exit synclink_exit(void) 
+static void synclink_cleanup(void) 
 {
 	int rc;
 	struct mgsl_struct *info;
@@ -4598,11 +4560,13 @@
 
 	printk("Unloading %s: %s\n", driver_name, driver_version);
 
-	if ((rc = tty_unregister_driver(serial_driver)))
-		printk("%s(%d) failed to unregister tty driver err=%d\n",
-		       __FILE__,__LINE__,rc);
+	if (serial_driver) {
+		if ((rc = tty_unregister_driver(serial_driver)))
+			printk("%s(%d) failed to unregister tty driver err=%d\n",
+			       __FILE__,__LINE__,rc);
+		put_tty_driver(serial_driver);
+	}
 
-	put_tty_driver(serial_driver);
 	info = mgsl_device_list;
 	while(info) {
 #ifdef CONFIG_SYNCLINK_SYNCPPP
@@ -4623,6 +4587,40 @@
 	pci_unregister_driver(&synclink_pci_driver);
 }
 
+static int __init synclink_init(void)
+{
+	int rc;
+
+	if (break_on_load) {
+	 	mgsl_get_text_ptr();
+  		BREAKPOINT();
+	}
+
+ 	printk("%s %s\n", driver_name, driver_version);
+	
+	mgsl_enum_isa_devices();
+	pci_register_driver(&synclink_pci_driver);
+
+	if ( !mgsl_device_list ) {
+		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
+		rc = -ENODEV;
+		goto error;
+	}
+	if ((rc = mgsl_init_tty()) < 0)
+		goto error;
+	
+	return 0;
+
+error:
+	synclink_cleanup();
+	return rc;
+}
+
+static void __exit synclink_exit(void) 
+{
+	synclink_cleanup();
+}
+
 module_init(synclink_init);
 module_exit(synclink_exit);
 

Re: [PATCH] 2.6.6 synclink.c

[Paul Fulghum]

Here is a corrected patch that properly distinguishes
between pci_register_driver failure and the case of
finding no hardware.


--
Paul Fulghum
paulkf@microgate.com


--- linux-2.6.6/drivers/char/synclink.c	2004-06-02 09:07:40.452558825 -0500
+++ linux-2.6.6-mg1/drivers/char/synclink.c	2004-06-02 09:07:59.780003834 -0500
@@ -1,7 +1,7 @@
 /*
  * linux/drivers/char/synclink.c
  *
- * $Id: synclink.c,v 4.21 2004/03/08 15:29:22 paulkf Exp $
+ * $Id: synclink.c,v 4.23 2004/06/02 14:07:14 paulkf Exp $
  *
  * Device driver for Microgate SyncLink ISA and PCI
  * high speed multiprotocol serial adapters.
@@ -782,7 +782,6 @@
 void mgsl_release_resources(struct mgsl_struct *info);
 void mgsl_add_device(struct mgsl_struct *info);
 struct mgsl_struct* mgsl_allocate_device(void);
-int mgsl_enum_isa_devices(void);
 
 /*
  * DMA buffer manupulation functions.
@@ -866,6 +865,9 @@
 
 #define jiffies_from_ms(a) ((((a) * HZ)/1000)+1)
 
+/* set non-zero on successful registration with PCI subsystem */
+static int pci_registered;
+
 /*
  * Global linked list of SyncLink devices
  */
@@ -909,7 +911,7 @@
 MODULE_PARM(txholdbufs,"1-" __MODULE_STRING(MAX_TOTAL_DEVICES) "i");
 
 static char *driver_name = "SyncLink serial driver";
-static char *driver_version = "$Revision: 4.21 $";
+static char *driver_version = "$Revision: 4.23 $";
 
 static int synclink_init_one (struct pci_dev *dev,
 				     const struct pci_device_id *ent);
@@ -4484,9 +4486,10 @@
 /*
  * perform tty device initialization
  */
-int mgsl_init_tty(void);
-int mgsl_init_tty()
+static int mgsl_init_tty(void)
 {
+	int rc;
+
 	serial_driver = alloc_tty_driver(mgsl_device_count);
 	if (!serial_driver)
 		return -ENOMEM;
@@ -4503,9 +4506,13 @@
 		B9600 | CS8 | CREAD | HUPCL | CLOCAL;
 	serial_driver->flags = TTY_DRIVER_REAL_RAW;
 	tty_set_operations(serial_driver, &mgsl_ops);
-	if (tty_register_driver(serial_driver) < 0)
+	if ((rc = tty_register_driver(serial_driver)) < 0) {
 		printk("%s(%d):Couldn't register serial driver\n",
 			__FILE__,__LINE__);
+		put_tty_driver(serial_driver);
+		serial_driver = NULL;
+		return rc;
+	}
 			
  	printk("%s %s, tty major#%d\n",
 		driver_name, driver_version,
@@ -4515,7 +4522,7 @@
 
 /* enumerate user specified ISA adapters
  */
-int mgsl_enum_isa_devices()
+static void mgsl_enum_isa_devices(void)
 {
 	struct mgsl_struct *info;
 	int i;
@@ -4546,51 +4553,9 @@
 		
 		mgsl_add_device( info );
 	}
-	
-	return 0;
-}
-
-/* mgsl_init()
- * 
- * 	Driver initialization entry point.
- * 	
- * Arguments:	None
- * Return Value:	0 if success, otherwise error code
- */
-int __init mgsl_init(void)
-{
-	int rc;
-
- 	printk("%s %s\n", driver_name, driver_version);
-	
-	mgsl_enum_isa_devices();
-	pci_register_driver(&synclink_pci_driver);
-
-	if ( !mgsl_device_list ) {
-		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
-		return -ENODEV;
-	}
-	if ((rc = mgsl_init_tty()))
-		return rc;
-	
-	return 0;
-}
-
-static int __init synclink_init(void)
-{
-/* Uncomment this to kernel debug module.
- * mgsl_get_text_ptr() leaves the .text address in eax
- * which can be used with add-symbol-file with gdb.
- */
-	if (break_on_load) {
-	 	mgsl_get_text_ptr();
-  		BREAKPOINT();
-	}
-	
-	return mgsl_init();
 }
 
-static void __exit synclink_exit(void) 
+static void synclink_cleanup(void) 
 {
 	int rc;
 	struct mgsl_struct *info;
@@ -4598,11 +4563,13 @@
 
 	printk("Unloading %s: %s\n", driver_name, driver_version);
 
-	if ((rc = tty_unregister_driver(serial_driver)))
-		printk("%s(%d) failed to unregister tty driver err=%d\n",
-		       __FILE__,__LINE__,rc);
+	if (serial_driver) {
+		if ((rc = tty_unregister_driver(serial_driver)))
+			printk("%s(%d) failed to unregister tty driver err=%d\n",
+			       __FILE__,__LINE__,rc);
+		put_tty_driver(serial_driver);
+	}
 
-	put_tty_driver(serial_driver);
 	info = mgsl_device_list;
 	while(info) {
 #ifdef CONFIG_SYNCLINK_SYNCPPP
@@ -4620,7 +4587,45 @@
 		tmp_buf = NULL;
 	}
 	
-	pci_unregister_driver(&synclink_pci_driver);
+	if (pci_registered)
+		pci_unregister_driver(&synclink_pci_driver);
+}
+
+static int __init synclink_init(void)
+{
+	int rc;
+
+	if (break_on_load) {
+	 	mgsl_get_text_ptr();
+  		BREAKPOINT();
+	}
+
+ 	printk("%s %s\n", driver_name, driver_version);
+	
+	mgsl_enum_isa_devices();
+	if ((rc = pci_register_driver(&synclink_pci_driver)) < 0)
+		printk("%s:failed to register PCI driver, error=%d\n",__FILE__,rc);
+	else
+		pci_registered = 1;
+
+	if ( !mgsl_device_list ) {
+		printk("%s(%d):No SyncLink devices found.\n",__FILE__,__LINE__);
+		rc = -ENODEV;
+		goto error;
+	}
+	if ((rc = mgsl_init_tty()) < 0)
+		goto error;
+	
+	return 0;
+
+error:
+	synclink_cleanup();
+	return rc;
+}
+
+static void __exit synclink_exit(void) 
+{
+	synclink_cleanup();
 }
 
 module_init(synclink_init);

[PATCH] 2.6.6 synclink_cs.c

[Paul Fulghum]

Patch to drivers/char/pcmcia/synclink_cs.c against 2.6.6
to cleanup properly on errors during
driver initialization.

Please apply.

--
Paul Fulghum
paulkf@microgate.com


--- linux-2.6.6/drivers/char/pcmcia/synclink_cs.c	2004-06-01 15:30:02.945252239 -0500
+++ linux-2.6.6-mg1/drivers/char/pcmcia/synclink_cs.c	2004-06-01 15:28:41.033080615 -0500
@@ -1,7 +1,7 @@
 /*
  * linux/drivers/char/pcmcia/synclink_cs.c
  *
- * $Id: synclink_cs.c,v 4.21 2004/03/08 15:29:23 paulkf Exp $
+ * $Id: synclink_cs.c,v 4.22 2004/06/01 20:27:46 paulkf Exp $
  *
  * Device driver for Microgate SyncLink PC Card
  * multiprotocol serial adapter.
@@ -489,7 +489,7 @@
 MODULE_LICENSE("GPL");
 
 static char *driver_name = "SyncLink PC Card driver";
-static char *driver_version = "$Revision: 4.21 $";
+static char *driver_version = "$Revision: 4.22 $";
 
 static struct tty_driver *serial_driver;
 
@@ -3130,9 +3130,35 @@
 	.tiocmset = tiocmset,
 };
 
+static void synclink_cs_cleanup(void) 
+{
+	int rc;
+
+	printk("Unloading %s: version %s\n", driver_name, driver_version);
+
+	while(mgslpc_device_list)
+		mgslpc_remove_device(mgslpc_device_list);
+
+	if (serial_driver) {
+		if ((rc = tty_unregister_driver(serial_driver)))
+			printk("%s(%d) failed to unregister tty driver err=%d\n",
+			       __FILE__,__LINE__,rc);
+		put_tty_driver(serial_driver);
+	}
+
+	pcmcia_unregister_driver(&mgslpc_driver);
+
+	/* XXX: this really needs to move into generic code.. */
+	while (dev_list != NULL) {
+		if (dev_list->state & DEV_CONFIG)
+			mgslpc_release((u_long)dev_list);
+		mgslpc_detach(dev_list);
+	}
+}
+
 static int __init synclink_cs_init(void)
 {
-    int error;
+    int rc;
 
     if (break_on_load) {
 	    mgslpc_get_text_ptr();
@@ -3141,14 +3167,13 @@
 
     printk("%s %s\n", driver_name, driver_version);
 
-    serial_driver = alloc_tty_driver(MAX_DEVICE_COUNT);
-    if (!serial_driver)
-	    return -ENOMEM;
+    if ((rc = pcmcia_register_driver(&mgslpc_driver)) < 0)
+	    return rc;
 
-    error = pcmcia_register_driver(&mgslpc_driver);
-    if (error) {
-	    put_tty_driver(serial_driver);
-	    return error;
+    serial_driver = alloc_tty_driver(MAX_DEVICE_COUNT);
+    if (!serial_driver) {
+	    rc = -ENOMEM;
+	    goto error;
     }
 
     /* Initialize the tty_driver structure */
@@ -3166,39 +3191,28 @@
     serial_driver->flags = TTY_DRIVER_REAL_RAW;
     tty_set_operations(serial_driver, &mgslpc_ops);
 
-    if (tty_register_driver(serial_driver) < 0)
+    if ((rc = tty_register_driver(serial_driver)) < 0) {
 	    printk("%s(%d):Couldn't register serial driver\n",
 		   __FILE__,__LINE__);
+	    put_tty_driver(serial_driver);
+	    serial_driver = NULL;
+	    goto error;
+    }
 			
     printk("%s %s, tty major#%d\n",
 	   driver_name, driver_version,
 	   serial_driver->major);
 	
     return 0;
+
+error:
+    synclink_cs_cleanup();
+    return rc;
 }
 
 static void __exit synclink_cs_exit(void) 
 {
-	int rc;
-
-	printk("Unloading %s: version %s\n", driver_name, driver_version);
-
-	while(mgslpc_device_list)
-		mgslpc_remove_device(mgslpc_device_list);
-
-	if ((rc = tty_unregister_driver(serial_driver)))
-		printk("%s(%d) failed to unregister tty driver err=%d\n",
-		       __FILE__,__LINE__,rc);
-	put_tty_driver(serial_driver);
-
-	pcmcia_unregister_driver(&mgslpc_driver);
-
-	/* XXX: this really needs to move into generic code.. */
-	while (dev_list != NULL) {
-		if (dev_list->state & DEV_CONFIG)
-			mgslpc_release((u_long)dev_list);
-		mgslpc_detach(dev_list);
-	}
+	synclink_cs_cleanup();
 }
 
 module_init(synclink_cs_init);

Re: [PATCH] 2.6.6 synclink_cs.c

[Russell King]

On Tue, Jun 01, 2004 at 03:53:02PM -0500, Paul Fulghum wrote:
> Patch to drivers/char/pcmcia/synclink_cs.c against 2.6.6
> to cleanup properly on errors during
> driver initialization.

To put my PCMCIA hat on, are you sure you should be registering with
a subsystem which can cause you to create tty devices before you've
registered with the tty subsystem?

Eg, what could happen if you register with PCMCIA, PCMCIA hands you
a card to drive and register a tty for, and you do all of that
_before_ you've registered with the tty subsystem?

-- 
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] 2.6.6 synclink_cs.c

[Paul Fulghum]

Russell King wrote:

>To put my PCMCIA hat on,
>
Is that the one with a propeller and flashing LEDs?
I'm jealous.

> are you sure you should be registering with
>a subsystem which can cause you to create tty devices before you've
>registered with the tty subsystem?
>
>Eg, what could happen if you register with PCMCIA, PCMCIA hands you
>a card to drive and register a tty for, and you do all of that
>_before_ you've registered with the tty subsystem?
>  
>
Nothing.

Devices can come and go. Until the driver
registers with the tty subsystem they are not
externally accessible.  The driver itself does
nothing tty related until it registers with the
tty subsystem and an external entity opens
the device as a tty device.

I believe what you describe happens with the 1st card insertion:
1. cardmgr loads the driver
2. driver registers with pcmcia
3. pcmcia creates device
4. driver registers with tty

Registering with pcmcia first makes cleanup a little easier
if it fails for some reason. This scheme has worked so far.

--
Paul Fulghum
paulkf@microgate.com

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Jurjen Oskam]

On Fri, May 28, 2004 at 01:42:50PM -0500, Paul Fulghum wrote:

> The following patch removes unnecessary disabling of
> interrupts when processing hangup for tty devices.
> 
> This was introduced way back in August 1998
> with patch 2.1.115 when the original hangup processing was
> changed from cli/sti to lock_kernel()/unlock_kernel().

I applied this patch to vanilla 2.2.5, and have been running with it
for a week or two. During that time, I frequently used PPP. For testing, I
interrupted the connection when sending and/or receiving data.

I didn't experience any problems whatsoever. 

Thanks,
-- 
Jurjen Oskam

"Avoid putting a paging file on a fault-tolerant drive, such as a mirrored
volume or a RAID-5 volume. Paging files do not need fault-tolerance."-MS Q308417

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Paul Fulghum]

On Sun, 2004-06-13 at 04:05, Jurjen Oskam wrote:
> On Fri, May 28, 2004 at 01:42:50PM -0500, Paul Fulghum wrote:
> 
> > The following patch removes unnecessary disabling of
> > interrupts when processing hangup for tty devices.
> > 
> > This was introduced way back in August 1998
> > with patch 2.1.115 when the original hangup processing was
> > changed from cli/sti to lock_kernel()/unlock_kernel().
> 
> I applied this patch to vanilla 2.2.5, and have been running with it
> for a week or two. During that time, I frequently used PPP. For testing, I
> interrupted the connection when sending and/or receiving data.
> 
> I didn't experience any problems whatsoever. 

Could I persuade you to also try this patch for ppp_synctty.c?
If I remember correctly, you were using this module for PPPoE.

It also fixes the softirq warning, but from a different angle.
The tty_io.c patch will not be accepted until tty locking is
completely reworked (which won't happen anytime soon).
The patch below mirrors changes to ppp_async.c which have
already been accepted.

The ppp_synctty patch also fixes some callbacks to allow
calls from hard interrupt context. I have been
banging on it for a week with no problems.

So, if possible, revert the tty_io.c patch, apply this one, and
continue your testing.

Thanks,
Paul

--- linux-2.6.6/drivers/net/ppp_synctty.c       2004-04-03 21:36:57.000000000 -0600
+++ linux-2.6.6-mg1/drivers/net/ppp_synctty.c   2004-06-02
15:01:35.177725315 -0500
@@ -65,7 +65,9 @@
        struct sk_buff  *tpkt;
        unsigned long   last_xmit;
 
-       struct sk_buff  *rpkt;
+       struct sk_buff_head rqueue;
+
+       struct tasklet_struct tsk;
 
        atomic_t        refcnt;
        struct semaphore dead_sem;
@@ -88,6 +90,7 @@
 static int ppp_sync_send(struct ppp_channel *chan, struct sk_buff
*skb);
 static int ppp_sync_ioctl(struct ppp_channel *chan, unsigned int cmd,
                          unsigned long arg);
+static void ppp_sync_process(unsigned long arg);
 static int ppp_sync_push(struct syncppp *ap);
 static void ppp_sync_flush_output(struct syncppp *ap);
 static void ppp_sync_input(struct syncppp *ap, const unsigned char
*buf,
@@ -217,6 +220,9 @@
        ap->xaccm[3] = 0x60000000U;
        ap->raccm = ~0U;
 
+       skb_queue_head_init(&ap->rqueue);
+       tasklet_init(&ap->tsk, ppp_sync_process, (unsigned long) ap);
+
        atomic_set(&ap->refcnt, 1);
        init_MUTEX_LOCKED(&ap->dead_sem);
 
@@ -267,10 +273,10 @@
         */
        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);
@@ -369,17 +375,24 @@
        return 65535;
 }
 
+/*
+ * This can now be called from hard interrupt level as well
+ * as soft interrupt level or mainline.
+ */
 static void
 ppp_sync_receive(struct tty_struct *tty, const unsigned char *buf,
-                 char *flags, int count)
+                 char *cflags, int count)
 {
        struct syncppp *ap = sp_get(tty);
+       unsigned long flags;
 
        if (ap == 0)
                return;
-       spin_lock_bh(&ap->recv_lock);
-       ppp_sync_input(ap, buf, flags, count);
-       spin_unlock_bh(&ap->recv_lock);
+       spin_lock_irqsave(&ap->recv_lock, flags);
+       ppp_sync_input(ap, buf, cflags, count);
+       spin_unlock_irqrestore(&ap->recv_lock, flags);
+       if (skb_queue_len(&ap->rqueue))
+               tasklet_schedule(&ap->tsk);
        sp_put(ap);
        if (test_and_clear_bit(TTY_THROTTLED, &tty->flags)
            && tty->driver->unthrottle)
@@ -394,8 +407,8 @@
        clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
        if (ap == 0)
                return;
-       if (ppp_sync_push(ap))
-               ppp_output_wakeup(&ap->chan);
+       set_bit(XMIT_WAKEUP, &ap->xmit_flags);
+       tasklet_schedule(&ap->tsk);
        sp_put(ap);
 }
 
@@ -449,9 +462,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;
 
@@ -512,6 +525,32 @@
 }
 
 /*
+ * 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_sync_process(unsigned long arg)
+{
+       struct syncppp *ap = (struct syncppp *) arg;
+       struct sk_buff *skb;
+
+       /* process received packets */
+       while ((skb = skb_dequeue(&ap->rqueue)) != NULL) {
+               if (skb->len == 0) {
+                       /* zero length buffers indicate error */
+                       ppp_input_error(&ap->chan, 0);
+                       kfree_skb(skb);
+               }
+               else
+                       ppp_input(&ap->chan, skb);
+       }
+
+       /* try to push more stuff out */
+       if (test_bit(XMIT_WAKEUP, &ap->xmit_flags) && ppp_sync_push(ap))
+               ppp_output_wakeup(&ap->chan);
+}
+
+/*
  * Procedures for encapsulation and framing.
  */
 
@@ -600,7 +639,6 @@
        struct tty_struct *tty = ap->tty;
        int tty_stuffed = 0;
 
-       set_bit(XMIT_WAKEUP, &ap->xmit_flags);
        if (!spin_trylock_bh(&ap->xmit_lock))
                return 0;
        for (;;) {
@@ -667,15 +705,44 @@
  * Receive-side routines.
  */
 
-static inline void
-process_input_packet(struct syncppp *ap)
+/* called when the tty driver has data for us. 
+ *
+ * Data is frame oriented: each call to ppp_sync_input is considered
+ * a whole frame. If the 1st flag byte is non-zero then the whole
+ * frame is considered to be in error and is tossed.
+ */
+static void
+ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
+               char *flags, int count)
 {
        struct sk_buff *skb;
        unsigned char *p;
-       int code = 0;
 
-       skb = ap->rpkt;
-       ap->rpkt = 0;
+       if (count == 0)
+               return;
+
+       if (ap->flags & SC_LOG_INPKT)
+               ppp_print_buffer ("receive buffer", buf, count);
+
+       /* stuff the chars in the skb */
+       if ((skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2)) == 0) {
+               printk(KERN_ERR "PPPsync: no memory (input pkt)\n");
+               goto err;
+       }
+       /* Try to get the payload 4-byte aligned */
+       if (buf[0] != PPP_ALLSTATIONS)
+               skb_reserve(skb, 2 + (buf[0] & 1));
+
+       if (flags != 0 && *flags) {
+               /* error flag set, ignore frame */
+               goto err;
+       } else if (count > skb_tailroom(skb)) {
+               /* packet overflowed MRU */
+               goto err;
+       }
+
+       p = skb_put(skb, count);
+       memcpy(p, buf, count);
 
        /* strip address/control field if present */
        p = skb->data;
@@ -693,59 +760,15 @@
        } else if (skb->len < 2)
                goto err;
 
-       /* pass to generic layer */
-       ppp_input(&ap->chan, skb);
+       /* queue the frame to be processed */
+       skb_queue_tail(&ap->rqueue, skb);
        return;
 
- err:
-       kfree_skb(skb);
-       ppp_input_error(&ap->chan, code);
-}
-
-/* called when the tty driver has data for us. 
- *
- * Data is frame oriented: each call to ppp_sync_input is considered
- * a whole frame. If the 1st flag byte is non-zero then the whole
- * frame is considered to be in error and is tossed.
- */
-static void
-ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
-               char *flags, int count)
-{
-       struct sk_buff *skb;
-       unsigned char *sp;
-
-       if (count == 0)
-               return;
-
-       /* if flag set, then error, ignore frame */
-       if (flags != 0 && *flags) {
-               ppp_input_error(&ap->chan, *flags);
-               return;
-       }
-
-       if (ap->flags & SC_LOG_INPKT)
-               ppp_print_buffer ("receive buffer", buf, count);
-
-       /* stuff the chars in the skb */
-       if ((skb = ap->rpkt) == 0) {
-               if ((skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2)) ==
0) {
-                       printk(KERN_ERR "PPPsync: no memory (input
pkt)\n");
-                       ppp_input_error(&ap->chan, 0);
-                       return;
-               }
-               /* Try to get the payload 4-byte aligned */
-               if (buf[0] != PPP_ALLSTATIONS)
-                       skb_reserve(skb, 2 + (buf[0] & 1));
-               ap->rpkt = skb;
-       }
-       if (count > skb_tailroom(skb)) {
-               /* packet overflowed MRU */
-               ppp_input_error(&ap->chan, 1);
-       } else {
-               sp = skb_put(skb, count);
-               memcpy(sp, buf, count);
-               process_input_packet(ap);
+err:
+       /* queue zero length packet as error indication */
+       if (skb || (skb = dev_alloc_skb(0))) {
+               skb_trim(skb, 0);
+               skb_queue_tail(&ap->rqueue, skb);
        }
 }
 

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Jurjen Oskam]

On Sun, Jun 13, 2004 at 08:29:17AM -0500, Paul Fulghum wrote:

> Could I persuade you to also try this patch for ppp_synctty.c?

The patch doesn't apply: it seems to contain spaces where there are
tabs in the original file, and some lines seem to be cut off early...

> If I remember correctly, you were using this module for PPPoE.

PPTP actually, but I do use the "sync" option of pppd.

-- 
Jurjen Oskam

"Avoid putting a paging file on a fault-tolerant drive, such as a mirrored
volume or a RAID-5 volume. Paging files do not need fault-tolerance."-MS Q308417

Re: [PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Paul Fulghum]

On Sun, 2004-06-13 at 09:24, Jurjen Oskam wrote:
> On Sun, Jun 13, 2004 at 08:29:17AM -0500, Paul Fulghum wrote:
> 
> > Could I persuade you to also try this patch for ppp_synctty.c?
> 
> The patch doesn't apply: it seems to contain spaces where there are
> tabs in the original file, and some lines seem to be cut off early...

My mail client mangled it, try this.

> > If I remember correctly, you were using this module for PPPoE.
> 
> PPTP actually, but I do use the "sync" option of pppd.

OK


--- linux-2.6.6/drivers/net/ppp_synctty.c       2004-04-03 21:36:57.000000000 -0600
+++ linux-2.6.6-mg1/drivers/net/ppp_synctty.c   2004-06-02 15:01:35.177725315 -0500
@@ -65,7 +65,9 @@
        struct sk_buff  *tpkt;
        unsigned long   last_xmit;
 
-       struct sk_buff  *rpkt;
+       struct sk_buff_head rqueue;
+
+       struct tasklet_struct tsk;
 
        atomic_t        refcnt;
        struct semaphore dead_sem;
@@ -88,6 +90,7 @@
 static int ppp_sync_send(struct ppp_channel *chan, struct sk_buff *skb);
 static int ppp_sync_ioctl(struct ppp_channel *chan, unsigned int cmd,
                          unsigned long arg);
+static void ppp_sync_process(unsigned long arg);
 static int ppp_sync_push(struct syncppp *ap);
 static void ppp_sync_flush_output(struct syncppp *ap);
 static void ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
@@ -217,6 +220,9 @@
        ap->xaccm[3] = 0x60000000U;
        ap->raccm = ~0U;
 
+       skb_queue_head_init(&ap->rqueue);
+       tasklet_init(&ap->tsk, ppp_sync_process, (unsigned long) ap);
+
        atomic_set(&ap->refcnt, 1);
        init_MUTEX_LOCKED(&ap->dead_sem);
 
@@ -267,10 +273,10 @@
         */
        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);
@@ -369,17 +375,24 @@
        return 65535;
 }
 
+/*
+ * This can now be called from hard interrupt level as well
+ * as soft interrupt level or mainline.
+ */
 static void
 ppp_sync_receive(struct tty_struct *tty, const unsigned char *buf,
-                 char *flags, int count)
+                 char *cflags, int count)
 {
        struct syncppp *ap = sp_get(tty);
+       unsigned long flags;
 
        if (ap == 0)
                return;
-       spin_lock_bh(&ap->recv_lock);
-       ppp_sync_input(ap, buf, flags, count);
-       spin_unlock_bh(&ap->recv_lock);
+       spin_lock_irqsave(&ap->recv_lock, flags);
+       ppp_sync_input(ap, buf, cflags, count);
+       spin_unlock_irqrestore(&ap->recv_lock, flags);
+       if (skb_queue_len(&ap->rqueue))
+               tasklet_schedule(&ap->tsk);
        sp_put(ap);
        if (test_and_clear_bit(TTY_THROTTLED, &tty->flags)
            && tty->driver->unthrottle)
@@ -394,8 +407,8 @@
        clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
        if (ap == 0)
                return;
-       if (ppp_sync_push(ap))
-               ppp_output_wakeup(&ap->chan);
+       set_bit(XMIT_WAKEUP, &ap->xmit_flags);
+       tasklet_schedule(&ap->tsk);
        sp_put(ap);
 }
 
@@ -449,9 +462,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;
 
@@ -512,6 +525,32 @@
 }
 
 /*
+ * 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_sync_process(unsigned long arg)
+{
+       struct syncppp *ap = (struct syncppp *) arg;
+       struct sk_buff *skb;
+
+       /* process received packets */
+       while ((skb = skb_dequeue(&ap->rqueue)) != NULL) {
+               if (skb->len == 0) {
+                       /* zero length buffers indicate error */
+                       ppp_input_error(&ap->chan, 0);
+                       kfree_skb(skb);
+               }
+               else
+                       ppp_input(&ap->chan, skb);
+       }
+
+       /* try to push more stuff out */
+       if (test_bit(XMIT_WAKEUP, &ap->xmit_flags) && ppp_sync_push(ap))
+               ppp_output_wakeup(&ap->chan);
+}
+
+/*
  * Procedures for encapsulation and framing.
  */
 
@@ -600,7 +639,6 @@
        struct tty_struct *tty = ap->tty;
        int tty_stuffed = 0;
 
-       set_bit(XMIT_WAKEUP, &ap->xmit_flags);
        if (!spin_trylock_bh(&ap->xmit_lock))
                return 0;
        for (;;) {
@@ -667,15 +705,44 @@
  * Receive-side routines.
  */
 
-static inline void
-process_input_packet(struct syncppp *ap)
+/* called when the tty driver has data for us. 
+ *
+ * Data is frame oriented: each call to ppp_sync_input is considered
+ * a whole frame. If the 1st flag byte is non-zero then the whole
+ * frame is considered to be in error and is tossed.
+ */
+static void
+ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
+               char *flags, int count)
 {
        struct sk_buff *skb;
        unsigned char *p;
-       int code = 0;
 
-       skb = ap->rpkt;
-       ap->rpkt = 0;
+       if (count == 0)
+               return;
+
+       if (ap->flags & SC_LOG_INPKT)
+               ppp_print_buffer ("receive buffer", buf, count);
+
+       /* stuff the chars in the skb */
+       if ((skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2)) == 0) {
+               printk(KERN_ERR "PPPsync: no memory (input pkt)\n");
+               goto err;
+       }
+       /* Try to get the payload 4-byte aligned */
+       if (buf[0] != PPP_ALLSTATIONS)
+               skb_reserve(skb, 2 + (buf[0] & 1));
+
+       if (flags != 0 && *flags) {
+               /* error flag set, ignore frame */
+               goto err;
+       } else if (count > skb_tailroom(skb)) {
+               /* packet overflowed MRU */
+               goto err;
+       }
+
+       p = skb_put(skb, count);
+       memcpy(p, buf, count);
 
        /* strip address/control field if present */
        p = skb->data;
@@ -693,59 +760,15 @@
        } else if (skb->len < 2)
                goto err;
 
-       /* pass to generic layer */
-       ppp_input(&ap->chan, skb);
+       /* queue the frame to be processed */
+       skb_queue_tail(&ap->rqueue, skb);
        return;
 
- err:
-       kfree_skb(skb);
-       ppp_input_error(&ap->chan, code);
-}
-
-/* called when the tty driver has data for us. 
- *
- * Data is frame oriented: each call to ppp_sync_input is considered
- * a whole frame. If the 1st flag byte is non-zero then the whole
- * frame is considered to be in error and is tossed.
- */
-static void
-ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
-               char *flags, int count)
-{
-       struct sk_buff *skb;
-       unsigned char *sp;
-
-       if (count == 0)
-               return;
-
-       /* if flag set, then error, ignore frame */
-       if (flags != 0 && *flags) {
-               ppp_input_error(&ap->chan, *flags);
-               return;
-       }
-
-       if (ap->flags & SC_LOG_INPKT)
-               ppp_print_buffer ("receive buffer", buf, count);
-
-       /* stuff the chars in the skb */
-       if ((skb = ap->rpkt) == 0) {
-               if ((skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2)) == 0) {
-                       printk(KERN_ERR "PPPsync: no memory (input pkt)\n");
-                       ppp_input_error(&ap->chan, 0);
-                       return;
-               }
-               /* Try to get the payload 4-byte aligned */
-               if (buf[0] != PPP_ALLSTATIONS)
-                       skb_reserve(skb, 2 + (buf[0] & 1));
-               ap->rpkt = skb;
-       }
-       if (count > skb_tailroom(skb)) {
-               /* packet overflowed MRU */
-               ppp_input_error(&ap->chan, 1);
-       } else {
-               sp = skb_put(skb, count);
-               memcpy(sp, buf, count);
-               process_input_packet(ap);
+err:
+       /* queue zero length packet as error indication */
+       if (skb || (skb = dev_alloc_skb(0))) {
+               skb_trim(skb, 0);
+               skb_queue_tail(&ap->rqueue, skb);
        }
 }