Re: [PATCH 06/13] USBATM: shutdown open connections when disconnected

Re: [PATCH 06/13] USBATM: shutdown open connections when disconnected

[juampe]

> This patch causes vcc_release_async to be applied to any *open
>** v*cc's when the modem is *disconnected*. 


> Unfortunately this patch may create problems
> for those rare users like me who use routed IP or some other
> non-ppp connection method that goes via the ATM ARP daemon: the
> daemon is buggy, and with this patch will crash when the modem
> is *disconnected*.  Users with a buggy atmarpd can simply restart
> it after disconnecting the modem.

First i must thanks all effort in usbatm development.
IMHO New fatures to a driver that works well and can block the use, 
especially if it can disable internet access and the problem is know,
MUST be disabled by default or provide a mechanism to disable it.

I'm a rare user with routed IP and this patch blocks the normal use of internet
I dont understand how this patch can be accepted for a stable release without 
any kind of disable mechanism.

Yeah, i know that atmarp is buggy, but before speedtouch driver and atm works well during months.

Best Regards.
Juampe.

Re: [PATCH 06/13] USBATM: shutdown open connections when disconnected

[juampe]

juampe wrote:
>> This patch causes vcc_release_async to be applied to any *open
>> ** v*cc's when the modem is *disconnected*. 
>>     
>
>
>   
>> Unfortunately this patch may create problems
>> for those rare users like me who use routed IP or some other
>> non-ppp connection method that goes via the ATM ARP daemon: the
>> daemon is buggy, and with this patch will crash when the modem
>> is *disconnected*.  Users with a buggy atmarpd can simply restart
>> it after disconnecting the modem.
>>     
>
> First i must thanks all effort in usbatm development.
> IMHO New fatures to a driver that works well and can block the use, 
> especially if it can disable internet access and the problem is know,
> MUST be disabled by default or provide a mechanism to disable it.
>
> I'm a rare user with routed IP and this patch blocks the normal use of internet
> I dont understand how this patch can be accepted for a stable release without 
> any kind of disable mechanism.
>
> Yeah, i know that atmarp is buggy, but before speedtouch driver and atm works well during months.
>
> Best Regards.
> Juampe.
>
>
>   
I really sorry, this is a patch to 2.6.16 and this kernel version works
well the speedtouch driver.
I have problems with 2.6.17 and the speedtocuh driver block the
conection at some point that  i can't locate.

wave:~# atmdiag
Itf       TX_okay   TX_err    RX_okay   RX_err    RX_drop
  0 AAL0         0         0         0         0         0
    AAL5    266537         0    198023         1         0

RX cells don't increment, AFAIK the driver don't manage incoming ATM cells

How i can dig more into this issue in order to extract  relevant
information for a bug report?.

Best Regards
Juampe.

Re: [PATCH 06/13] USBATM: shutdown open connections when disconnected

[Duncan Sands]

Hi Juampe,

On Monday 19 June 2006 10:13, juampe wrote:
> > This patch causes vcc_release_async to be applied to any *open
> >** v*cc's when the modem is *disconnected*. 
> 
> 
> > Unfortunately this patch may create problems
> > for those rare users like me who use routed IP or some other
> > non-ppp connection method that goes via the ATM ARP daemon: the
> > daemon is buggy, and with this patch will crash when the modem
> > is *disconnected*.  Users with a buggy atmarpd can simply restart
> > it after disconnecting the modem.
> 
> First i must thanks all effort in usbatm development.
> IMHO New fatures to a driver that works well and can block the use, 
> especially if it can disable internet access and the problem is know,
> MUST be disabled by default or provide a mechanism to disable it.
>
> I'm a rare user with routed IP and this patch blocks the normal use of internet
> I dont understand how this patch can be accepted for a stable release without 
> any kind of disable mechanism.
> 
> Yeah, i know that atmarp is buggy, but before speedtouch driver and atm works well during months.

why don't you just restart atmarpd?  After all, if you are unplugging and
replugging your modem, surely you can restart the daemon at the same time?

I didn't feel it was necessary to have an override mechanism for this new,
correct behavior (which makes things better for people using pppd, i.e. most
people) since a simple workaround exists.

What is very bad however is that atmarpd is still not fixed.  I had a look,
got stuck, and asked for help on the linux-atm list, but no-one replied.
There has been no progress since then.  I will have another look - maybe
you can too?

Best wishes,

Duncan.

Re: [PATCH 06/13] USBATM: shutdown open connections when disconnected

[Duncan Sands]

> I really sorry, this is a patch to 2.6.16 and this kernel version works
> well the speedtouch driver.
> I have problems with 2.6.17 and the speedtocuh driver block the
> conection at some point that  i can't locate.
> 
> wave:~# atmdiag
> Itf       TX_okay   TX_err    RX_okay   RX_err    RX_drop
>   0 AAL0         0         0         0         0         0
>     AAL5    266537         0    198023         1         0
> 
> RX cells don't increment, AFAIK the driver don't manage incoming ATM cells
> 
> How i can dig more into this issue in order to extract  relevant
> information for a bug report?.

Hi Juampe, there were only two changes to the speedtouch driver
between 2.6.16 and 2.6.17: a cosmetic one (change to modinfo
output), and one only affecting people using iso urb support
(are you running with enable_isoc=1?).  So any new problems in
2.6.17 must be coming from elsewhere, such as ATM or USB changes.
I didn't spot any interesting ATM changes, and haven't looked at
the USB changes yet.  Are you sure you don't get the same problem
with 2.6.16?  Also, do you get any interesting messages in your
system logs (eg: check the dmesg output)?

Ciao,

Duncan.

Re: [PATCH 06/13] USBATM: shutdown open connections when disconnected

[Duncan Sands]

Hi Juampe,

> I have problems with 2.6.17 and the speedtocuh driver block the
> conection at some point that  i can't locate.

I've just had a second report about problems with 2.6.17.
Two things to try:
(1) build your kernel with CONFIG_USB_DEBUG=y and check for
interesting messages in your system logs (dmesg).
(2) use bisection to try to work out exactly which change
to the kernel caused this problem to appear.  Check out
Documentation/BUG-HUNTING in the kernel source for an
description of how to do this.

Ciao,

Duncan.

[PATCH 06/13] USBATM: shutdown open connections when disconnected

[Duncan Sands]

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

This patch causes vcc_release_async to be applied to any open
vcc's when the modem is disconnected.  This signals a socket
shutdown, letting the socket user know that the game is up.
I wrote this patch because of reports that pppd would keep
connections open forever when the modem is disconnected.
This patch does not fix that problem, but it's a step in the
right direction.  It doesn't help because the pppoatm module
doesn't yet monitor state changes on the ATM socket, so simply
never realises that the ATM connection has gone down (meaning
it doesn't tell the ppp layer).  But at least there is a socket
state change now.  Unfortunately this patch may create problems
for those rare users like me who use routed IP or some other
non-ppp connection method that goes via the ATM ARP daemon: the
daemon is buggy, and with this patch will crash when the modem
is disconnected.  Users with a buggy atmarpd can simply restart
it after disconnecting the modem.

Signed-off-by: Duncan Sands <baldrick@free.fr>

[-- Attachment #2: 06-disconnect --]
[-- Type: text/x-diff, Size: 4338 bytes --]

Index: Linux/drivers/usb/atm/usbatm.c
===================================================================
--- Linux.orig/drivers/usb/atm/usbatm.c	2006-01-13 08:51:00.000000000 +0100
+++ Linux/drivers/usb/atm/usbatm.c	2006-01-13 08:57:48.000000000 +0100
@@ -602,8 +602,12 @@
 
 	vdbg("%s called (skb 0x%p, len %u)", __func__, skb, skb->len);
 
-	if (!instance) {
-		dbg("%s: NULL data!", __func__);
+	/* racy disconnection check - fine */
+	if (!instance || instance->disconnected) {
+#ifdef DEBUG
+		if (printk_ratelimit())
+			printk(KERN_DEBUG "%s: %s!\n", __func__, instance ? "disconnected" : "NULL instance");
+#endif
 		err = -ENODEV;
 		goto fail;
 	}
@@ -715,15 +719,19 @@
 			       atomic_read(&atm_dev->stats.aal5.rx_err),
 			       atomic_read(&atm_dev->stats.aal5.rx_drop));
 
-	if (!left--)
-		switch (atm_dev->signal) {
-		case ATM_PHY_SIG_FOUND:
-			return sprintf(page, "Line up\n");
-		case ATM_PHY_SIG_LOST:
-			return sprintf(page, "Line down\n");
-		default:
-			return sprintf(page, "Line state unknown\n");
-		}
+	if (!left--) {
+		if (instance->disconnected)
+			return sprintf(page, "Disconnected\n");
+		else
+			switch (atm_dev->signal) {
+			case ATM_PHY_SIG_FOUND:
+				return sprintf(page, "Line up\n");
+			case ATM_PHY_SIG_LOST:
+				return sprintf(page, "Line down\n");
+			default:
+				return sprintf(page, "Line state unknown\n");
+			}
+	}
 
 	return 0;
 }
@@ -757,6 +765,12 @@
 
 	down(&instance->serialize);	/* vs self, usbatm_atm_close, usbatm_usb_disconnect */
 
+	if (instance->disconnected) {
+		atm_dbg(instance, "%s: disconnected!\n", __func__);
+		ret = -ENODEV;
+		goto fail;
+	}
+
 	if (usbatm_find_vcc(instance, vpi, vci)) {
 		atm_dbg(instance, "%s: %hd/%d already in use!\n", __func__, vpi, vci);
 		ret = -EADDRINUSE;
@@ -845,6 +859,13 @@
 static int usbatm_atm_ioctl(struct atm_dev *atm_dev, unsigned int cmd,
 			  void __user * arg)
 {
+	struct usbatm_data *instance = atm_dev->dev_data;
+
+	if (!instance || instance->disconnected) {
+		dbg("%s: %s!", __func__, instance ? "disconnected" : "NULL instance");
+		return -ENODEV;
+	}
+
 	switch (cmd) {
 	case ATM_QUERYLOOP:
 		return put_user(ATM_LM_NONE, (int __user *)arg) ? -EFAULT : 0;
@@ -1129,6 +1150,7 @@
 {
 	struct device *dev = &intf->dev;
 	struct usbatm_data *instance = usb_get_intfdata(intf);
+	struct usbatm_vcc_data *vcc_data;
 	int i;
 
 	dev_dbg(dev, "%s entered\n", __func__);
@@ -1141,12 +1163,18 @@
 	usb_set_intfdata(intf, NULL);
 
 	down(&instance->serialize);
+	instance->disconnected = 1;
 	if (instance->thread_pid >= 0)
 		kill_proc(instance->thread_pid, SIGTERM, 1);
 	up(&instance->serialize);
 
 	wait_for_completion(&instance->thread_exited);
 
+	down(&instance->serialize);
+	list_for_each_entry(vcc_data, &instance->vcc_list, list)
+		vcc_release_async(vcc_data->vcc, -EPIPE);
+	up(&instance->serialize);
+
 	tasklet_disable(&instance->rx_channel.tasklet);
 	tasklet_disable(&instance->tx_channel.tasklet);
 
@@ -1156,6 +1184,14 @@
 	del_timer_sync(&instance->rx_channel.delay);
 	del_timer_sync(&instance->tx_channel.delay);
 
+	/* turn usbatm_[rt]x_process into something close to a no-op */
+	/* no need to take the spinlock */
+	INIT_LIST_HEAD(&instance->rx_channel.list);
+	INIT_LIST_HEAD(&instance->tx_channel.list);
+
+	tasklet_enable(&instance->rx_channel.tasklet);
+	tasklet_enable(&instance->tx_channel.tasklet);
+
 	if (instance->atm_dev && instance->driver->atm_stop)
 		instance->driver->atm_stop(instance, instance->atm_dev);
 
@@ -1164,14 +1200,6 @@
 
 	instance->driver_data = NULL;
 
-	/* turn usbatm_[rt]x_process into noop */
-	/* no need to take the spinlock */
-	INIT_LIST_HEAD(&instance->rx_channel.list);
-	INIT_LIST_HEAD(&instance->tx_channel.list);
-
-	tasklet_enable(&instance->rx_channel.tasklet);
-	tasklet_enable(&instance->tx_channel.tasklet);
-
 	for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++) {
 		kfree(instance->urbs[i]->transfer_buffer);
 		usb_free_urb(instance->urbs[i]);
Index: Linux/drivers/usb/atm/usbatm.h
===================================================================
--- Linux.orig/drivers/usb/atm/usbatm.h	2006-01-13 08:48:09.000000000 +0100
+++ Linux/drivers/usb/atm/usbatm.h	2006-01-13 08:57:48.000000000 +0100
@@ -168,6 +168,7 @@
 
 	struct kref refcount;
 	struct semaphore serialize;
+	int disconnected;
 
 	/* heavy init */
 	int thread_pid;