[wireless] wireless: Keep phy name consistent across module reloads.

[wireless] wireless: Keep phy name consistent across module reloads.

[greearb]

From: Ben Greear <greearb@candelatech.com>

It adds needless pain to user-space scripts to change the name
of the phy on each module reload.  Instead, find the first
index.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 9c21ebf... 8226ba7... M	net/wireless/core.c
 net/wireless/core.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 9c21ebf..8226ba7 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -319,8 +319,7 @@ static void cfg80211_event_work(struct work_struct *work)
 
 struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
 {
-	static int wiphy_counter;
-
+	int i;
 	struct cfg80211_registered_device *rdev;
 	int alloc_size;
 
@@ -342,12 +341,18 @@ struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
 
 	mutex_lock(&cfg80211_mutex);
 
-	rdev->wiphy_idx = wiphy_counter++;
+	/* 64k wiphy devices is enough for anyone! */
+	for (i = 0; i < 0xFFFF; i++) {
+		if (!cfg80211_rdev_by_wiphy_idx(i))
+			break;
+	}
+	if (i == 0xFFFF)
+		i = -1; /* invalid */
+	rdev->wiphy_idx = i;
 
 	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx))) {
-		wiphy_counter--;
 		mutex_unlock(&cfg80211_mutex);
-		/* ugh, wrapped! */
+		/* ugh, too many devices already! */
 		kfree(rdev);
 		return NULL;
 	}
-- 
1.7.2.2

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Johannes Berg]

On Tue, 2010-09-21 at 16:57 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> It adds needless pain to user-space scripts to change the name
> of the phy on each module reload.  Instead, find the first
> index.

Crap, I clearly missed this patch, John, please revert this.

We cannot do this because otherwise operations you attempt to do one one
PHY will race with re-creating of the same phy index. For the same
reason, interface indexes aren't reused for netdevs.

If this bothers you so much, you can easily write a udev rule that
renames all your phys to "myphy" or whatever.

johannes

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Ben Greear]

On 09/24/2010 02:02 PM, Johannes Berg wrote:
> On Tue, 2010-09-21 at 16:57 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> It adds needless pain to user-space scripts to change the name
>> of the phy on each module reload.  Instead, find the first
>> index.
>
> Crap, I clearly missed this patch, John, please revert this.
>
> We cannot do this because otherwise operations you attempt to do one one
> PHY will race with re-creating of the same phy index. For the same
> reason, interface indexes aren't reused for netdevs.
>
> If this bothers you so much, you can easily write a udev rule that
> renames all your phys to "myphy" or whatever.

Sorry about that.

Would a similar patch be acceptable that just set the name to the first
available phyX, but left the index to increment monotomically as it does
now be OK?

I'd really like to not have to deal with udev if possible!

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Johannes Berg]

On Fri, 2010-09-24 at 14:10 -0700, Ben Greear wrote:

> > We cannot do this because otherwise operations you attempt to do one one
> > PHY will race with re-creating of the same phy index. For the same
> > reason, interface indexes aren't reused for netdevs.
> >
> > If this bothers you so much, you can easily write a udev rule that
> > renames all your phys to "myphy" or whatever.
> 
> Sorry about that.
> 
> Would a similar patch be acceptable that just set the name to the first
> available phyX, but left the index to increment monotomically as it does
> now be OK?

Actually, right now it doesn't even allow you to use a "phy%d" name,
which is because I don't want (sysfs, debugfs) errors coming from doing
"iw phy0 set name phy1" and then plugging in a new device ... In doing
that, I manage to avoid having to allocate two different numbers...

I suppose it'd be possible to treat this more like interfaces. Want to
also make it per net namespace? ;-)

> I'd really like to not have to deal with udev if possible!

It's really not very hard, you just need a rule that reacts to adding a
new phy, and calls "iw phy#7 set name myphy" :-)

johannes

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Ben Greear]

On 09/24/2010 02:18 PM, Johannes Berg wrote:
> On Fri, 2010-09-24 at 14:10 -0700, Ben Greear wrote:
>
>>> We cannot do this because otherwise operations you attempt to do one one
>>> PHY will race with re-creating of the same phy index. For the same
>>> reason, interface indexes aren't reused for netdevs.
>>>
>>> If this bothers you so much, you can easily write a udev rule that
>>> renames all your phys to "myphy" or whatever.
>>
>> Sorry about that.
>>
>> Would a similar patch be acceptable that just set the name to the first
>> available phyX, but left the index to increment monotomically as it does
>> now be OK?
>
> Actually, right now it doesn't even allow you to use a "phy%d" name,
> which is because I don't want (sysfs, debugfs) errors coming from doing
> "iw phy0 set name phy1" and then plugging in a new device ... In doing
> that, I manage to avoid having to allocate two different numbers...

I tried rename few months ago, to rename back to phy0, and it failed
for no obvious reason..so I just assumed rename didn't work at all.
Would you like a patch to print something in kernel logs when someone
tries to rename to phy%d?

> I suppose it'd be possible to treat this more like interfaces. Want to
> also make it per net namespace? ;-)

Probably best for all involved if I don't mess with namespaces right now,
but I'll be happy to work on (or test) a patch to find the first un-used phyX name,
if that's what you mean.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Johannes Berg]

On Fri, 2010-09-24 at 14:25 -0700, Ben Greear wrote:

> > Actually, right now it doesn't even allow you to use a "phy%d" name,
> > which is because I don't want (sysfs, debugfs) errors coming from doing
> > "iw phy0 set name phy1" and then plugging in a new device ... In doing
> > that, I manage to avoid having to allocate two different numbers...
> 
> I tried rename few months ago, to rename back to phy0, and it failed
> for no obvious reason..so I just assumed rename didn't work at all.
> Would you like a patch to print something in kernel logs when someone
> tries to rename to phy%d?

Well, if you want to work on getting an unused phy%d name this would be
kinda pointless, no?

> > I suppose it'd be possible to treat this more like interfaces. Want to
> > also make it per net namespace? ;-)
> 
> Probably best for all involved if I don't mess with namespaces right now,
> but I'll be happy to work on (or test) a patch to find the first un-used phyX name,
> if that's what you mean.

I think that'd be OK, even if maybe at this point a little unexpected?

johannes

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Ben Greear]

On 09/24/2010 02:31 PM, Johannes Berg wrote:
> On Fri, 2010-09-24 at 14:25 -0700, Ben Greear wrote:
>
>>> Actually, right now it doesn't even allow you to use a "phy%d" name,
>>> which is because I don't want (sysfs, debugfs) errors coming from doing
>>> "iw phy0 set name phy1" and then plugging in a new device ... In doing
>>> that, I manage to avoid having to allocate two different numbers...
>>
>> I tried rename few months ago, to rename back to phy0, and it failed
>> for no obvious reason..so I just assumed rename didn't work at all.
>> Would you like a patch to print something in kernel logs when someone
>> tries to rename to phy%d?
>
> Well, if you want to work on getting an unused phy%d name this would be
> kinda pointless, no?
>
>>> I suppose it'd be possible to treat this more like interfaces. Want to
>>> also make it per net namespace? ;-)
>>
>> Probably best for all involved if I don't mess with namespaces right now,
>> but I'll be happy to work on (or test) a patch to find the first un-used phyX name,
>> if that's what you mean.
>
> I think that'd be OK, even if maybe at this point a little unexpected?

I'll give it a try right quick.

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Ben Greear]

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

On 09/24/2010 02:31 PM, Johannes Berg wrote:
> On Fri, 2010-09-24 at 14:25 -0700, Ben Greear wrote:
>
>>> Actually, right now it doesn't even allow you to use a "phy%d" name,
>>> which is because I don't want (sysfs, debugfs) errors coming from doing
>>> "iw phy0 set name phy1" and then plugging in a new device ... In doing
>>> that, I manage to avoid having to allocate two different numbers...
>>
>> I tried rename few months ago, to rename back to phy0, and it failed
>> for no obvious reason..so I just assumed rename didn't work at all.
>> Would you like a patch to print something in kernel logs when someone
>> tries to rename to phy%d?
>
> Well, if you want to work on getting an unused phy%d name this would be
> kinda pointless, no?
>
>>> I suppose it'd be possible to treat this more like interfaces. Want to
>>> also make it per net namespace? ;-)
>>
>> Probably best for all involved if I don't mess with namespaces right now,
>> but I'll be happy to work on (or test) a patch to find the first un-used phyX name,
>> if that's what you mean.
>
> I think that'd be OK, even if maybe at this point a little unexpected?

Here's a patch for consideration.  It's on top of my other patch,
but I can re-do it against a tree with that reverted if you prefer.

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 8226ba7..312a44b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -319,9 +319,12 @@ static void cfg80211_event_work(struct work_struct *work)

  struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
  {
+	static int wiphy_counter;
  	int i;
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev, *rdev2;
  	int alloc_size;
+	char nname[IFNAMSIZ + 1];
+	bool found = false;

  	WARN_ON(ops->add_key && (!ops->del_key || !ops->set_default_key));
  	WARN_ON(ops->auth && (!ops->assoc || !ops->deauth || !ops->disassoc));
@@ -341,26 +344,38 @@ struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)

  	mutex_lock(&cfg80211_mutex);

+	rdev->wiphy_idx = wiphy_counter++;
+
+	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx)))
+		goto too_many_devs;
+
  	/* 64k wiphy devices is enough for anyone! */
  	for (i = 0; i < 0xFFFF; i++) {
-		if (!cfg80211_rdev_by_wiphy_idx(i))
+		found = false;
+		snprintf(nname, sizeof(nname)-1, PHY_NAME "%d", i);
+		nname[sizeof(nname)-1] = 0;
+		list_for_each_entry(rdev2, &cfg80211_rdev_list, list)
+			if (strcmp(nname, dev_name(&rdev2->wiphy.dev)) == 0) {
+				found = true;
+				break;
+			}
+
+		if (!found)
  			break;
  	}
-	if (i == 0xFFFF)
-		i = -1; /* invalid */
-	rdev->wiphy_idx = i;

-	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx))) {
+	if (unlikely(found)) {
+too_many_devs:
  		mutex_unlock(&cfg80211_mutex);
  		/* ugh, too many devices already! */
  		kfree(rdev);
  		return NULL;
  	}

-	mutex_unlock(&cfg80211_mutex);
-
  	/* give it a proper name */
-	dev_set_name(&rdev->wiphy.dev, PHY_NAME "%d", rdev->wiphy_idx);
+	dev_set_name(&rdev->wiphy.dev, "%s", nname);
+
+	mutex_unlock(&cfg80211_mutex);

  	mutex_init(&rdev->mtx);
  	mutex_init(&rdev->devlist_mtx);


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


[-- Attachment #2: phy_name.patch --]
[-- Type: text/plain, Size: 1842 bytes --]

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 8226ba7..312a44b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -319,9 +319,12 @@ static void cfg80211_event_work(struct work_struct *work)
 
 struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
 {
+	static int wiphy_counter;
 	int i;
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev, *rdev2;
 	int alloc_size;
+	char nname[IFNAMSIZ + 1];
+	bool found = false;
 
 	WARN_ON(ops->add_key && (!ops->del_key || !ops->set_default_key));
 	WARN_ON(ops->auth && (!ops->assoc || !ops->deauth || !ops->disassoc));
@@ -341,26 +344,38 @@ struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
 
 	mutex_lock(&cfg80211_mutex);
 
+	rdev->wiphy_idx = wiphy_counter++;
+
+	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx)))
+		goto too_many_devs;
+
 	/* 64k wiphy devices is enough for anyone! */
 	for (i = 0; i < 0xFFFF; i++) {
-		if (!cfg80211_rdev_by_wiphy_idx(i))
+		found = false;
+		snprintf(nname, sizeof(nname)-1, PHY_NAME "%d", i);
+		nname[sizeof(nname)-1] = 0;
+		list_for_each_entry(rdev2, &cfg80211_rdev_list, list)
+			if (strcmp(nname, dev_name(&rdev2->wiphy.dev)) == 0) {
+				found = true;
+				break;
+			}
+
+		if (!found)
 			break;
 	}
-	if (i == 0xFFFF)
-		i = -1; /* invalid */
-	rdev->wiphy_idx = i;
 
-	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx))) {
+	if (unlikely(found)) {
+too_many_devs:
 		mutex_unlock(&cfg80211_mutex);
 		/* ugh, too many devices already! */
 		kfree(rdev);
 		return NULL;
 	}
 
-	mutex_unlock(&cfg80211_mutex);
-
 	/* give it a proper name */
-	dev_set_name(&rdev->wiphy.dev, PHY_NAME "%d", rdev->wiphy_idx);
+	dev_set_name(&rdev->wiphy.dev, "%s", nname);
+
+	mutex_unlock(&cfg80211_mutex);
 
 	mutex_init(&rdev->mtx);
 	mutex_init(&rdev->devlist_mtx);

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Johannes Berg]

On Fri, 2010-09-24 at 15:21 -0700, Ben Greear wrote:

> Here's a patch for consideration.  It's on top of my other patch,
> but I can re-do it against a tree with that reverted if you prefer.

You also want to remove the restriction on renaming to phy%d now.

> +	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx)))
> +		goto too_many_devs;
> +
>   	/* 64k wiphy devices is enough for anyone! */
>   	for (i = 0; i < 0xFFFF; i++) {

why limit yourself to this? You're guaranteed to find a free index if
you don't, so error handling could be easier?

johannes

Re: [wireless] wireless: Keep phy name consistent across module reloads.

[Ben Greear]

On 09/24/2010 03:25 PM, Johannes Berg wrote:
> On Fri, 2010-09-24 at 15:21 -0700, Ben Greear wrote:
>
>> Here's a patch for consideration.  It's on top of my other patch,
>> but I can re-do it against a tree with that reverted if you prefer.
>
> You also want to remove the restriction on renaming to phy%d now.

Same patch, or a second one?

>> +	if (unlikely(!wiphy_idx_valid(rdev->wiphy_idx)))
>> +		goto too_many_devs;
>> +
>>    	/* 64k wiphy devices is enough for anyone! */
>>    	for (i = 0; i<  0xFFFF; i++) {
>
> why limit yourself to this? You're guaranteed to find a free index if
> you don't, so error handling could be easier?

It's an N^2 loop, and if we are unlikely to ever need more than 512 STAs, I figure
64k phys is probably enough. :)

But, will change it if you want.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

wireless-next-2.6 rebased -- Re: [wireless] wireless: Keep phy name consistent across module reloads.

[John W. Linville]

On Fri, Sep 24, 2010 at 11:02:50PM +0200, Johannes Berg wrote:
> On Tue, 2010-09-21 at 16:57 -0700, greearb@candelatech.com wrote:
> > From: Ben Greear <greearb@candelatech.com>
> > 
> > It adds needless pain to user-space scripts to change the name
> > of the phy on each module reload.  Instead, find the first
> > index.
> 
> Crap, I clearly missed this patch, John, please revert this.

Sure, np...wireless-next-2.6 has been rebased from
9cf13668a5f8165a81349defc5f82c57a4a8279b.

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.