[PATCH net] Phonet: hold GPRS device while cleaning up

[PATCH net] Phonet: hold GPRS device while cleaning up

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/pep-gprs.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/phonet/pep-gprs.c b/net/phonet/pep-gprs.c
index 9978afb..9602bfc 100644
--- a/net/phonet/pep-gprs.c
+++ b/net/phonet/pep-gprs.c
@@ -340,8 +340,10 @@ void gprs_detach(struct sock *sk)
 	release_sock(sk);
 
 	printk(KERN_DEBUG"%s: detached\n", net->name);
+	dev_hold(net); /* TX work still needs it */
 	unregister_netdev(net);
 	flush_scheduled_work();
+	dev_put(net);
 	sock_put(sk);
 	skb_queue_purge(&dev->tx_queue);
 }
-- 
1.5.4.3

Re: [PATCH net] Phonet: hold GPRS device while cleaning up

[David Miller]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Mon,  8 Dec 2008 12:09:25 +0200

This is just a side-comment about the phonet stack, not directly
related to this patch.

> diff --git a/net/phonet/pep-gprs.c b/net/phonet/pep-gprs.c
> index 9978afb..9602bfc 100644
> --- a/net/phonet/pep-gprs.c
> +++ b/net/phonet/pep-gprs.c
> @@ -340,8 +340,10 @@ void gprs_detach(struct sock *sk)
>  	release_sock(sk);
>  
>  	printk(KERN_DEBUG"%s: detached\n", net->name);
> +	dev_hold(net); /* TX work still needs it */
>  	unregister_netdev(net);
>  	flush_scheduled_work();
> +	dev_put(net);
>  	sock_put(sk);
>  	skb_queue_purge(&dev->tx_queue);
>  }

Can we get the phonet stack using more sensible local
variable names for netdev pointers?

We have network namespace objects, and everything says
"net" as the variable name to hold pointers to such
objects.

Using the same local variable name for "struct netdev"
pointers breeds confusion and makes phonet patches and
code difficult to read for people familiar with the
rest of the Linux networking.

Since you use "dev", which is the typical way to name
netdev pointers in networking code, for the phonet et al.
private stuff, one thing you can do is adopt the technique
I usually use:

1) Driver/subsystem private objects get a two letter local
   variable name, the second letter is 'p' for pointer.
   So a TG3 driver private pointer variable is "tp".  You'll
   see this in pretty much every single driver I ever wrote.

2) struct netdev pointer variables are "dev"

3) Network namespace pointer variables are "net"

With this we'll achieve consistency and much easier to read
and validate patches.

Just a suggestion :)

Re: [PATCH net] Phonet: hold GPRS device while cleaning up

[Rémi Denis-Courmont]

On Monday 08 December 2008 12:21:53 ext David Miller, you wrote:
> Can we get the phonet stack using more sensible local
> variable names for netdev pointers?

Sure. Should I send a large cosmetic s/net/dev/ et al patch against net-next?

-- 
Rémi Denis-Courmont
Maemo Software, Nokia Devices R&D

Re: [PATCH net] Phonet: hold GPRS device while cleaning up

[David Miller]

From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia.com>
Date: Mon, 8 Dec 2008 13:03:55 +0200

> On Monday 08 December 2008 12:21:53 ext David Miller, you wrote:
> > Can we get the phonet stack using more sensible local
> > variable names for netdev pointers?
> 
> Sure. Should I send a large cosmetic s/net/dev/ et al patch against net-next?

That would work just fine, thanks.

[PATCH] Phonet: improve GPRS variable names

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/pep-gprs.c |  170 +++++++++++++++++++++++++------------------------
 1 files changed, 86 insertions(+), 84 deletions(-)

diff --git a/net/phonet/pep-gprs.c b/net/phonet/pep-gprs.c
index a24d01e..7461a61 100644
--- a/net/phonet/pep-gprs.c
+++ b/net/phonet/pep-gprs.c
@@ -40,7 +40,7 @@ struct gprs_dev {
 	void			(*old_data_ready)(struct sock *, int);
 	void			(*old_write_space)(struct sock *);
 
-	struct net_device	*net;
+	struct net_device	*dev;
 
 	struct sk_buff_head	tx_queue;
 	struct work_struct	tx_work;
@@ -72,17 +72,19 @@ static __be16 gprs_type_trans(struct sk_buff *skb)
 
 static void gprs_state_change(struct sock *sk)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
+	struct gprs_dev *gp = sk->sk_user_data;
 
 	if (sk->sk_state == TCP_CLOSE_WAIT) {
-		netif_stop_queue(dev->net);
-		netif_carrier_off(dev->net);
+		struct net_device *dev = gp->dev;
+
+		netif_stop_queue(dev);
+		netif_carrier_off(dev);
 	}
 }
 
-static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
+static int gprs_recv(struct gprs_dev *gp, struct sk_buff *skb)
 {
-	struct net_device *net = dev->net;
+	struct net_device *dev = gp->dev;
 	int err = 0;
 	__be16 protocol = gprs_type_trans(skb);
 
@@ -99,7 +101,7 @@ static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
 		 * so wrap the IP packet as a single fragment of an head-less
 		 * socket buffer. The network stack will pull what it needs,
 		 * but at least, the whole IP payload is not memcpy'd. */
-		rskb = netdev_alloc_skb(net, 0);
+		rskb = netdev_alloc_skb(dev, 0);
 		if (!rskb) {
 			err = -ENOBUFS;
 			goto drop;
@@ -123,11 +125,11 @@ static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
 
 	skb->protocol = protocol;
 	skb_reset_mac_header(skb);
-	skb->dev = net;
+	skb->dev = dev;
 
-	if (likely(net->flags & IFF_UP)) {
-		net->stats.rx_packets++;
-		net->stats.rx_bytes += skb->len;
+	if (likely(dev->flags & IFF_UP)) {
+		dev->stats.rx_packets++;
+		dev->stats.rx_bytes += skb->len;
 		netif_rx(skb);
 		skb = NULL;
 	} else
@@ -136,41 +138,41 @@ static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
 drop:
 	if (skb) {
 		dev_kfree_skb(skb);
-		net->stats.rx_dropped++;
+		dev->stats.rx_dropped++;
 	}
 	return err;
 }
 
 static void gprs_data_ready(struct sock *sk, int len)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
+	struct gprs_dev *gp = sk->sk_user_data;
 	struct sk_buff *skb;
 
 	while ((skb = pep_read(sk)) != NULL) {
 		skb_orphan(skb);
-		gprs_recv(dev, skb);
+		gprs_recv(gp, skb);
 	}
 }
 
 static void gprs_write_space(struct sock *sk)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
+	struct gprs_dev *gp = sk->sk_user_data;
 	unsigned credits = pep_writeable(sk);
 
-	spin_lock_bh(&dev->tx_lock);
-	dev->tx_max = credits;
-	if (credits > skb_queue_len(&dev->tx_queue))
-		netif_wake_queue(dev->net);
-	spin_unlock_bh(&dev->tx_lock);
+	spin_lock_bh(&gp->tx_lock);
+	gp->tx_max = credits;
+	if (credits > skb_queue_len(&gp->tx_queue))
+		netif_wake_queue(gp->dev);
+	spin_unlock_bh(&gp->tx_lock);
 }
 
 /*
  * Network device callbacks
  */
 
-static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
+static int gprs_xmit(struct sk_buff *skb, struct net_device *dev)
 {
-	struct gprs_dev *dev = netdev_priv(net);
+	struct gprs_dev *gp = netdev_priv(dev);
 
 	switch (skb->protocol) {
 	case  htons(ETH_P_IP):
@@ -181,16 +183,16 @@ static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
 		return 0;
 	}
 
-	spin_lock(&dev->tx_lock);
-	if (likely(skb_queue_len(&dev->tx_queue) < dev->tx_max)) {
-		skb_queue_tail(&dev->tx_queue, skb);
+	spin_lock(&gp->tx_lock);
+	if (likely(skb_queue_len(&gp->tx_queue) < gp->tx_max)) {
+		skb_queue_tail(&gp->tx_queue, skb);
 		skb = NULL;
 	}
-	if (skb_queue_len(&dev->tx_queue) >= dev->tx_max)
-		netif_stop_queue(net);
-	spin_unlock(&dev->tx_lock);
+	if (skb_queue_len(&gp->tx_queue) >= gp->tx_max)
+		netif_stop_queue(dev);
+	spin_unlock(&gp->tx_lock);
 
-	schedule_work(&dev->tx_work);
+	schedule_work(&gp->tx_work);
 	if (unlikely(skb))
 		dev_kfree_skb(skb);
 	return 0;
@@ -198,16 +200,16 @@ static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
 
 static void gprs_tx(struct work_struct *work)
 {
-	struct gprs_dev *dev = container_of(work, struct gprs_dev, tx_work);
-	struct net_device *net = dev->net;
-	struct sock *sk = dev->sk;
+	struct gprs_dev *gp = container_of(work, struct gprs_dev, tx_work);
+	struct net_device *dev = gp->dev;
+	struct sock *sk = gp->sk;
 	struct sk_buff *skb;
 
-	while ((skb = skb_dequeue(&dev->tx_queue)) != NULL) {
+	while ((skb = skb_dequeue(&gp->tx_queue)) != NULL) {
 		int err;
 
-		net->stats.tx_bytes += skb->len;
-		net->stats.tx_packets++;
+		dev->stats.tx_bytes += skb->len;
+		dev->stats.tx_packets++;
 
 		skb_orphan(skb);
 		skb_set_owner_w(skb, sk);
@@ -216,9 +218,9 @@ static void gprs_tx(struct work_struct *work)
 		err = pep_write(sk, skb);
 		if (err) {
 			LIMIT_NETDEBUG(KERN_WARNING"%s: TX error (%d)\n",
-					net->name, err);
-			net->stats.tx_aborted_errors++;
-			net->stats.tx_errors++;
+					dev->name, err);
+			dev->stats.tx_aborted_errors++;
+			dev->stats.tx_errors++;
 		}
 		release_sock(sk);
 	}
@@ -228,28 +230,28 @@ static void gprs_tx(struct work_struct *work)
 	release_sock(sk);
 }
 
-static int gprs_set_mtu(struct net_device *net, int new_mtu)
+static int gprs_set_mtu(struct net_device *dev, int new_mtu)
 {
 	if ((new_mtu < 576) || (new_mtu > (PHONET_MAX_MTU - 11)))
 		return -EINVAL;
 
-	net->mtu = new_mtu;
+	dev->mtu = new_mtu;
 	return 0;
 }
 
-static void gprs_setup(struct net_device *net)
+static void gprs_setup(struct net_device *dev)
 {
-	net->features		= NETIF_F_FRAGLIST;
-	net->type		= ARPHRD_NONE;
-	net->flags		= IFF_POINTOPOINT | IFF_NOARP;
-	net->mtu		= GPRS_DEFAULT_MTU;
-	net->hard_header_len	= 0;
-	net->addr_len		= 0;
-	net->tx_queue_len	= 10;
-
-	net->destructor		= free_netdev;
-	net->hard_start_xmit	= gprs_xmit; /* mandatory */
-	net->change_mtu		= gprs_set_mtu;
+	dev->features		= NETIF_F_FRAGLIST;
+	dev->type		= ARPHRD_NONE;
+	dev->flags		= IFF_POINTOPOINT | IFF_NOARP;
+	dev->mtu		= GPRS_DEFAULT_MTU;
+	dev->hard_header_len	= 0;
+	dev->addr_len		= 0;
+	dev->tx_queue_len	= 10;
+
+	dev->destructor		= free_netdev;
+	dev->hard_start_xmit	= gprs_xmit; /* mandatory */
+	dev->change_mtu		= gprs_set_mtu;
 }
 
 /*
@@ -263,28 +265,28 @@ static void gprs_setup(struct net_device *net)
 int gprs_attach(struct sock *sk)
 {
 	static const char ifname[] = "gprs%d";
-	struct gprs_dev *dev;
-	struct net_device *net;
+	struct gprs_dev *gp;
+	struct net_device *dev;
 	int err;
 
 	if (unlikely(sk->sk_type == SOCK_STREAM))
 		return -EINVAL; /* need packet boundaries */
 
 	/* Create net device */
-	net = alloc_netdev(sizeof(*dev), ifname, gprs_setup);
-	if (!net)
+	dev = alloc_netdev(sizeof(*gp), ifname, gprs_setup);
+	if (!dev)
 		return -ENOMEM;
-	dev = netdev_priv(net);
-	dev->net = net;
-	dev->tx_max = 0;
-	spin_lock_init(&dev->tx_lock);
-	skb_queue_head_init(&dev->tx_queue);
-	INIT_WORK(&dev->tx_work, gprs_tx);
-
-	netif_stop_queue(net);
-	err = register_netdev(net);
+	gp = netdev_priv(dev);
+	gp->dev = dev;
+	gp->tx_max = 0;
+	spin_lock_init(&gp->tx_lock);
+	skb_queue_head_init(&gp->tx_queue);
+	INIT_WORK(&gp->tx_work, gprs_tx);
+
+	netif_stop_queue(dev);
+	err = register_netdev(dev);
 	if (err) {
-		free_netdev(net);
+		free_netdev(dev);
 		return err;
 	}
 
@@ -298,45 +300,45 @@ int gprs_attach(struct sock *sk)
 		err = -EINVAL;
 		goto out_rel;
 	}
-	sk->sk_user_data	= dev;
-	dev->old_state_change	= sk->sk_state_change;
-	dev->old_data_ready	= sk->sk_data_ready;
-	dev->old_write_space	= sk->sk_write_space;
+	sk->sk_user_data	= gp;
+	gp->old_state_change	= sk->sk_state_change;
+	gp->old_data_ready	= sk->sk_data_ready;
+	gp->old_write_space	= sk->sk_write_space;
 	sk->sk_state_change	= gprs_state_change;
 	sk->sk_data_ready	= gprs_data_ready;
 	sk->sk_write_space	= gprs_write_space;
 	release_sock(sk);
 
 	sock_hold(sk);
-	dev->sk = sk;
+	gp->sk = sk;
 
-	printk(KERN_DEBUG"%s: attached\n", net->name);
+	printk(KERN_DEBUG"%s: attached\n", dev->name);
 	gprs_write_space(sk); /* kick off TX */
-	return net->ifindex;
+	return dev->ifindex;
 
 out_rel:
 	release_sock(sk);
-	unregister_netdev(net);
+	unregister_netdev(dev);
 	return err;
 }
 
 void gprs_detach(struct sock *sk)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
-	struct net_device *net = dev->net;
+	struct gprs_dev *gp = sk->sk_user_data;
+	struct net_device *dev = gp->dev;
 
 	lock_sock(sk);
 	sk->sk_user_data	= NULL;
-	sk->sk_state_change	= dev->old_state_change;
-	sk->sk_data_ready	= dev->old_data_ready;
-	sk->sk_write_space	= dev->old_write_space;
+	sk->sk_state_change	= gp->old_state_change;
+	sk->sk_data_ready	= gp->old_data_ready;
+	sk->sk_write_space	= gp->old_write_space;
 	release_sock(sk);
 
-	printk(KERN_DEBUG"%s: detached\n", net->name);
-	dev_hold(net); /* TX work still needs it */
-	unregister_netdev(net);
+	printk(KERN_DEBUG"%s: detached\n", dev->name);
+	dev_hold(dev); /* TX work still needs it */
+	unregister_netdev(dev);
 	flush_scheduled_work();
-	dev_put(net);
+	dev_put(dev);
 	sock_put(sk);
-	skb_queue_purge(&dev->tx_queue);
+	skb_queue_purge(&gp->tx_queue);
 }
-- 
1.5.4.3

Re: [PATCH] Phonet: improve GPRS variable names

[Rémi Denis-Courmont]

That one depends on the first patch though.

-- 
Rémi Denis-Courmont
Maemo Software, Nokia Devices R&D

Re: [PATCH] Phonet: improve GPRS variable names

[David Miller]

From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia.com>
Date: Tue, 9 Dec 2008 09:51:16 +0200

> That one depends on the first patch though.

No problem, thanks for mentioning.

Re: [PATCH] Phonet: improve GPRS variable names

[David Miller]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Tue,  9 Dec 2008 09:45:59 +0200

> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Since this depends upon the buggy refcount change, I'm
tossing this and will wait for an updated version once
everything is sorted out.

Re: [PATCH] Phonet: improve GPRS variable names

[Rémi Denis-Courmont]

On Wednesday 10 December 2008 01:29:11 ext David Miller, you wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Date: Tue,  9 Dec 2008 09:45:59 +0200
>
> > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> Since this depends upon the buggy refcount change, I'm
> tossing this and will wait for an updated version once
> everything is sorted out.

Unfortunately, I don't think there is a way around this dependency.

I believe there is a -theoretical- race condition between the deferred work, 
and the device unregistering, but I could not produce it this far. The fixed 
patches follow.

-- 
Rémi Denis-Courmont
Maemo Software, Nokia Devices R&D

[PATCH net] Phonet: ensure GPRS device does not go away during TX work

[Rémi Denis-Courmont]

We need to hold the device while TX work is pending, as work is flushed
only after the network device is unregistered.

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/pep-gprs.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/phonet/pep-gprs.c b/net/phonet/pep-gprs.c
index e6e8e44..5985bc5 100644
--- a/net/phonet/pep-gprs.c
+++ b/net/phonet/pep-gprs.c
@@ -184,6 +184,7 @@ static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
 	spin_lock(&dev->tx_lock);
 	if (likely(skb_queue_len(&dev->tx_queue) < dev->tx_max)) {
 		skb_queue_tail(&dev->tx_queue, skb);
+		dev_hold(net);
 		skb = NULL;
 	}
 	if (skb_queue_len(&dev->tx_queue) >= dev->tx_max)
@@ -221,6 +222,7 @@ static void gprs_tx(struct work_struct *work)
 			net->stats.tx_errors++;
 		}
 		release_sock(sk);
+		dev_put(net);
 	}
 
 	lock_sock(sk);
-- 
1.5.4.3

Re: [PATCH net] Phonet: ensure GPRS device does not go away during TX work

[David Miller]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Wed, 10 Dec 2008 15:15:28 +0200

> We need to hold the device while TX work is pending, as work is flushed
> only after the network device is unregistered.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

This seems unnecessary.

If you simply create proper ->open and ->stop methods for the GPRS net
device you can purge the TX queue in ->stop() and avoid this
refcounting business.

Re: [PATCH net] Phonet: ensure GPRS device does not go away during TX work

[Rémi Denis-Courmont]

On Thursday 11 December 2008 01:28:45 ext David Miller, you wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Date: Wed, 10 Dec 2008 15:15:28 +0200
>
> > We need to hold the device while TX work is pending, as work is flushed
> > only after the network device is unregistered.
> >
> > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> This seems unnecessary.
>
> If you simply create proper ->open and ->stop methods for the GPRS net
> device you can purge the TX queue in ->stop() and avoid this
> refcounting business.

Ok. Can I assume that scheduled work will be flushed sometime between ->stop 
and ->destructor then?

-- 
Rémi Denis-Courmont
Maemo Software, Nokia Devices R&D

Re: [PATCH net] Phonet: ensure GPRS device does not go away during TX work

[David Miller]

From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia.com>
Date: Thu, 11 Dec 2008 10:11:17 +0200

> Can I assume that scheduled work will be flushed sometime between
> ->stop and ->destructor then?

If you have a workqueue you can explicitly flush it in your
->stop() method.

[PATCH net-2.6] Phonet: keep TX queue disabled when the device is off

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/pep-gprs.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/net/phonet/pep-gprs.c b/net/phonet/pep-gprs.c
index e6e8e44..22848dd 100644
--- a/net/phonet/pep-gprs.c
+++ b/net/phonet/pep-gprs.c
@@ -155,12 +155,13 @@ static void gprs_data_ready(struct sock *sk, int len)
 static void gprs_write_space(struct sock *sk)
 {
 	struct gprs_dev *dev = sk->sk_user_data;
+	struct net_device *net = dev->net;
 	unsigned credits = pep_writeable(sk);
 
 	spin_lock_bh(&dev->tx_lock);
 	dev->tx_max = credits;
-	if (credits > skb_queue_len(&dev->tx_queue))
-		netif_wake_queue(dev->net);
+	if (credits > skb_queue_len(&dev->tx_queue) && netif_running(net))
+		netif_wake_queue(net);
 	spin_unlock_bh(&dev->tx_lock);
 }
 
@@ -168,6 +169,23 @@ static void gprs_write_space(struct sock *sk)
  * Network device callbacks
  */
 
+static int gprs_open(struct net_device *dev)
+{
+	struct gprs_dev *gp = netdev_priv(dev);
+
+	gprs_write_space(gp->sk);
+	return 0;
+}
+
+static int gprs_close(struct net_device *dev)
+{
+	struct gprs_dev *gp = netdev_priv(dev);
+
+	netif_stop_queue(dev);
+	flush_work(&gp->tx_work);
+	return 0;
+}
+
 static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
 {
 	struct gprs_dev *dev = netdev_priv(net);
@@ -248,6 +266,8 @@ static void gprs_setup(struct net_device *net)
 	net->tx_queue_len	= 10;
 
 	net->destructor		= free_netdev;
+	net->open		= gprs_open;
+	net->stop		= gprs_close;
 	net->hard_start_xmit	= gprs_xmit; /* mandatory */
 	net->change_mtu		= gprs_set_mtu;
 }
@@ -311,7 +331,6 @@ int gprs_attach(struct sock *sk)
 	dev->sk = sk;
 
 	printk(KERN_DEBUG"%s: attached\n", net->name);
-	gprs_write_space(sk); /* kick off TX */
 	return net->ifindex;
 
 out_rel:
@@ -334,7 +353,5 @@ void gprs_detach(struct sock *sk)
 
 	printk(KERN_DEBUG"%s: detached\n", net->name);
 	unregister_netdev(net);
-	flush_scheduled_work();
 	sock_put(sk);
-	skb_queue_purge(&dev->tx_queue);
 }
-- 
1.5.4.3

Re: [PATCH net-2.6] Phonet: keep TX queue disabled when the device is off

[David Miller]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Thu, 11 Dec 2008 16:04:02 +0200

> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Applied, thanks.

[PATCH net-next] Phonet: improve GPRS variable names

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/pep-gprs.c |  170 +++++++++++++++++++++++++------------------------
 1 files changed, 86 insertions(+), 84 deletions(-)

diff --git a/net/phonet/pep-gprs.c b/net/phonet/pep-gprs.c
index 5985bc5..e60ba59 100644
--- a/net/phonet/pep-gprs.c
+++ b/net/phonet/pep-gprs.c
@@ -40,7 +40,7 @@ struct gprs_dev {
 	void			(*old_data_ready)(struct sock *, int);
 	void			(*old_write_space)(struct sock *);
 
-	struct net_device	*net;
+	struct net_device	*dev;
 
 	struct sk_buff_head	tx_queue;
 	struct work_struct	tx_work;
@@ -72,17 +72,19 @@ static __be16 gprs_type_trans(struct sk_buff *skb)
 
 static void gprs_state_change(struct sock *sk)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
+	struct gprs_dev *gp = sk->sk_user_data;
 
 	if (sk->sk_state == TCP_CLOSE_WAIT) {
-		netif_stop_queue(dev->net);
-		netif_carrier_off(dev->net);
+		struct net_device *dev = gp->dev;
+
+		netif_stop_queue(dev);
+		netif_carrier_off(dev);
 	}
 }
 
-static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
+static int gprs_recv(struct gprs_dev *gp, struct sk_buff *skb)
 {
-	struct net_device *net = dev->net;
+	struct net_device *dev = gp->dev;
 	int err = 0;
 	__be16 protocol = gprs_type_trans(skb);
 
@@ -99,7 +101,7 @@ static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
 		 * so wrap the IP packet as a single fragment of an head-less
 		 * socket buffer. The network stack will pull what it needs,
 		 * but at least, the whole IP payload is not memcpy'd. */
-		rskb = netdev_alloc_skb(net, 0);
+		rskb = netdev_alloc_skb(dev, 0);
 		if (!rskb) {
 			err = -ENOBUFS;
 			goto drop;
@@ -123,11 +125,11 @@ static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
 
 	skb->protocol = protocol;
 	skb_reset_mac_header(skb);
-	skb->dev = net;
+	skb->dev = dev;
 
-	if (likely(net->flags & IFF_UP)) {
-		net->stats.rx_packets++;
-		net->stats.rx_bytes += skb->len;
+	if (likely(dev->flags & IFF_UP)) {
+		dev->stats.rx_packets++;
+		dev->stats.rx_bytes += skb->len;
 		netif_rx(skb);
 		skb = NULL;
 	} else
@@ -136,41 +138,41 @@ static int gprs_recv(struct gprs_dev *dev, struct sk_buff *skb)
 drop:
 	if (skb) {
 		dev_kfree_skb(skb);
-		net->stats.rx_dropped++;
+		dev->stats.rx_dropped++;
 	}
 	return err;
 }
 
 static void gprs_data_ready(struct sock *sk, int len)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
+	struct gprs_dev *gp = sk->sk_user_data;
 	struct sk_buff *skb;
 
 	while ((skb = pep_read(sk)) != NULL) {
 		skb_orphan(skb);
-		gprs_recv(dev, skb);
+		gprs_recv(gp, skb);
 	}
 }
 
 static void gprs_write_space(struct sock *sk)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
+	struct gprs_dev *gp = sk->sk_user_data;
 	unsigned credits = pep_writeable(sk);
 
-	spin_lock_bh(&dev->tx_lock);
-	dev->tx_max = credits;
-	if (credits > skb_queue_len(&dev->tx_queue))
-		netif_wake_queue(dev->net);
-	spin_unlock_bh(&dev->tx_lock);
+	spin_lock_bh(&gp->tx_lock);
+	gp->tx_max = credits;
+	if (credits > skb_queue_len(&gp->tx_queue))
+		netif_wake_queue(gp->dev);
+	spin_unlock_bh(&gp->tx_lock);
 }
 
 /*
  * Network device callbacks
  */
 
-static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
+static int gprs_xmit(struct sk_buff *skb, struct net_device *dev)
 {
-	struct gprs_dev *dev = netdev_priv(net);
+	struct gprs_dev *gp = netdev_priv(dev);
 
 	switch (skb->protocol) {
 	case  htons(ETH_P_IP):
@@ -181,17 +183,17 @@ static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
 		return 0;
 	}
 
-	spin_lock(&dev->tx_lock);
-	if (likely(skb_queue_len(&dev->tx_queue) < dev->tx_max)) {
-		skb_queue_tail(&dev->tx_queue, skb);
-		dev_hold(net);
+	spin_lock(&gp->tx_lock);
+	if (likely(skb_queue_len(&gp->tx_queue) < gp->tx_max)) {
+		skb_queue_tail(&gp->tx_queue, skb);
+		dev_hold(dev);
 		skb = NULL;
 	}
-	if (skb_queue_len(&dev->tx_queue) >= dev->tx_max)
-		netif_stop_queue(net);
-	spin_unlock(&dev->tx_lock);
+	if (skb_queue_len(&gp->tx_queue) >= gp->tx_max)
+		netif_stop_queue(dev);
+	spin_unlock(&gp->tx_lock);
 
-	schedule_work(&dev->tx_work);
+	schedule_work(&gp->tx_work);
 	if (unlikely(skb))
 		dev_kfree_skb(skb);
 	return 0;
@@ -199,16 +201,16 @@ static int gprs_xmit(struct sk_buff *skb, struct net_device *net)
 
 static void gprs_tx(struct work_struct *work)
 {
-	struct gprs_dev *dev = container_of(work, struct gprs_dev, tx_work);
-	struct net_device *net = dev->net;
-	struct sock *sk = dev->sk;
+	struct gprs_dev *gp = container_of(work, struct gprs_dev, tx_work);
+	struct net_device *dev = gp->dev;
+	struct sock *sk = gp->sk;
 	struct sk_buff *skb;
 
-	while ((skb = skb_dequeue(&dev->tx_queue)) != NULL) {
+	while ((skb = skb_dequeue(&gp->tx_queue)) != NULL) {
 		int err;
 
-		net->stats.tx_bytes += skb->len;
-		net->stats.tx_packets++;
+		dev->stats.tx_bytes += skb->len;
+		dev->stats.tx_packets++;
 
 		skb_orphan(skb);
 		skb_set_owner_w(skb, sk);
@@ -217,12 +219,12 @@ static void gprs_tx(struct work_struct *work)
 		err = pep_write(sk, skb);
 		if (err) {
 			LIMIT_NETDEBUG(KERN_WARNING"%s: TX error (%d)\n",
-					net->name, err);
-			net->stats.tx_aborted_errors++;
-			net->stats.tx_errors++;
+					dev->name, err);
+			dev->stats.tx_aborted_errors++;
+			dev->stats.tx_errors++;
 		}
 		release_sock(sk);
-		dev_put(net);
+		dev_put(dev);
 	}
 
 	lock_sock(sk);
@@ -230,28 +232,28 @@ static void gprs_tx(struct work_struct *work)
 	release_sock(sk);
 }
 
-static int gprs_set_mtu(struct net_device *net, int new_mtu)
+static int gprs_set_mtu(struct net_device *dev, int new_mtu)
 {
 	if ((new_mtu < 576) || (new_mtu > (PHONET_MAX_MTU - 11)))
 		return -EINVAL;
 
-	net->mtu = new_mtu;
+	dev->mtu = new_mtu;
 	return 0;
 }
 
-static void gprs_setup(struct net_device *net)
+static void gprs_setup(struct net_device *dev)
 {
-	net->features		= NETIF_F_FRAGLIST;
-	net->type		= ARPHRD_NONE;
-	net->flags		= IFF_POINTOPOINT | IFF_NOARP;
-	net->mtu		= GPRS_DEFAULT_MTU;
-	net->hard_header_len	= 0;
-	net->addr_len		= 0;
-	net->tx_queue_len	= 10;
-
-	net->destructor		= free_netdev;
-	net->hard_start_xmit	= gprs_xmit; /* mandatory */
-	net->change_mtu		= gprs_set_mtu;
+	dev->features		= NETIF_F_FRAGLIST;
+	dev->type		= ARPHRD_NONE;
+	dev->flags		= IFF_POINTOPOINT | IFF_NOARP;
+	dev->mtu		= GPRS_DEFAULT_MTU;
+	dev->hard_header_len	= 0;
+	dev->addr_len		= 0;
+	dev->tx_queue_len	= 10;
+
+	dev->destructor		= free_netdev;
+	dev->hard_start_xmit	= gprs_xmit; /* mandatory */
+	dev->change_mtu		= gprs_set_mtu;
 }
 
 /*
@@ -265,28 +267,28 @@ static void gprs_setup(struct net_device *net)
 int gprs_attach(struct sock *sk)
 {
 	static const char ifname[] = "gprs%d";
-	struct gprs_dev *dev;
-	struct net_device *net;
+	struct gprs_dev *gp;
+	struct net_device *dev;
 	int err;
 
 	if (unlikely(sk->sk_type == SOCK_STREAM))
 		return -EINVAL; /* need packet boundaries */
 
 	/* Create net device */
-	net = alloc_netdev(sizeof(*dev), ifname, gprs_setup);
-	if (!net)
+	dev = alloc_netdev(sizeof(*gp), ifname, gprs_setup);
+	if (!dev)
 		return -ENOMEM;
-	dev = netdev_priv(net);
-	dev->net = net;
-	dev->tx_max = 0;
-	spin_lock_init(&dev->tx_lock);
-	skb_queue_head_init(&dev->tx_queue);
-	INIT_WORK(&dev->tx_work, gprs_tx);
-
-	netif_stop_queue(net);
-	err = register_netdev(net);
+	gp = netdev_priv(dev);
+	gp->dev = dev;
+	gp->tx_max = 0;
+	spin_lock_init(&gp->tx_lock);
+	skb_queue_head_init(&gp->tx_queue);
+	INIT_WORK(&gp->tx_work, gprs_tx);
+
+	netif_stop_queue(dev);
+	err = register_netdev(dev);
 	if (err) {
-		free_netdev(net);
+		free_netdev(dev);
 		return err;
 	}
 
@@ -300,43 +302,43 @@ int gprs_attach(struct sock *sk)
 		err = -EINVAL;
 		goto out_rel;
 	}
-	sk->sk_user_data	= dev;
-	dev->old_state_change	= sk->sk_state_change;
-	dev->old_data_ready	= sk->sk_data_ready;
-	dev->old_write_space	= sk->sk_write_space;
+	sk->sk_user_data	= gp;
+	gp->old_state_change	= sk->sk_state_change;
+	gp->old_data_ready	= sk->sk_data_ready;
+	gp->old_write_space	= sk->sk_write_space;
 	sk->sk_state_change	= gprs_state_change;
 	sk->sk_data_ready	= gprs_data_ready;
 	sk->sk_write_space	= gprs_write_space;
 	release_sock(sk);
 
 	sock_hold(sk);
-	dev->sk = sk;
+	gp->sk = sk;
 
-	printk(KERN_DEBUG"%s: attached\n", net->name);
+	printk(KERN_DEBUG"%s: attached\n", dev->name);
 	gprs_write_space(sk); /* kick off TX */
-	return net->ifindex;
+	return dev->ifindex;
 
 out_rel:
 	release_sock(sk);
-	unregister_netdev(net);
+	unregister_netdev(dev);
 	return err;
 }
 
 void gprs_detach(struct sock *sk)
 {
-	struct gprs_dev *dev = sk->sk_user_data;
-	struct net_device *net = dev->net;
+	struct gprs_dev *gp = sk->sk_user_data;
+	struct net_device *dev = gp->dev;
 
 	lock_sock(sk);
 	sk->sk_user_data	= NULL;
-	sk->sk_state_change	= dev->old_state_change;
-	sk->sk_data_ready	= dev->old_data_ready;
-	sk->sk_write_space	= dev->old_write_space;
+	sk->sk_state_change	= gp->old_state_change;
+	sk->sk_data_ready	= gp->old_data_ready;
+	sk->sk_write_space	= gp->old_write_space;
 	release_sock(sk);
 
-	printk(KERN_DEBUG"%s: detached\n", net->name);
-	unregister_netdev(net);
+	printk(KERN_DEBUG"%s: detached\n", dev->name);
+	unregister_netdev(dev);
 	flush_scheduled_work();
 	sock_put(sk);
-	skb_queue_purge(&dev->tx_queue);
+	skb_queue_purge(&gp->tx_queue);
 }
-- 
1.5.4.3

Re: [PATCH net] Phonet: hold GPRS device while cleaning up

[David Miller]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Mon,  8 Dec 2008 12:09:25 +0200

> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Applied.

Re: [PATCH net] Phonet: hold GPRS device while cleaning up

[Rémi Denis-Courmont]

On Tuesday 09 December 2008 10:15:11 ext David Miller, you wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Date: Mon,  8 Dec 2008 12:09:25 +0200
>
> > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> Applied.

Uho :( I ended up not testing what I thought I was :-$ This patch deadlocks 
unregister_netdev(), since that waits for the refcount to drop to zero.

-- 
Rémi Denis-Courmont
Maemo Software, Nokia Devices R&D

Re: [PATCH net] Phonet: hold GPRS device while cleaning up

[David Miller]

From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia.com>
Date: Tue, 9 Dec 2008 17:03:41 +0200

> On Tuesday 09 December 2008 10:15:11 ext David Miller, you wrote:
> > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> > Date: Mon,  8 Dec 2008 12:09:25 +0200
> >
> > > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> >
> > Applied.
> 
> Uho :( I ended up not testing what I thought I was :-$ This patch deadlocks 
> unregister_netdev(), since that waits for the refcount to drop to zero.

Lucky for you I didn't push this out anywhere yet.  I'll
just revert it locally.

You're on double-secret-probation, please test your patches
in the future :)