[PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[Jiri Slaby]

We register lapb when tty is created, but unregister it only when the
device is UP. So move the lapb_unregister to x25_asy_close_tty after
the device is down.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: Sergey Lapin <slapin@ossfans.org>
Cc: Andrew Hendry <andrew.hendry@gmail.com>
---
 drivers/net/wan/x25_asy.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
index 66cda25..24297b2 100644
--- a/drivers/net/wan/x25_asy.c
+++ b/drivers/net/wan/x25_asy.c
@@ -498,7 +498,6 @@ norbuff:
 static int x25_asy_close(struct net_device *dev)
 {
 	struct x25_asy *sl = netdev_priv(dev);
-	int err;
 
 	spin_lock(&sl->lock);
 	if (sl->tty)
@@ -507,10 +506,6 @@ static int x25_asy_close(struct net_device *dev)
 	netif_stop_queue(dev);
 	sl->rcount = 0;
 	sl->xleft  = 0;
-	err = lapb_unregister(dev);
-	if (err != LAPB_OK)
-		printk(KERN_ERR "x25_asy_close: lapb_unregister error -%d\n",
-			err);
 	spin_unlock(&sl->lock);
 	return 0;
 }
@@ -595,6 +590,7 @@ static int x25_asy_open_tty(struct tty_struct *tty)
 static void x25_asy_close_tty(struct tty_struct *tty)
 {
 	struct x25_asy *sl = tty->disc_data;
+	int err;
 
 	/* First make sure we're connected. */
 	if (!sl || sl->magic != X25_ASY_MAGIC)
@@ -605,6 +601,11 @@ static void x25_asy_close_tty(struct tty_struct *tty)
 		dev_close(sl->dev);
 	rtnl_unlock();
 
+	err = lapb_unregister(sl->dev);
+	if (err != LAPB_OK)
+		printk(KERN_ERR "x25_asy_close: lapb_unregister error -%d\n",
+			err);
+
 	tty->disc_data = NULL;
 	sl->tty = NULL;
 	x25_asy_free(sl);
-- 
1.7.3.1

Re: [PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[Andrew Hendry]

Sorry I haven't used this driver so can't fully test it. Looks
straightforward and compile tested ok.

On Thu, Nov 25, 2010 at 10:54 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> We register lapb when tty is created, but unregister it only when the
> device is UP. So move the lapb_unregister to x25_asy_close_tty after
> the device is down.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: Sergey Lapin <slapin@ossfans.org>
> Cc: Andrew Hendry <andrew.hendry@gmail.com>
> ---
>  drivers/net/wan/x25_asy.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
> index 66cda25..24297b2 100644
> --- a/drivers/net/wan/x25_asy.c
> +++ b/drivers/net/wan/x25_asy.c
> @@ -498,7 +498,6 @@ norbuff:
>  static int x25_asy_close(struct net_device *dev)
>  {
>        struct x25_asy *sl = netdev_priv(dev);
> -       int err;
>
>        spin_lock(&sl->lock);
>        if (sl->tty)
> @@ -507,10 +506,6 @@ static int x25_asy_close(struct net_device *dev)
>        netif_stop_queue(dev);
>        sl->rcount = 0;
>        sl->xleft  = 0;
> -       err = lapb_unregister(dev);
> -       if (err != LAPB_OK)
> -               printk(KERN_ERR "x25_asy_close: lapb_unregister error -%d\n",
> -                       err);
>        spin_unlock(&sl->lock);
>        return 0;
>  }
> @@ -595,6 +590,7 @@ static int x25_asy_open_tty(struct tty_struct *tty)
>  static void x25_asy_close_tty(struct tty_struct *tty)
>  {
>        struct x25_asy *sl = tty->disc_data;
> +       int err;
>
>        /* First make sure we're connected. */
>        if (!sl || sl->magic != X25_ASY_MAGIC)
> @@ -605,6 +601,11 @@ static void x25_asy_close_tty(struct tty_struct *tty)
>                dev_close(sl->dev);
>        rtnl_unlock();
>
> +       err = lapb_unregister(sl->dev);
> +       if (err != LAPB_OK)
> +               printk(KERN_ERR "x25_asy_close: lapb_unregister error -%d\n",
> +                       err);
> +
>        tty->disc_data = NULL;
>        sl->tty = NULL;
>        x25_asy_free(sl);
> --
> 1.7.3.1
>
>
>

Re: [PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[Jiri Slaby]

On 11/25/2010 12:37 PM, Andrew Hendry wrote:
> Sorry I haven't used this driver so can't fully test it. Looks
> straightforward and compile tested ok.
> 
> On Thu, Nov 25, 2010 at 10:54 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>> We register lapb when tty is created, but unregister it only when the
>> device is UP. So move the lapb_unregister to x25_asy_close_tty after
>> the device is down.

I forgot to mention what it causes. The commit message should add:
The old behaviour causes ldisc switching to fail each second attempt,
because we noted for us that the device is unused, so we use it the
second time, but labp layer still have it registered, so it fails
obviously.

thanks,
-- 
js

Re: [PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[Sergey Lapin]

On Thu, Nov 25, 2010 at 12:54:54AM +0100, Jiri Slaby wrote:
> We register lapb when tty is created, but unregister it only when the
> device is UP. So move the lapb_unregister to x25_asy_close_tty after
> the device is down.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: Sergey Lapin <slapin@ossfans.org>
> Cc: Andrew Hendry <andrew.hendry@gmail.com>
Tested-by: Sergey Lapin <slapin@ossfans.org>
Tested-by: Mikhail Ulyanov <ulyanov.mikhail@gmail.com>

Re: [PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[Andrew Hendry]

Looks like some more X.25 users. Not sure on your usage, but I should point out:
https://patchwork.kernel.org/patch/332981/
X.25 protocol needs to have the big kernel lock removed so it can stay
around in the future.
I have been working through them, but there are still some of the more
complex ones remaining.

The test setups I am using are:
Sockets<->X25<->x25loop<->X25<->Sockets
Sockets<->X25<->xotd<->network<->xotd<->X25<->Sockets
and I know some users have:
Sockets<->X25<->xotd<->network<->CiscoXOT<->devices.

Where x25loop is a userspace tun device which shuffles the LCIs, does
some basic call handling and loops the calls back into kernel X25.
xotd is a basic implementation of the X25 over TCP RFC.

Are you using something like this?
Sockets<->X25<->lapb<->x25_async<->.....

If you are using X25 I would be interested to see how your setup goes
with the X25 BKL cleanup work, and if anyone wanted to help with the
remaining BKLs :)


On Fri, Nov 26, 2010 at 9:56 AM, Sergey Lapin <slapin@ossfans.org> wrote:
> On Thu, Nov 25, 2010 at 12:54:54AM +0100, Jiri Slaby wrote:
>> We register lapb when tty is created, but unregister it only when the
>> device is UP. So move the lapb_unregister to x25_asy_close_tty after
>> the device is down.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Reported-by: Sergey Lapin <slapin@ossfans.org>
>> Cc: Andrew Hendry <andrew.hendry@gmail.com>
> Tested-by: Sergey Lapin <slapin@ossfans.org>
> Tested-by: Mikhail Ulyanov <ulyanov.mikhail@gmail.com>
>

Re: [PATCH 1/1] NET: wan/x25_asy, move lapb_unregister to x25_asy_close_tty

[David Miller]

From: Jiri Slaby <jslaby@suse.cz>
Date: Thu, 25 Nov 2010 00:54:54 +0100

> We register lapb when tty is created, but unregister it only when the
> device is UP. So move the lapb_unregister to x25_asy_close_tty after
> the device is down.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: Sergey Lapin <slapin@ossfans.org>
> Cc: Andrew Hendry <andrew.hendry@gmail.com>

Applied with commit message addition you mentioned Jiri.

Thanks.