| Message ID | 1228730965-19087-1-git-send-email-remi.denis-courmont@nokia.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(-)