[PATCH v2] mac80211: Call commit() on channel setting

Alina Friedrichsen

author

2009-02-26 23:43 UTC #0 lore raw

PATCH v2 1/1 mac80211: Call commit() on channel setting

This calls commit() on all logical interfaces of a physical interface, = if the channel of it is changed. Before it works as it should, I must f= ix the joining process of STA and IBSS a little, which I will make tomo= rrow.

Signed-off-by Alina Friedrichsen <[email protected]>

---

net/mac80211/cfg.c +0 -0

index c43129e..015062e 100644
@@ -1173,11 +1173,19 @@ static int ieee80211_set_channel(struct wiphy *=

wiphy,
enum nl80211_channel_type channel_type)
{
struct ieee80211_local *local =3D wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata;
+ int ret;
=20
local->oper_channel =3D chan; local->oper_channel_type =3D channel_type;
=20
- return ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+ ret =3D ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+
+ list_for_each_entry(sdata, &local->interfaces, list) {
+ ieee80211_commit(sdata);
+ }
+
+ return ret;
}
=20
static int set_mgmt_extra_ie_sta(struct ieee80211_sub_if_data *sdata,

net/mac80211/wext.c +0 -0

index 1ac0516..dce1618 100644
@@ -157,7 +157,10 @@ static int ieee80211_ioctl_siwfreq(struct net_devi=

ce *dev,
struct iw_request_info *info, struct iw_freq *freq, char *extra)
{
+ struct ieee80211_local *local =3D wdev_priv(dev->ieee80211_ptr);
struct ieee80211_sub_if_data *sdata =3D IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_sub_if_data *nsdata;
+ int ret;
=20
if (sdata->vif.type =3D=3D NL80211_IFTYPE_ADHOC)
sdata->u.ibss.flags &=3D ~IEEE80211_IBSS_AUTO_CHANNEL_SEL;
@@ -173,19 +176,25 @@ static int ieee80211_ioctl_siwfreq(struct net_dev= ice *dev,
else if (sdata->vif.type =3D=3D NL80211_IFTYPE_STATION)
sdata->u.mgd.flags |=3D
IEEE80211_STA_AUTO_CHANNEL_SEL;
- return 0;
+ ret =3D 0;
} else
- return ieee80211_set_freq(sdata,
+ ret =3D ieee80211_set_freq(sdata,
ieee80211_channel_to_frequency(freq->m));
} else {
int i, div =3D 1000000;
for (i =3D 0; i < freq->e; i++)
div /=3D 10;
if (div > 0)
- return ieee80211_set_freq(sdata, freq->m / div);
+ ret =3D ieee80211_set_freq(sdata, freq->m / div);
else
- return -EINVAL;
+ ret =3D -EINVAL;
}
+
+ list_for_each_entry(nsdata, &local->interfaces, list) {
+ ieee80211_commit(nsdata);
+ }
+
+ return ret;
}
=20
=20

--=20
Computer Bild Tarifsieger! GMX FreeDSL - Telefonanschluss + DSL f=FCr nur 17,95 =BF/mtl.!* http://dsl.gmx.de/?ac=3DOM.AD.PD003K11308T45=
69a
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael Buesch

2009-02-27 14:04 UTC #1 lore raw

Quoted 1 line · On Friday 27 February 2009 00:43:07 Alina Friedrichsen wrote:

This calls commit() on all logical interfaces of a physical interface, if the channel of it is changed. Before it works as it should, I must fix the joining process of STA and IBSS a little, which I will make tomorrow.

Could you explain what commit is supposed to to after the channel switch? To me it's not clear what the difference in functionality is. What do you expect to happen on the commit call after the channel change.

Quoted 12 lines

Signed-off-by Alina Friedrichsen <[email protected]>

---

net/mac80211/cfg.c +2 -0

index c43129e..015062e 100644
@@ -1173,11 +1173,19 @@ static int ieee80211_set_channel(struct wiphy *wiphy,
 				 enum nl80211_channel_type channel_type)
 {
 	struct ieee80211_local *local = wiphy_priv(wiphy);
+	struct ieee80211_sub_if_data *sdata;
+	int ret;

net/mac80211/cfg.c static int ieee80211_set_channel(struct wiphy *wiphy, · line 1186

Quoted 10 lines

net/mac80211/cfg.c +5 -1

 
 	local->oper_channel = chan;
 	local->oper_channel_type = channel_type;
 
-	return ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+	ret = ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		ieee80211_commit(sdata);
+	}

Why do you commit, even if channel change failed?

Quoted 35 lines

net/mac80211/cfg.c +2 -0

+
+	return ret;
 }
 
 static int set_mgmt_extra_ie_sta(struct ieee80211_sub_if_data *sdata,

net/mac80211/wext.c +5 -2

index 1ac0516..dce1618 100644
@@ -157,7 +157,10 @@ static int ieee80211_ioctl_siwfreq(struct net_device *dev,
 				   struct iw_request_info *info,
 				   struct iw_freq *freq, char *extra)
 {
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct ieee80211_sub_if_data *nsdata;
+	int ret;
 
 	if (sdata->vif.type == NL80211_IFTYPE_ADHOC)
 		sdata->u.ibss.flags &= ~IEEE80211_IBSS_AUTO_CHANNEL_SEL;

@@ -173,19 +176,25 @@ static int ieee80211_ioctl_siwfreq(struct net_device *dev,
 			else if (sdata->vif.type == NL80211_IFTYPE_STATION)
 				sdata->u.mgd.flags |=
 					IEEE80211_STA_AUTO_CHANNEL_SEL;
-			return 0;
+			ret = 0;
 		} else
-			return ieee80211_set_freq(sdata,
+			ret = ieee80211_set_freq(sdata,
 				ieee80211_channel_to_frequency(freq->m));
 	} else {
 		int i, div = 1000000;
 		for (i = 0; i < freq->e; i++)
 			div /= 10;
 		if (div > 0)

net/mac80211/wext.c static int ieee80211_ioctl_siwfreq(struct net_device *dev, · line 195

Quoted 10 lines

net/mac80211/wext.c +6 -2

-			return ieee80211_set_freq(sdata, freq->m / div);
+			ret = ieee80211_set_freq(sdata, freq->m / div);
 		else
-			return -EINVAL;
+			ret = -EINVAL;
 	}
+
+	list_for_each_entry(nsdata, &local->interfaces, list) {
+		ieee80211_commit(nsdata);
+	}

dito.

Quoted 6 lines

net/mac80211/wext.c +2 -0
+
+	return ret;
 }
 
 

Signature · 1 line

Greetings, Michael.

Alina Friedrichsen

author

2009-02-27 15:35 UTC #2 lore raw

Hello Michael!

Quoted 1 line

Could you explain what commit is supposed to to after the channel swi=

tch?

Quoted 2 lines

To me it's not clear what the difference in functionality is. What do you expect to happen on the commit call after the channel cha=

nge.

I think after all changes which affect the selected network a rejoin sh= ould be done.

=46or example you have at channel 4 a AP with the SSID "test" und at ch= annel 11 an other AP with the same SSID, too.

If you now first say set_ssid("test") and then set_channel(11), we firs= t join the network at channel 4 and then we switch the hardware low-lev= el to channel 11. So we hang now in channel 4 with the BSSID of the net= work in channel 4 and nothing works anymore.

I think the order of the commands should not affect the result on the e= nd and the driver should never hang in an broken/undefined state if it = can be recovered.

Regards
Alina

--=20
Computer Bild Tarifsieger! GMX FreeDSL - Telefonanschluss + DSL f=FCr nur 17,95 =BF/mtl.!* http://dsl.gmx.de/?ac=3DOM.AD.PD003K11308T45=
69a
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alina Friedrichsen

author

2009-02-27 15:39 UTC #3 lore raw

Quoted 4 lines

I think after all changes which affect the selected network a rejoin should be done.
=20
For example you have at channel 4 a AP with the SSID "test" und at ch=

annel

Quoted 3 lines

11 an other AP with the same SSID, too.
=20
If you now first say set_ssid("test") and then set_channel(11), we fi=

rst

Quoted 1 line

join the network at channel 4 and then we switch the hardware low-lev=

el to

Quoted 1 line

channel 11. So we hang now in channel 4 with the BSSID of the network=

in

Quoted 1 line

channel 4 and nothing works anymore.

So we hang now in channel _11_ with the BSSID of the network in channel 4 and nothing works anymore.

Quoted 1 line

I think the order of the commands should not affect the result on the=

end

Quoted 1 line

and the driver should never hang in an broken/undefined state if it c=

an be

Quoted 1 line

recovered.

--=20
Computer Bild Tarifsieger! GMX FreeDSL - Telefonanschluss + DSL f=FCr nur 17,95 =BF/mtl.!* http://dsl.gmx.de/?ac=3DOM.AD.PD003K11308T45=
69a
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael Buesch

2009-02-27 16:16 UTC #5 lore raw

Quoted 9 lines · On Friday 27 February 2009 16:34:18 Alina Friedrichsen wrote:

Quoted 3 lines
Could you explain what commit is supposed to to after the channel switch? To me it's not clear what the difference in functionality is. What do you expect to happen on the commit call after the channel change.
I think after all changes which affect the selected network a rejoin should be done.
For example you have at channel 4 a AP with the SSID "test" und at channel 11 an other AP with the same SSID, too.
If you now first say set_ssid("test") and then set_channel(11), we first join the network at channel 4 and then we switch the hardware low-level to channel 11. So we hang now in channel 4 with the BSSID of the network in channel 4 and nothing works anymore.

I think this is desired behavior. We voluntarily associated with the "test" AP on channel 4. Who says that we want to assoc to the "test" ssid on channel 11? This is a policy decision and such policy doesn't belong into the kernel. Userspace can initiate a reassociation after the channel change, if the AP switch is desired.
There are good reasons to leave roaming decisions in userspace, because these decisions are simply very complex and would put lots of policy into the kernel or require lots of kernel API.

Quoted 1 line

I think the order of the commands should not affect the result on the end and the driver should never hang in an broken/undefined state if it can be recovered.

Signature · 1 line

Greetings, Michael.

Alina Friedrichsen

author

2009-02-27 17:16 UTC #6 lore raw

Hello Michael!

Quoted 1 line

I think this is desired behavior.

Yes.

Quoted 2 lines

We voluntarily associated with the "test" AP on channel 4.

Yes this is correct because IEEE80211_IBSS_AUTO_CHANNEL_SEL flag was se= t.

Quoted 1 line

Who says that we want to assoc to the "test" ssid on channel 11?

Because this setting is now outdated. The channel is set to 11 and the = IEEE80211_IBSS_AUTO_CHANNEL_SEL flag is clear.

If we now set the SSID to test again, we assoc to "test" on channel 11.=
Do not recover from the channel set automatically, is a broken behavio=
r, I think.

Quoted 3 lines

This is a
policy decision and such policy
doesn't belong into the kernel. Userspace can initiate a reassociatio=

n

Quoted 3 lines

after the channel change,
if the AP switch is desired.
There are good reasons to leave roaming decisions in userspace, becau=

se

Quoted 2 lines

these decisions are simply very
complex and would put lots of policy into the kernel or require lots =

of

Quoted 1 line

kernel API.

I think the basic functionality should work without any userspace daemo= n. All the that can be implemented without real problems with a few lin= es of code. This would better for embedded systems, if we don't need th= e wpa_supplicant for an unencrypted network, or so.

Regards
Alina

--=20
Computer Bild Tarifsieger! GMX FreeDSL - Telefonanschluss + DSL f=FCr nur 17,95 =BF/mtl.!* http://dsl.gmx.de/?ac=3DOM.AD.PD003K11308T45=
69a
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alina Friedrichsen

author

2009-02-27 18:25 UTC #7 lore raw

PATCH v3 1/1 mac80211: Call commit() on channel setting

This calls commit() on all logical interfaces of a physical interface, = if the channel of it is changed. Before it works as it should, I must f= ix the joining process of STA and IBSS a little, which I will make tomo= rrow.

This version call it only in the wireless extensions, because the handl= ing in nl80211 must discussed more, but I think in the case of wext it = is the right way.

Signed-off-by Alina Friedrichsen <[email protected]>

---

net/mac80211/wext.c +0 -0

index 1ac0516..dce1618 100644
@@ -157,7 +157,10 @@ static int ieee80211_ioctl_siwfreq(struct net_devi=

ce *dev,
struct iw_request_info *info, struct iw_freq *freq, char *extra)
{
+ struct ieee80211_local *local =3D wdev_priv(dev->ieee80211_ptr);
struct ieee80211_sub_if_data *sdata =3D IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_sub_if_data *nsdata;
+ int ret;
=20
if (sdata->vif.type =3D=3D NL80211_IFTYPE_ADHOC)
sdata->u.ibss.flags &=3D ~IEEE80211_IBSS_AUTO_CHANNEL_SEL;
@@ -173,19 +176,25 @@ static int ieee80211_ioctl_siwfreq(struct net_dev= ice *dev,
else if (sdata->vif.type =3D=3D NL80211_IFTYPE_STATION)
sdata->u.mgd.flags |=3D
IEEE80211_STA_AUTO_CHANNEL_SEL;
- return 0;
+ ret =3D 0;
} else
- return ieee80211_set_freq(sdata,
+ ret =3D ieee80211_set_freq(sdata,
ieee80211_channel_to_frequency(freq->m));
} else {
int i, div =3D 1000000;
for (i =3D 0; i < freq->e; i++)
div /=3D 10;
if (div > 0)
- return ieee80211_set_freq(sdata, freq->m / div);
+ ret =3D ieee80211_set_freq(sdata, freq->m / div);
else
- return -EINVAL;
+ ret =3D -EINVAL;
}
+
+ list_for_each_entry(nsdata, &local->interfaces, list) {
+ ieee80211_commit(nsdata);
+ }
+
+ return ret;
}
=20
=20

--=20
Psssst! Schon vom neuen GMX MultiMessenger geh=F6rt? Der kann`s mit all=
en: http://www.gmx.net/de/go/multimessenger01
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johannes Berg

maintainer

2009-02-27 21:02 UTC #8 lore raw

In reply to Alina Friedrichsen

Quoted 1 line · Alina Friedrichsen

This calls commit() on all logical interfaces of a physical interface, if the channel of it is changed. Before it works as it should, I must fix the joining process of STA and IBSS a little, which I will make tomorrow.

You really should remember to linewrap your descriptions.

Review

Quoted 3 lines

(quoted diff) +3 -0

+	list_for_each_entry(sdata, &local->interfaces, list) {
+		ieee80211_commit(sdata);
+	}

That doesn't look like it can work? sta_commit/ibss_commit? Separate by interface type?

johannes

Alina Friedrichsen

author

2009-02-27 21:26 UTC #9 lore raw

Hello Johannes!

Quoted 1 line

You really should remember to linewrap your descriptions.

Sorry, I'm accustomed that the software wrap it automatically to the screen width. (XHTML and so...)

Can you please describe how a IBSS join should done with nl80211? "Das sich wext und nl80211 nicht gegenseitig auf die Fuesse treten." (Sorry I can't translate this to English - locution.)

Regards
Alina

P.S.: I have received this mail 5 hours to late.

--=20
Psssst! Schon vom neuen GMX MultiMessenger geh=F6rt? Der kann`s mit all=
en: http://www.gmx.net/de/go/multimessenger01
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johannes Berg

maintainer

2009-02-27 21:35 UTC #10 lore raw

Quoted 11 lines · Alina Friedrichsen

Hello Johannes!
Quoted 1 line
You really should remember to linewrap your descriptions.
Sorry, I'm accustomed that the software wrap it automatically to the screen width. (XHTML and so...)
Can you please describe how a IBSS join should done with nl80211? "Das sich wext und nl80211 nicht gegenseitig auf die Fuesse treten." (Sorry I can't translate this to English - locution.)

on the todo list

Quoted 4 lines

Regards
Alina
P.S.: I have received this mail 5 hours to late.

well I\m travelling and wrote that on plane -- off for a week now

johannes