[PATCH] mac80211: don't assume driver has been attached on registration

[PATCH] mac80211: don't assume driver has been attached on registration

[Luis R. Rodriguez]

mac80211's ieee80211_register_hw() is often called within the
probe path so it cannot assume the device's driver structure
has been attached yet so to create a workqueue instead of
using driver->name use the wiphy's phy%d name. The name doesn't
really matter anyway.

This should fix sporadic oopses found when we race to beat the
driver pointer setting. Not even sure how this was working properly.

http://www.kerneloops.org/search.php?search=ieee80211_register_hw

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
Acked-by: Johannes Berg <johannes@sipsolutions.net>
---

This has been acked now.

 net/mac80211/main.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d631dc9..cec9b6d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -722,7 +722,6 @@ EXPORT_SYMBOL(ieee80211_alloc_hw);
 int ieee80211_register_hw(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
-	const char *name;
 	int result;
 	enum ieee80211_band band;
 	struct net_device *mdev;
@@ -787,8 +786,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 	mdev->header_ops = &ieee80211_header_ops;
 	mdev->set_multicast_list = ieee80211_master_set_multicast_list;
 
-	name = wiphy_dev(local->hw.wiphy)->driver->name;
-	local->hw.workqueue = create_freezeable_workqueue(name);
+	local->hw.workqueue =
+		create_freezeable_workqueue(wiphy_name(local->hw.wiphy));
 	if (!local->hw.workqueue) {
 		result = -ENOMEM;
 		goto fail_workqueue;
-- 
1.5.6.3

Re: [PATCH] mac80211: don't assume driver has been attached on registration

[Bob Copeland]

On Fri, Nov 14, 2008 at 5:44 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> mac80211's ieee80211_register_hw() is often called within the
> probe path so it cannot assume the device's driver structure
> has been attached yet so to create a workqueue instead of
> using driver->name use the wiphy's phy%d name. The name doesn't
> really matter anyway.
>
> This should fix sporadic oopses found when we race to beat the
> driver pointer setting. Not even sure how this was working properly.

I'd still like to identify the race first.  This patch won't hurt
anything, but I'm not confident it will solve any problem either (the
pci_dev->driver setting I was seeing after probe was for a different
struct driver).  Also it could make the bug harder to track down.

In the reported case, wiphy_name will be empty string which isn't too
helpful for the workqueue name either, though it probably won't crash.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] mac80211: don't assume driver has been attached on registration

[Johannes Berg]

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

On Fri, 2008-11-14 at 18:05 -0500, Bob Copeland wrote:
> On Fri, Nov 14, 2008 at 5:44 PM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
> > mac80211's ieee80211_register_hw() is often called within the
> > probe path so it cannot assume the device's driver structure
> > has been attached yet so to create a workqueue instead of
> > using driver->name use the wiphy's phy%d name. The name doesn't
> > really matter anyway.
> >
> > This should fix sporadic oopses found when we race to beat the
> > driver pointer setting. Not even sure how this was working properly.
> 
> I'd still like to identify the race first.  This patch won't hurt
> anything, but I'm not confident it will solve any problem either (the
> pci_dev->driver setting I was seeing after probe was for a different
> struct driver).  Also it could make the bug harder to track down.
> 
> In the reported case, wiphy_name will be empty string which isn't too
> helpful for the workqueue name either, though it probably won't crash.

That can't be right! wiphy_name is assigned right in wiphy_new()!

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mac80211: don't assume driver has been attached on registration

[Bob Copeland]

On Sat, Nov 15, 2008 at 12:14:24AM +0100, Johannes Berg wrote:
> > In the reported case, wiphy_name will be empty string which isn't too
> > helpful for the workqueue name either, though it probably won't crash.
> 
> That can't be right! wiphy_name is assigned right in wiphy_new()!

I know, it doesn't make any sense.  But the posted dmesg clearly shows 
ath5k printing the wiphy_name (after ieee80211_alloc_hw returned not
null): "ath5k_pci xxxx registered as ''".  So, something really odd is
going on.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] mac80211: don't assume driver has been attached on registration

[Luis R. Rodriguez]

On Fri, Nov 14, 2008 at 2:44 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> mac80211's ieee80211_register_hw() is often called within the
> probe path so it cannot assume the device's driver structure
> has been attached yet so to create a workqueue instead of
> using driver->name use the wiphy's phy%d name. The name doesn't
> really matter anyway.
>
> This should fix sporadic oopses found when we race to beat the
> driver pointer setting. Not even sure how this was working properly.
>
> http://www.kerneloops.org/search.php?search=ieee80211_register_hw
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> Acked-by: Johannes Berg <johannes@sipsolutions.net>

This is not necessary actually, it took a while to wrap my head around
the possible chicken and the egg problem (thanks to Bob for beating my
head to it) but at least I verified this is not an issue at least on
>= 2.6.25 as the device's driver *is* assigned prior to probe. This
patch however should make reading this a bit easier and the since the
naming doesn't matter maybe its best to apply it to make things
simpler. Up to you.

  Luis