[PATCH 0/5] N_GSM patchset : 06/03/2011

[PATCH 0/5] N_GSM patchset : 06/03/2011

[Russ Gorby]

Hello N_GSM Maintainers,

I'm sending this patchset for your consideration.
These are the pertinent changes that we have done to the driver.
Thank you for yor comments.

[PATCH 1/5] tty: n_gsm: Add raw-ip support
       - Added netif glue so a DLCI can map to a network IF
[PATCH 2/5] tty: n_gsm: expose gsmtty device nodes at ldisc open time
       - Caused gsmttyN device nodes to be exposed automatically.
[PATCH 3/5] tty: n_gsm: Added refcount usage to gsm_mux and gsm_dlci structs
       - added refcount access to struct shared between MUX and
         channel clients
[PATCH 4/5] tty: n_gsm: initiate close of all DLCIs during mux shutdown
[PATCH 5/5] tty: n_gsm: Fixed NULL ptr OOPs in tty_write_room()

[PATCH 1/5] tty: n_gsm: Add raw-ip support

[Russ Gorby]

This patch adds the ability to open a network data connection over a mux
virtual tty channel. This is for modems that support data connections
with raw IP frames instead of PPP. On high speed data connections this
eliminates a significant amount of PPP overhead. To use this interface,
the application must first tell the modem to open a network connection on
a virtual tty. Once that has been accomplished, the app will issue an
IOCTL on that virtual tty to create the network interface. The IOCTL will
return the index of the interface created.

The two IOCTL commands are:

    ioctl( fd, GSMIOC_ENABLE_NET );

    ioctl( fd, GSMIOC_DISABLE_NET );

Signed-off-by: Russ Gorby <russ.gorby@intel.com>
---
 drivers/tty/n_gsm.c    |  260 ++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/gsmmux.h |   11 ++
 2 files changed, 263 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index a4c42a7..47897c2 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -58,6 +58,10 @@
 #include <linux/serial.h>
 #include <linux/kfifo.h>
 #include <linux/skbuff.h>
+#include <net/arp.h>
+#include <linux/ip.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
 #include <linux/gsmmux.h>
 
 static int debug;
@@ -77,8 +81,24 @@ module_param(debug, int, 0600);
  * Semi-arbitrary buffer size limits. 0710 is normally run with 32-64 byte
  * limits so this is plenty
  */
-#define MAX_MRU 512
-#define MAX_MTU 512
+#define MAX_MRU 1500
+#define MAX_MTU 1500
+#define	GSM_NET_TX_TIMEOUT (HZ*10)
+
+/**
+ *	struct gsm_mux_net	-	network interface
+ *	@struct gsm_dlci* dlci
+ *	@struct net_device_stats stats;
+ *
+ *	Created when net interface is initialized.
+ **/
+struct gsm_mux_net {
+	struct kref ref;
+	struct gsm_dlci *dlci;
+	struct net_device_stats stats;
+};
+
+#define STATS(net) (((struct gsm_mux_net *)netdev_priv(net))->stats)
 
 /*
  *	Each block of data we have queued to go out is in the form of
@@ -134,6 +154,7 @@ struct gsm_dlci {
 	struct sk_buff_head skb_list;	/* Queued frames */
 	/* Data handling callback */
 	void (*data)(struct gsm_dlci *dlci, u8 *data, int len);
+	struct net_device *net; /* network interface, if created */
 };
 
 /* DLCI 0, 62/63 are special or reseved see gsmtty_open */
@@ -877,8 +898,10 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
 	}
 	memcpy(dp, skb_pull(dlci->skb, len), len);
 	__gsm_data_queue(dlci, msg);
-	if (last)
+	if (last) {
+		kfree_skb(dlci->skb);
 		dlci->skb = NULL;
+	}
 	return size;
 }
 
@@ -911,7 +934,7 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
 			i++;
 			continue;
 		}
-		if (dlci->adaption < 3)
+		if (dlci->adaption < 3 && !dlci->net)
 			len = gsm_dlci_data_output(gsm, dlci);
 		else
 			len = gsm_dlci_data_output_framed(gsm, dlci);
@@ -938,9 +961,12 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
 
 	spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
 	/* If we have nothing running then we need to fire up */
-	if (dlci->gsm->tx_bytes == 0)
-		gsm_dlci_data_output(dlci->gsm, dlci);
-	else if (dlci->gsm->tx_bytes < TX_THRESH_LO)
+	if (dlci->gsm->tx_bytes == 0) {
+		if (dlci->net)
+			gsm_dlci_data_output_framed(dlci->gsm, dlci);
+		else
+			gsm_dlci_data_output(dlci->gsm, dlci);
+	} else if (dlci->gsm->tx_bytes < TX_THRESH_LO)
 		gsm_dlci_data_sweep(dlci->gsm);
 	spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
 }
@@ -1590,6 +1616,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 	dlci->addr = addr;
 	dlci->adaption = gsm->adaption;
 	dlci->state = DLCI_CLOSED;
+	dlci->net = NULL;	/* network not initially created */
 	if (addr)
 		dlci->data = gsm_dlci_data;
 	else
@@ -2464,6 +2491,201 @@ static int gsmld_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
+/*
+ *	Network interface
+ *
+ */
+
+static int gsm_mux_net_open(struct net_device *net)
+{
+	pr_debug("%s called\n", __func__);
+	netif_start_queue(net);
+	return 0;
+}
+
+static int gsm_mux_net_close(struct net_device *net)
+{
+	netif_stop_queue(net);
+	return 0;
+}
+
+static struct net_device_stats *gsm_mux_net_get_stats(struct net_device *net)
+{
+	return &((struct gsm_mux_net *)netdev_priv(net))->stats;
+}
+static void net_free(struct kref *ref)
+{
+	struct gsm_mux_net *mux_net;
+	struct gsm_dlci *dlci;
+
+	mux_net = container_of(ref, struct gsm_mux_net, ref);
+	dlci = mux_net->dlci;
+
+	if (dlci->net) {
+		dlci->adaption = 1;
+		dlci->data = gsm_dlci_data;
+		unregister_netdev(dlci->net);
+		free_netdev(dlci->net);
+		dlci->net = 0;
+	}
+}
+static int gsm_mux_net_start_xmit(struct sk_buff *skb,
+				      struct net_device *net)
+{
+	struct gsm_dlci *dlci = ((struct gsm_mux_net *)netdev_priv(net))->dlci;
+	struct gsm_mux_net *mux_net;
+
+	mux_net = (struct gsm_mux_net *)netdev_priv(net);
+	kref_get(&mux_net->ref);
+
+	skb_queue_head(&dlci->skb_list, skb);
+	STATS(net).tx_packets++;
+	STATS(net).tx_bytes += skb->len;
+	gsm_dlci_data_kick(dlci);
+	/* And tell the kernel when the last transmit started. */
+	net->trans_start = jiffies;
+	kref_put(&mux_net->ref, net_free);
+	return NETDEV_TX_OK;
+}
+
+/* called when a packet did not ack after watchdogtimeout */
+static void gsm_mux_net_tx_timeout(struct net_device *net)
+{
+	/* Tell syslog we are hosed. */
+	dev_dbg(&net->dev, "Tx timed out.\n");
+
+	/* Update statistics */
+	STATS(net).tx_errors++;
+}
+
+static void gsm_mux_rx_netchar(struct gsm_dlci *dlci,
+				   unsigned char *in_buf, int size)
+{
+	struct net_device *net = dlci->net;
+	struct sk_buff *skb;
+	struct gsm_mux_net *mux_net;
+
+	mux_net = (struct gsm_mux_net *)netdev_priv(net);
+	kref_get(&mux_net->ref);
+
+	/* Allocate an sk_buff */
+	skb = dev_alloc_skb(size + NET_IP_ALIGN);
+	if (!skb) {
+		/* We got no receive buffer. */
+		STATS(net).rx_dropped++;
+		kref_put(&mux_net->ref, net_free);
+		return;
+	}
+	skb_reserve(skb, NET_IP_ALIGN);
+	memcpy(skb_put(skb, size), in_buf, size);
+
+	skb->dev = net;
+	skb->protocol = __constant_htons(ETH_P_IP);
+
+	/* Ship it off to the kernel */
+	netif_rx(skb);
+
+	/* update out statistics */
+	STATS(net).rx_packets++;
+	STATS(net).rx_bytes += size;
+	kref_put(&mux_net->ref, net_free);
+	return;
+}
+
+int gsm_change_mtu(struct net_device *net, int new_mtu)
+{
+	if ((new_mtu < 8) || (new_mtu > MAX_MTU))
+		return -EINVAL;
+	net->mtu = new_mtu;
+	return 0;
+}
+
+static void gsm_mux_net_init(struct net_device *net)
+{
+	static const struct net_device_ops gsm_netdev_ops = {
+		.ndo_open		= gsm_mux_net_open,
+		.ndo_stop		= gsm_mux_net_close,
+		.ndo_start_xmit		= gsm_mux_net_start_xmit,
+		.ndo_tx_timeout		= gsm_mux_net_tx_timeout,
+		.ndo_get_stats		= gsm_mux_net_get_stats,
+		.ndo_change_mtu		= gsm_change_mtu,
+	};
+	net->netdev_ops = &gsm_netdev_ops;
+
+	/* fill in the other fields */
+	net->watchdog_timeo = GSM_NET_TX_TIMEOUT;
+	net->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
+	net->type = ARPHRD_NONE;
+	net->mtu = MAX_MTU;
+	net->tx_queue_len = 10;
+}
+
+
+static void gsm_destroy_network(struct gsm_dlci *dlci)
+{
+	struct gsm_mux_net *mux_net;
+
+	pr_debug("destroy network interface");
+	if (dlci->net) {
+		mux_net = (struct gsm_mux_net *)netdev_priv(dlci->net);
+		kref_put(&mux_net->ref, net_free);
+	}
+}
+
+
+static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
+{
+	char *netname;
+	int retval = 0;
+	struct net_device *net;
+	struct gsm_mux_net *mux_net;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	/* Already in a non tty mode */
+	if (dlci->adaption > 2)
+		return -EBUSY;
+
+	if (nc->protocol != htons(ETH_P_IP))
+		return -EPROTONOSUPPORT;
+
+	if (nc->adaption != 3 && nc->adaption != 4)
+		return -EPROTONOSUPPORT;
+
+	pr_debug("create network interface");
+
+	netname = "gsm%d";
+	if (nc->if_name[0] != '\0')
+		netname = nc->if_name;
+	net = alloc_netdev(sizeof(struct gsm_mux_net),
+			netname,
+			gsm_mux_net_init);
+	if (!net) {
+		pr_err("alloc_netdev failed");
+		retval = -ENOMEM;
+		goto error_ret;
+	}
+	pr_debug("register netdev");
+	net->mtu = dlci->gsm->mtu;
+	retval = register_netdev(net);
+	if (retval) {
+		pr_err("network register fail %d\n", retval);
+		free_netdev(net);
+		goto error_ret;
+	}
+	dlci->net = net;
+	dlci->adaption = nc->adaption;
+	dlci->data = gsm_mux_rx_netchar;
+	mux_net = (struct gsm_mux_net *)netdev_priv(net);
+	mux_net->dlci = dlci;
+	kref_init(&mux_net->ref);
+	strncpy(nc->if_name, net->name, IFNAMSIZ); /* return net name */
+	return net->ifindex;	/* return network index */
+
+error_ret:
+	return retval;
+}
 
 /* Line discipline for real tty */
 struct tty_ldisc_ops tty_ldisc_packet = {
@@ -2584,6 +2806,7 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 	struct gsm_dlci *dlci = tty->driver_data;
 	if (dlci == NULL)
 		return;
+	gsm_destroy_network(dlci);
 	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
 		return;
 	gsm_dlci_begin_close(dlci);
@@ -2665,7 +2888,28 @@ static int gsmtty_tiocmset(struct tty_struct *tty,
 static int gsmtty_ioctl(struct tty_struct *tty,
 			unsigned int cmd, unsigned long arg)
 {
-	return -ENOIOCTLCMD;
+	struct gsm_dlci *dlci = tty->driver_data;
+	struct gsm_netconfig nc;
+	int index;
+
+	switch (cmd) {
+	case GSMIOC_ENABLE_NET:
+		if (copy_from_user(&nc, (void __user *)arg, sizeof(nc)))
+			return -EFAULT;
+		nc.if_name[IFNAMSIZ-1] = '\0';
+		/* return net interface index or error code */
+		index = gsm_create_network(dlci, &nc);
+		if (copy_to_user((void __user *)arg, &nc, sizeof(nc)))
+			return -EFAULT;
+		return index;
+	case GSMIOC_DISABLE_NET:
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+		gsm_destroy_network(dlci);
+		return 0;
+	default:
+		return -ENOIOCTLCMD;
+	}
 }
 
 static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
diff --git a/include/linux/gsmmux.h b/include/linux/gsmmux.h
index 378de41..c25e947 100644
--- a/include/linux/gsmmux.h
+++ b/include/linux/gsmmux.h
@@ -21,5 +21,16 @@ struct gsm_config
 #define GSMIOC_GETCONF		_IOR('G', 0, struct gsm_config)
 #define GSMIOC_SETCONF		_IOW('G', 1, struct gsm_config)
 
+struct gsm_netconfig {
+	unsigned int adaption;  /* Adaption to use in network mode */
+	unsigned short protocol;/* Protocol to use - only ETH_P_IP supported */
+	unsigned short unused2;
+	char if_name[IFNAMSIZ];	/* interface name format string */
+	__u8 unused[28];        /* For future use */
+};
+
+#define GSMIOC_ENABLE_NET      _IOW('G', 2, struct gsm_netconfig)
+#define GSMIOC_DISABLE_NET     _IO('G', 3)
+
 
 #endif
-- 
1.7.0.4

Re: [PATCH 1/5] tty: n_gsm: Add raw-ip support

[Alan Cox]

> @@ -1590,6 +1616,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
>  	dlci->addr = addr;
>  	dlci->adaption = gsm->adaption;
>  	dlci->state = DLCI_CLOSED;
> +	dlci->net = NULL;	/* network not initially created */

kzalloc ised used anyway so this starts NULL

>
> +int gsm_change_mtu(struct net_device *net, int new_mtu)
> +{
> +	if ((new_mtu < 8) || (new_mtu > MAX_MTU))
> +		return -EINVAL;
> +	net->mtu = new_mtu;
> +	return 0;
> +}

Surely at that point you need to renegotiate the DLCI parameters for the
DLCI in question. At the very least you need to sanity check versus the
current settings ?


> +static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
> +{
> +	char *netname;
> +	int retval = 0;
> +	struct net_device *net;
> +	struct gsm_mux_net *mux_net;
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	/* Already in a non tty mode */
> +	if (dlci->adaption > 2)
> +		return -EBUSY;

Only you don't have any locking so two ioctls at once will get very
confused


> +	if (retval) {
> +		pr_err("network register fail %d\n", retval);
> +		free_netdev(net);
> +		goto error_ret;

And this leaves the DLCI messed up

> +	kref_init(&mux_net->ref);

What stops this getting referenced between the net register and here ?

On the destroy side you don't seem to put back the old adaption settings
and restore the state (and also have locking v another ioctl missing)

Doesn't look too hard to fix - save the old dlci states when you go
network, put them back when you destroy the network state. For the ioctls
you probably need to cover most of each one with the mutex.

RE: [PATCH 1/5] tty: n_gsm: Add raw-ip support

[Gorby, Russ]

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
>Sent: Monday, June 06, 2011 2:29 AM
>To: Gorby, Russ
>Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; Ahmed, Suhail
>Subject: Re: [PATCH 1/5] tty: n_gsm: Add raw-ip support
>
>
>> @@ -1590,6 +1616,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct
>gsm_mux *gsm, int addr)
>>  	dlci->addr = addr;
>>  	dlci->adaption = gsm->adaption;
>>  	dlci->state = DLCI_CLOSED;
>> +	dlci->net = NULL;	/* network not initially created */
>
>kzalloc ised used anyway so this starts NULL
 
[Gorby, Russ] sorry, inherited code - I'll fix it.

>
>>
>> +int gsm_change_mtu(struct net_device *net, int new_mtu)
>> +{
>> +	if ((new_mtu < 8) || (new_mtu > MAX_MTU))
>> +		return -EINVAL;
>> +	net->mtu = new_mtu;
>> +	return 0;
>> +}
>
>Surely at that point you need to renegotiate the DLCI parameters for the
>DLCI in question. At the very least you need to sanity check versus the
>current settings ?

[Gorby, Russ] Thanks, I see we are misusing MAX_MTU, should be gsm->mtu instead

>
>
>> +static int gsm_create_network(struct gsm_dlci *dlci, struct
>gsm_netconfig *nc)
>> +{
>> +	char *netname;
>> +	int retval = 0;
>> +	struct net_device *net;
>> +	struct gsm_mux_net *mux_net;
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
>> +
>> +	/* Already in a non tty mode */
>> +	if (dlci->adaption > 2)
>> +		return -EBUSY;
>
>Only you don't have any locking so two ioctls at once will get very
>confused

 [Gorby, Russ] This is an ioctl against a gsmttyN  TTY to turn it into a network IF.
	The same ioctl on different nodes can proceed in parallel w/o problems (that I see). So the exclusion
	you're concerned with is from multiple clients of the TTY issuing this same ioctl at the same time?
	This would indicate a per-dlci exclusion needed then.

>
>
>> +	if (retval) {
>> +		pr_err("network register fail %d\n", retval);
>> +		free_netdev(net);
>> +		goto error_ret;
>
>And this leaves the DLCI messed up

[Gorby, Russ] how so? We don't modify the dlci until after this. We've just copied the mtu at this point.

>
>> +	kref_init(&mux_net->ref);
>
>What stops this getting referenced between the net register and here ?

[Gorby, Russ] OOPs - will fix

>
>On the destroy side you don't seem to put back the old adaption settings
>and restore the state (and also have locking v another ioctl missing)
>
>Doesn't look too hard to fix - save the old dlci states when you go
>network, put them back when you destroy the network state. For the
>ioctls
>you probably need to cover most of each one with the mutex.

 [Gorby, Russ] Thanks for the comments. I appreciate all the help.

[PATCH 2/5] tty: n_gsm: expose gsmtty device nodes at ldisc open time

[Russ Gorby]

The driver being an ldisc, does not provide a convenient method
e.g. udev to create the tty device nodes automatically when the ldisc
is opened.

The TTY device nodes are npw created via calls to tty_register_device

Signed-off-by: Russ Gorby <russ.gorby@intel.com>
---
 drivers/tty/n_gsm.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 47897c2..0a2a5da 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -190,6 +190,7 @@ struct gsm_control {
 struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
+	unsigned int num;
 
 	/* Events on the GSM channel */
 	wait_queue_head_t event;
@@ -271,6 +272,8 @@ struct gsm_mux {
 static struct gsm_mux *gsm_mux[MAX_MUX];	/* GSM muxes */
 static spinlock_t gsm_mux_lock;
 
+static struct tty_driver *gsm_tty_driver;
+
 /*
  *	This section of the driver logic implements the GSM encodings
  *	both the basic and the 'advanced'. Reliable transport is not
@@ -2023,6 +2026,7 @@ int gsm_activate_mux(struct gsm_mux *gsm)
 	spin_lock(&gsm_mux_lock);
 	for (i = 0; i < MAX_MUX; i++) {
 		if (gsm_mux[i] == NULL) {
+			gsm->num = i;
 			gsm_mux[i] = gsm;
 			break;
 		}
@@ -2128,13 +2132,20 @@ static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len)
 
 static int gsmld_attach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
 {
-	int ret;
+	int ret, i;
+	int base = gsm->num << 6; /* Base for this MUX */
 
 	gsm->tty = tty_kref_get(tty);
 	gsm->output = gsmld_output;
 	ret =  gsm_activate_mux(gsm);
 	if (ret != 0)
 		tty_kref_put(gsm->tty);
+	else {
+		/* Don't register device 0 - this is the control channel and not
+		   a usable tty interface */
+		for (i = 1; i < NUM_DLCI; i++)
+			tty_register_device(gsm_tty_driver, base + i, NULL);
+	}
 	return ret;
 }
 
@@ -2149,7 +2160,12 @@ static int gsmld_attach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
 
 static void gsmld_detach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
 {
+	int i;
+	int base = gsm->num << 6; /* Base for this MUX */
+
 	WARN_ON(tty != gsm->tty);
+	for (i = 1; i < NUM_DLCI; i++)
+		tty_unregister_device(gsm_tty_driver, base + i);
 	gsm_cleanup_mux(gsm);
 	tty_kref_put(gsm->tty);
 	gsm->tty = NULL;
@@ -2958,7 +2974,6 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 	return gsmtty_modem_update(dlci, encode);
 }
 
-static struct tty_driver *gsm_tty_driver;
 
 /* Virtual ttys for the demux */
 static const struct tty_operations gsmtty_ops = {
-- 
1.7.0.4

Re: [PATCH 2/5] tty: n_gsm: expose gsmtty device nodes at ldisc open time

[Alan Cox]

On Fri,  3 Jun 2011 12:03:38 -0700
Russ Gorby <russ.gorby@intel.com> wrote:

> The driver being an ldisc, does not provide a convenient method
> e.g. udev to create the tty device nodes automatically when the ldisc
> is opened.
> 
> The TTY device nodes are npw created via calls to tty_register_device
> 
> Signed-off-by: Russ Gorby <russ.gorby@intel.com>

This stands alone and wants applying regardless

Acked-by: Alan Cox <alan@linux.intel.com>

[PATCH 3/5] tty: n_gsm: Added refcount usage to gsm_mux and gsm_dlci structs

[Russ Gorby]

The gsm_mux is created/destroyed when ldisc is
opened/closed but clients of the MUX channel devices (gsmttyN)
may access this structure as long as the TTYs are open.
For the open, the ldisc open is guaranteed to preceed the TTY open,
but the close has no such guaranteed ordering. As a result,
the gsm_mux can be freed in the ldisc close before being accessed
by one of the TTY clients. This can happen if the ldisc is removed
while there are open, active MUX channels.
A similar situation exists for DLCI-0, it is basically a resource
shared by MUX and DLCI  , and should not be freed while they can
be accessed

To avoid this, gsm_mux and dlcis now have a reference counter
ldisc open takes a reference on the mux and all the dlcis
gsmtty_open takes a reference on the mux, dlci0 and its specific
dlci. Dropping the last reference initiates the actual free.

Signed-off-by: Russ Gorby <russ.gorby@intel.com>
---
 drivers/tty/n_gsm.c |   83 ++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 0a2a5da..c8a43de 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -133,6 +133,7 @@ struct gsm_dlci {
 #define DLCI_OPENING		1	/* Sending SABM not seen UA */
 #define DLCI_OPEN		2	/* SABM/UA complete */
 #define DLCI_CLOSING		3	/* Sending DISC not seen UA/DM */
+	struct kref ref;		/* freed from port or mux close */
 
 	/* Link layer */
 	spinlock_t lock;	/* Protects the internal state */
@@ -191,6 +192,7 @@ struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	unsigned int num;
+	struct kref ref;
 
 	/* Events on the GSM channel */
 	wait_queue_head_t event;
@@ -1603,6 +1605,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 	if (dlci == NULL)
 		return NULL;
 	spin_lock_init(&dlci->lock);
+	kref_init(&dlci->ref);
 	dlci->fifo = &dlci->_fifo;
 	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
 		kfree(dlci);
@@ -1629,26 +1632,42 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 }
 
 /**
- *	gsm_dlci_free		-	release DLCI
+ *	gsm_dlci_free		-	free DLCI
+ *	@dlci: DLCI to free
+ *
+ *	Free up a DLCI.
+ *
+ *	Can sleep.
+ */
+static void gsm_dlci_free(struct kref *ref)
+{
+	struct gsm_dlci *dlci = container_of(ref, struct gsm_dlci, ref);
+
+	del_timer_sync(&dlci->t1);
+	dlci->gsm->dlci[dlci->addr] = NULL;
+	kfifo_free(dlci->fifo);
+	while ((dlci->skb = skb_dequeue(&dlci->skb_list)))
+		kfree_skb(dlci->skb);
+	kfree(dlci);
+}
+
+/**
+ *	gsm_dlci_release		-	release DLCI
  *	@dlci: DLCI to destroy
  *
- *	Free up a DLCI. Currently to keep the lifetime rules sane we only
- *	clean up DLCI objects when the MUX closes rather than as the port
- *	is closed down on both the tty and mux levels.
+ *	Release a DLCI. Actual free is deferred until either
+ *	mux is closed or tty is closed - whichever is last.
  *
  *	Can sleep.
  */
-static void gsm_dlci_free(struct gsm_dlci *dlci)
+static void gsm_dlci_release(struct gsm_dlci *dlci)
 {
 	struct tty_struct *tty = tty_port_tty_get(&dlci->port);
 	if (tty) {
 		tty_vhangup(tty);
 		tty_kref_put(tty);
 	}
-	del_timer_sync(&dlci->t1);
-	dlci->gsm->dlci[dlci->addr] = NULL;
-	kfifo_free(dlci->fifo);
-	kfree(dlci);
+	kref_put(&dlci->ref, gsm_dlci_free);
 }
 
 /*
@@ -1986,7 +2005,7 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
 	/* Free up any link layer users */
 	for (i = 0; i < NUM_DLCI; i++)
 		if (gsm->dlci[i])
-			gsm_dlci_free(gsm->dlci[i]);
+			gsm_dlci_release(gsm->dlci[i]);
 	/* Now wipe the queues */
 	for (txq = gsm->tx_head; txq != NULL; txq = gsm->tx_head) {
 		gsm->tx_head = txq->next;
@@ -2047,8 +2066,7 @@ EXPORT_SYMBOL_GPL(gsm_activate_mux);
  *	gsm_free_mux		-	free up a mux
  *	@mux: mux to free
  *
- *	Dispose of allocated resources for a dead mux. No refcounting
- *	at present so the mux must be truly dead.
+ *	Dispose of allocated resources for a dead mux
  */
 void gsm_free_mux(struct gsm_mux *gsm)
 {
@@ -2059,6 +2077,30 @@ void gsm_free_mux(struct gsm_mux *gsm)
 EXPORT_SYMBOL_GPL(gsm_free_mux);
 
 /**
+ *	gsm_free_muxr		-	free up a mux
+ *	@mux: mux to free
+ *
+ *	Dispose of allocated resources for a dead mux
+ */
+static void gsm_free_muxr(struct kref *ref)
+{
+	struct gsm_mux *gsm = container_of(ref, struct gsm_mux, ref);
+	gsm_free_mux(gsm);
+}
+
+/**
+ *	gsm_release_mux		-	release a mux
+ *	@mux: mux to release
+ *
+ *	Dispose of allocated resources for a dead mux on release
+ *	from last client.
+ */
+static void gsm_release_mux(struct gsm_mux *gsm)
+{
+	kref_put(&gsm->ref, gsm_free_muxr);
+}
+
+/**
  *	gsm_alloc_mux		-	allocate a mux
  *
  *	Creates a new mux ready for activation.
@@ -2081,6 +2123,7 @@ struct gsm_mux *gsm_alloc_mux(void)
 		return NULL;
 	}
 	spin_lock_init(&gsm->lock);
+	kref_init(&gsm->ref);
 
 	gsm->t1 = T1;
 	gsm->t2 = T2;
@@ -2255,7 +2298,7 @@ static void gsmld_close(struct tty_struct *tty)
 
 	gsmld_flush_buffer(tty);
 	/* Do other clean up here */
-	gsm_free_mux(gsm);
+	gsm_release_mux(gsm);
 }
 
 /**
@@ -2805,6 +2848,9 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 	port = &dlci->port;
 	port->count++;
 	tty->driver_data = dlci;
+	kref_get(&dlci->ref);
+	kref_get(&dlci->gsm->dlci[0]->ref);
+	kref_get(&dlci->gsm->ref);
 	tty_port_tty_set(port, tty);
 
 	dlci->modem_rx = 0;
@@ -2820,14 +2866,23 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	struct gsm_mux *gsm;
+
 	if (dlci == NULL)
 		return;
+	gsm = dlci->gsm;
 	gsm_destroy_network(dlci);
-	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
+	if (tty_port_close_start(&dlci->port, tty, filp) == 0) {
+		kref_put(&dlci->ref, gsm_dlci_free);
+		gsm_release_mux(gsm);
 		return;
+	}
 	gsm_dlci_begin_close(dlci);
 	tty_port_close_end(&dlci->port, tty);
 	tty_port_tty_set(&dlci->port, NULL);
+	kref_put(&dlci->ref, gsm_dlci_free);
+	kref_put(&gsm->dlci[0]->ref, gsm_dlci_free);
+	gsm_release_mux(gsm);
 }
 
 static void gsmtty_hangup(struct tty_struct *tty)
-- 
1.7.0.4

Re: [PATCH 3/5] tty: n_gsm: Added refcount usage to gsm_mux and gsm_dlci structs

[Alan Cox]

> To avoid this, gsm_mux and dlcis now have a reference counter
> ldisc open takes a reference on the mux and all the dlcis
> gsmtty_open takes a reference on the mux, dlci0 and its specific
> dlci. Dropping the last reference initiates the actual free.

Definitely the right direction. Only comment really is style related

We usualyl define

struct foo *foo_get(struct foo *)
void foo_put(struct foo *)

helpers/inlines just make it clearer and keep the knowledge of the
function pointer etc in one place

Also when you take a reference this lets you write the more natural
reading

	bar->foo = foo_get(foo);


Alan

[PATCH 4/5] tty: n_gsm: initiate close of all DLCIs during mux shutdown

[Russ Gorby]

Although DLCI-0 is closed during ldisc close, we found the
applications were better served when open DLCI shutdowns where
initiated at that time as well.

initiated shutdown of all DLCI during ldisc close.

Signed-off-by: Russ Gorby <russ.gorby@intel.com>
---
 drivers/tty/n_gsm.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c8a43de..22c844d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -43,6 +43,8 @@
 #include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/tty.h>
+#include <linux/timer.h>
+#include <linux/wait.h>
 #include <linux/ctype.h>
 #include <linux/mm.h>
 #include <linux/string.h>
@@ -1979,6 +1981,7 @@ static void gsm_error(struct gsm_mux *gsm,
 void gsm_cleanup_mux(struct gsm_mux *gsm)
 {
 	int i;
+	int t;
 	struct gsm_dlci *dlci = gsm->dlci[0];
 	struct gsm_msg *txq;
 
@@ -1996,14 +1999,27 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
 
 	del_timer_sync(&gsm->t2_timer);
 	/* Now we are sure T2 has stopped */
-	if (dlci) {
-		dlci->dead = 1;
-		gsm_dlci_begin_close(dlci);
-		wait_event_interruptible(gsm->event,
-					dlci->state == DLCI_CLOSED);
-	}
 	/* Free up any link layer users */
-	for (i = 0; i < NUM_DLCI; i++)
+	for (i = NUM_DLCI-1; i >= 0; i--) {
+		dlci = gsm->dlci[i];
+		if (dlci) {
+			if (i != 0)
+				gsm_dlci_begin_close(dlci);
+			else {
+				dlci->dead = 1;
+				gsm_dlci_begin_close(dlci);
+				t = wait_event_timeout(gsm->event,
+					   dlci->state == DLCI_CLOSED,
+					   gsm->t2 * HZ / 100);
+				if (!t) {
+					pr_info("%s: timeout dlci0 close",
+						__func__);
+					gsm_dlci_close(dlci);
+				}
+			}
+		}
+	}
+	for (i = NUM_DLCI-1; i >= 0; i--)
 		if (gsm->dlci[i])
 			gsm_dlci_release(gsm->dlci[i]);
 	/* Now wipe the queues */
-- 
1.7.0.4

Re: [PATCH 4/5] tty: n_gsm: initiate close of all DLCIs during mux shutdown

[Alan Cox]

On Fri,  3 Jun 2011 12:03:40 -0700
Russ Gorby <russ.gorby@intel.com> wrote:

> Although DLCI-0 is closed during ldisc close, we found the
> applications were better served when open DLCI shutdowns where
> initiated at that time as well.

They should all see a hangup on the gsm client tty anyway ?

> -	for (i = 0; i < NUM_DLCI; i++)
> +	for (i = NUM_DLCI-1; i >= 0; i--) {
> +		dlci = gsm->dlci[i];
> +		if (dlci) {
> +			if (i != 0)
> +				gsm_dlci_begin_close(dlci);
> +			else {
> +				dlci->dead = 1;
> +				gsm_dlci_begin_close(dlci);
> +				t = wait_event_timeout(gsm->event,
> +					   dlci->state == DLCI_CLOSED,
> +					   gsm->t2 * HZ / 100);
> +				if (!t) {
> +					pr_info("%s: timeout dlci0 close",
> +						__func__);
> +					gsm_dlci_close(dlci);
> +				}
> +			}
> +		}
> +	}
> +	for (i = NUM_DLCI-1; i >= 0; i--)

I'd like to understand better why it is needed and also why you don't
want for the others to close but just set it off ?

RE: [PATCH 4/5] tty: n_gsm: initiate close of all DLCIs during mux shutdown

[Gorby, Russ]

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
>Sent: Friday, June 03, 2011 3:05 PM
>To: Gorby, Russ
>Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; Ahmed, Suhail
>Subject: Re: [PATCH 4/5] tty: n_gsm: initiate close of all DLCIs during
>mux shutdown
>
>On Fri,  3 Jun 2011 12:03:40 -0700
>Russ Gorby <russ.gorby@intel.com> wrote:
>
>> Although DLCI-0 is closed during ldisc close, we found the
>> applications were better served when open DLCI shutdowns where
>> initiated at that time as well.
>
>They should all see a hangup on the gsm client tty anyway ?
>
>> -	for (i = 0; i < NUM_DLCI; i++)
>> +	for (i = NUM_DLCI-1; i >= 0; i--) {
>> +		dlci = gsm->dlci[i];
>> +		if (dlci) {
>> +			if (i != 0)
>> +				gsm_dlci_begin_close(dlci);
>> +			else {
>> +				dlci->dead = 1;
>> +				gsm_dlci_begin_close(dlci);
>> +				t = wait_event_timeout(gsm->event,
>> +					   dlci->state == DLCI_CLOSED,
>> +					   gsm->t2 * HZ / 100);
>> +				if (!t) {
>> +					pr_info("%s: timeout dlci0 close",
>> +						__func__);
>> +					gsm_dlci_close(dlci);
>> +				}
>> +			}
>> +		}
>> +	}
>> +	for (i = NUM_DLCI-1; i >= 0; i--)
>
>I'd like to understand better why it is needed and also why you don't
>want for the others to close but just set it off ?

[Gorby, Russ] I suppose it is possible the issue this code helped alleviate was due to modem firmware or
Other s/w bugs or timing issues, but since we're waiting for DLCI0 to close it made sense to at least start the
Close of the other channels first. If you don't want to take this patch at this point, I can revalidate if it is needed
at a later time and resubmit then if so.

[PATCH 5/5] tty: n_gsm: Fixed NULL ptr OOPs in tty_write_room()

[Russ Gorby]

We saw a case where gsmld_output was called after the MUX
was shutdown. In this case gsm->tty was null so tty_write_room(NULL)
was called which resulted in an exception.

checked for gsm->tty == NULL in gsmld_output()

Signed-off-by: Russ Gorby <russ.gorby@intel.com>
---
 drivers/tty/n_gsm.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 22c844d..2908199 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2168,6 +2168,8 @@ EXPORT_SYMBOL_GPL(gsm_alloc_mux);
 
 static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len)
 {
+	if (!gsm->tty)
+		return -ENXIO;
 	if (tty_write_room(gsm->tty) < len) {
 		set_bit(TTY_DO_WRITE_WAKEUP, &gsm->tty->flags);
 		return -ENOSPC;
-- 
1.7.0.4

Re: [PATCH 5/5] tty: n_gsm: Fixed NULL ptr OOPs in tty_write_room()

[Alan Cox]

On Fri,  3 Jun 2011 12:03:41 -0700
Russ Gorby <russ.gorby@intel.com> wrote:

> We saw a case where gsmld_output was called after the MUX
> was shutdown. In this case gsm->tty was null so tty_write_room(NULL)
> was called which resulted in an exception.

This is papering over a problem. It shouldn't be happening. Do you have a
copy of the backtrace of the event and does it occur a lot. If it's a one
off freak event so far then instead add

	if (gsm->tty == NULL) {
		WARN_ON(1);
		return -ENXIO;
	}

so we get dumps from it and the box survives when it occurs. That'll get
us traces to nail it properly. For one your patch is not a valid
ultimate fix because if it can occur what stops gsm->tty changing as it
occurs.

RE: [PATCH 5/5] tty: n_gsm: Fixed NULL ptr OOPs in tty_write_room()

[Gorby, Russ]

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
>Sent: Friday, June 03, 2011 3:10 PM
>To: Gorby, Russ
>Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; Ahmed, Suhail
>Subject: Re: [PATCH 5/5] tty: n_gsm: Fixed NULL ptr OOPs in
>tty_write_room()
>
>On Fri,  3 Jun 2011 12:03:41 -0700
>Russ Gorby <russ.gorby@intel.com> wrote:
>
>> We saw a case where gsmld_output was called after the MUX
>> was shutdown. In this case gsm->tty was null so tty_write_room(NULL)
>> was called which resulted in an exception.
>
>This is papering over a problem. It shouldn't be happening. Do you have
>a
>copy of the backtrace of the event and does it occur a lot. If it's a
>one
>off freak event so far then instead add
>
>	if (gsm->tty == NULL) {
>		WARN_ON(1);
>		return -ENXIO;
>	}
>
>so we get dumps from it and the box survives when it occurs. That'll get
>us traces to nail it properly. For one your patch is not a valid
>ultimate fix because if it can occur what stops gsm->tty changing as it
>occurs.

[Gorby, Russ] Fair enough - no I don't have a trace and it only happened once. We're also running on the .36 kernel that I've seen some other TTY-weirdness with. I'll add the WARN_ON locally and try to capture another trace for analysis.