Patchwork [RFC-PATCH,1/2] wpa_supplicant: add new config params to be used with the ibss join command

login
register
mail settings
Submitter Antonio Quartulli
Date June 4, 2012, 11:41 a.m.
Message ID <1338810072-31040-2-git-send-email-ordex@autistici.org>
Download mbox | patch
Permalink /patch/162759/
State Superseded
Headers show

Comments

Antonio Quartulli - June 4, 2012, 11:41 a.m.
Signed-hostap: Antonio Quartulli <ordex@autistici.org>
---
 src/drivers/driver.h            |    6 +++
 wpa_supplicant/config.c         |   96 +++++++++++++++++++++++++++++++++++++++
 wpa_supplicant/config_ssid.h    |    6 +++
 wpa_supplicant/wpa_supplicant.c |   23 +++++++---
 4 files changed, 124 insertions(+), 7 deletions(-)
Johannes Berg - June 4, 2012, 11:44 a.m.
On Mon, 2012-06-04 at 13:41 +0200, Antonio Quartulli wrote:

> +	{ INT_RANGE(fixed_freq, 0, 1) },

Fixed freq (and fixed BSSID) are kinda hacks and not really supported by
any other driver/device, I'm not sure they should be supported here?

johannes
Antonio Quartulli - June 4, 2012, 12:24 p.m.
On Mon, Jun 04, 2012 at 01:44:39PM +0200, Johannes Berg wrote:
> On Mon, 2012-06-04 at 13:41 +0200, Antonio Quartulli wrote:
> 
> > +	{ INT_RANGE(fixed_freq, 0, 1) },
> 
> Fixed freq (and fixed BSSID) are kinda hacks and not really supported by
> any other driver/device, I'm not sure they should be supported here?

mh..is there any way to specific "driver specific" options? Actually this param
will be used (if set) by the nl80211 driver and ignored by all the others..

I don't know where I could move this param.

Cheers,
Dan Williams - June 5, 2012, 9:53 p.m.
On Mon, 2012-06-04 at 14:24 +0200, Antonio Quartulli wrote:
> On Mon, Jun 04, 2012 at 01:44:39PM +0200, Johannes Berg wrote:
> > On Mon, 2012-06-04 at 13:41 +0200, Antonio Quartulli wrote:
> > 
> > > +	{ INT_RANGE(fixed_freq, 0, 1) },
> > 
> > Fixed freq (and fixed BSSID) are kinda hacks and not really supported by
> > any other driver/device, I'm not sure they should be supported here?
> 
> mh..is there any way to specific "driver specific" options? Actually this param
> will be used (if set) by the nl80211 driver and ignored by all the others..
> 
> I don't know where I could move this param.

For the most part, driver specific parameters are evil and shouldn't be
used.  What's fixed_freq again?  Does that force the device onto a
single frequency and disable IBSS merging?  Perhaps instead of using
"fixed_freq", which seems to overlap a lot with the existing 'frequency'
config item, you could just have an option to disable IBSS merging if
it's supported by the driver?  It's obviously just a hint, since not all
devices (even ones that support cfg80211) support disabling IBSS
merging.

Dan
Antonio Quartulli - June 6, 2012, 6:39 a.m.
Hello,

On Tue, Jun 05, 2012 at 04:53:01PM -0500, Dan Williams wrote:
> On Mon, 2012-06-04 at 14:24 +0200, Antonio Quartulli wrote:
> > On Mon, Jun 04, 2012 at 01:44:39PM +0200, Johannes Berg wrote:
> > > On Mon, 2012-06-04 at 13:41 +0200, Antonio Quartulli wrote:
> > > 
> > > > +	{ INT_RANGE(fixed_freq, 0, 1) },
> > > 
> > > Fixed freq (and fixed BSSID) are kinda hacks and not really supported by
> > > any other driver/device, I'm not sure they should be supported here?
> > 
> > mh..is there any way to specific "driver specific" options? Actually this param
> > will be used (if set) by the nl80211 driver and ignored by all the others..
> > 
> > I don't know where I could move this param.
> 
> For the most part, driver specific parameters are evil and shouldn't be
> used. 

I think that most of the params I added are supported by other drivers as well
(not 100% sure).
We just need to add the support for them in each driver-related code.

> What's fixed_freq again?  Does that force the device onto a
> single frequency and disable IBSS merging?  Perhaps instead of using
> "fixed_freq", which seems to overlap a lot with the existing 'frequency'
> config item, you could just have an option to disable IBSS merging if
> it's supported by the driver?  It's obviously just a hint, since not all
> devices (even ones that support cfg80211) support disabling IBSS
> merging.
> 

As far as I can tell, frequency is the initial value where the device will look
for other IBSS. Fixed-freq will force the driver to get stuck on that frequency,
even if there is another IBSS on another channel.

Johannes can probably confirm or disprove what I said...
Actually I'm not "adding a new behaviour" to wpa_s, I'm simply allowing the use
to configure all the parameters that the ibss join command support. Whether the 

There are
many people out there that use the "iw" command to join an IBSS with the freq
and fixed_freq params set. What I'm doing here is allowing these people to
configure the same params when using wpa_s.



Cheers,
Jouni Malinen - June 6, 2012, 11:56 a.m.
On Wed, Jun 06, 2012 at 08:39:40AM +0200, Antonio Quartulli wrote:
> I think that most of the params I added are supported by other drivers as well
> (not 100% sure).
> We just need to add the support for them in each driver-related code.

Well, I would first like to get some justification for this
functionality in the first place. This seems to be something that is not
compliant with the way IBSS was designed work. As such, there better be
some notes on the commit log and wpa_supplicant.conf describing what is
the use case for this and warnings on this breaking IBSS compatibility
due to missing merges with other parts of the same IBSS with standard
compliant STAs.

> Actually I'm not "adding a new behaviour" to wpa_s, I'm simply allowing the use
> to configure all the parameters that the ibss join command support.

This is new functionality from wpa_supplicant view point and needs to be
documented in wpa_supplicant.conf.

Does the multicast rate really need to be set in wpa_supplicant? If
someone needs that special configuration, I would assume that iw can be
used after the IBSS has been enabled.

By the way, you cannot include drivers/nl80211_copy.h into
wpa_supplicant/config_ssid.h since config_ssid.h needs to remain
independent of the driver interface.

I would suggest reordering the patches in a way that you add the
parameters one by one and include all the related changes
(driver_nl80211.c, core changes, wpa_supplicant.conf documentation) in
each patch. While I don't really in general like the non-compliant IBSS
operations, this would make it somewhat more likely that you can
convince me to take the fixed_freq and beacon_int (please rename that to
beacon_int to match with hostapd and also use it for AP mode
configuration instead of just IBSS).
Antonio Quartulli - June 6, 2012, 1:11 p.m.
On Wed, Jun 06, 2012 at 02:56:32PM +0300, Jouni Malinen wrote:
> Does the multicast rate really need to be set in wpa_supplicant? If
> someone needs that special configuration, I would assume that iw can be
> used after the IBSS has been enabled.

That's the problem. nl80211 doesn't provide any way to modify such params after
the ibss has been established.

> 
> By the way, you cannot include drivers/nl80211_copy.h into
> wpa_supplicant/config_ssid.h since config_ssid.h needs to remain
> independent of the driver interface.

ok

> 
> I would suggest reordering the patches in a way that you add the
> parameters one by one and include all the related changes
> (driver_nl80211.c, core changes, wpa_supplicant.conf documentation) in
> each patch. While I don't really in general like the non-compliant IBSS
> operations, this would make it somewhat more likely that you can
> convince me to take the fixed_freq and beacon_int (please rename that to
> beacon_int to match with hostapd and also use it for AP mode
> configuration instead of just IBSS).
> 


ok, will do! :)

Thanks a lot,
	Antonio
Jouni Malinen - June 6, 2012, 1:52 p.m.
On Wed, Jun 06, 2012 at 03:11:30PM +0200, Antonio Quartulli wrote:
> On Wed, Jun 06, 2012 at 02:56:32PM +0300, Jouni Malinen wrote:
> > Does the multicast rate really need to be set in wpa_supplicant? If
> > someone needs that special configuration, I would assume that iw can be
> > used after the IBSS has been enabled.
> 
> That's the problem. nl80211 doesn't provide any way to modify such params after
> the ibss has been established.

Ah.. I assumed that that was the case only for parameters than cannot
change during IBSS lifetime (e.g., set of supported rates), but it
shouldn't be limitation for multicast rate. It looks like the nl80211
interface is not really ideal for this particular parameter.
Antonio Quartulli - June 6, 2012, 2:54 p.m.
On Wed, Jun 06, 2012 at 04:52:44PM +0300, Jouni Malinen wrote:
> On Wed, Jun 06, 2012 at 03:11:30PM +0200, Antonio Quartulli wrote:
> > On Wed, Jun 06, 2012 at 02:56:32PM +0300, Jouni Malinen wrote:
> > > Does the multicast rate really need to be set in wpa_supplicant? If
> > > someone needs that special configuration, I would assume that iw can be
> > > used after the IBSS has been enabled.
> > 
> > That's the problem. nl80211 doesn't provide any way to modify such params after
> > the ibss has been established.
> 
> Ah.. I assumed that that was the case only for parameters than cannot
> change during IBSS lifetime (e.g., set of supported rates), but it
> shouldn't be limitation for multicast rate. It looks like the nl80211
> interface is not really ideal for this particular parameter.

I agree...I think that the interface for the IBSS handling in nl80211 is missing
something (I think IBSS is not the major objective of most developers out
there).

However, since the IBSS join command supports such params, why should wpa_s
continue to be not able to specify them?


Cheers,
Jouni Malinen - June 6, 2012, 3:11 p.m.
On Wed, Jun 06, 2012 at 04:54:07PM +0200, Antonio Quartulli wrote:
> However, since the IBSS join command supports such params, why should wpa_s
> continue to be not able to specify them?

Having pointless parameters available is not a good justification for
adding new code to increase complexity and size of wpa_supplicant.. That
said, if there is reasonable use case for parameters that have to be set
at the time of IBSS join command, it should be doable to come up with
such justification.

Patch

diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index f7fb2ef..dda2fbc 100644
--- a/src/drivers/driver.h
+++ b/src/drivers/driver.h
@@ -19,6 +19,7 @@ 
 
 #define WPA_SUPPLICANT_DRIVER_VERSION 4
 
+#include "drivers/nl80211_copy.h"
 #include "common/defs.h"
 
 #define HOSTAPD_CHAN_DISABLED 0x00000001
@@ -332,6 +333,11 @@  struct wpa_driver_associate_params {
 	 */
 	int freq;
 
+	int beacon_interval;
+	int fixed_freq;
+	unsigned char rates[NL80211_MAX_SUPP_RATES];
+	int mcast_rate;
+
 	/**
 	 * bg_scan_period - Background scan period in seconds, 0 to disable
 	 * background scan, or -1 to indicate no change to default driver
diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c
index ce763b3..3d6c0e6 100644
--- a/wpa_supplicant/config.c
+++ b/wpa_supplicant/config.c
@@ -14,6 +14,7 @@ 
 #include "rsn_supp/wpa.h"
 #include "eap_peer/eap.h"
 #include "p2p/p2p.h"
+#include "drivers/nl80211_copy.h"
 #include "config.h"
 
 
@@ -1436,6 +1437,97 @@  static char * wpa_config_write_p2p_client_list(const struct parse_data *data,
 
 #endif /* CONFIG_P2P */
 
+static int wpa_config_parse_mcast_rate(const struct parse_data *data,
+				       struct wpa_ssid *ssid, int line,
+				       const char *value)
+{
+	ssid->mcast_rate = (int)(strtod(value, NULL) * 10);
+
+	return 0;
+}
+
+#ifndef NO_CONFIG_WRITE
+static char * wpa_config_write_mcast_rate(const struct parse_data *data,
+					  struct wpa_ssid *ssid)
+{
+	char *value;
+	int res;
+
+	if (!ssid->mcast_rate == 0)
+		return NULL;
+
+	value = os_malloc(6); /* longest: 300.0 */
+	if (value == NULL)
+		return NULL;
+	res = os_snprintf(value, 5, "%.1f", (double)ssid->mcast_rate / 10);
+	if (res < 0) {
+		os_free(value);
+		return NULL;
+	}
+	return value;
+}
+#endif /* NO_CONFIG_WRITE */
+
+static int wpa_config_parse_rates(const struct parse_data *data,
+				  struct wpa_ssid *ssid, int line,
+				  const char *value)
+{
+	int i;
+	char *pos, *r, *sptr, *end;
+	double rate;
+
+	pos = (char *)value;
+	r = strtok_r(pos, ",", &sptr);
+	i = 0;
+	while (pos && i < NL80211_MAX_SUPP_RATES) {
+		rate = 0.0;
+		if (r)
+			rate = strtod(r, &end);
+		ssid->rates[i] = rate * 2;
+		if (*end != '\0' || rate * 2 != ssid->rates[i])
+			return 1;
+
+		i++;
+		r = strtok_r(NULL, ",", &sptr);
+	}
+
+	return 0;
+}
+
+#ifndef NO_CONFIG_WRITE
+static char * wpa_config_write_rates(const struct parse_data *data,
+				     struct wpa_ssid *ssid)
+{
+	char *value, *pos;
+	int res, i;
+
+	if (ssid->rates[0] <= 0)
+		return NULL;
+
+	value = os_malloc(6 * NL80211_MAX_SUPP_RATES + 1);
+	if (value == NULL)
+		return NULL;
+	pos = value;
+	for (i = 0; i < NL80211_MAX_SUPP_RATES - 1; i++) {
+		res = os_snprintf(pos, 6, "%.1f,", (double)ssid->rates[i] / 2);
+		if (res < 0) {
+			os_free(value);
+			return NULL;
+		}
+		pos += res;
+	}
+	res = os_snprintf(pos, 6, "%.1f",
+			  (double)ssid->rates[NL80211_MAX_SUPP_RATES - 1] / 2);
+	if (res < 0) {
+		os_free(value);
+		return NULL;
+	}
+
+	value[6 * NL80211_MAX_SUPP_RATES] = '\0';
+	return value;
+}
+#endif /* NO_CONFIG_WRITE */
+
 /* Helper macros for network block parser */
 
 #ifdef OFFSET
@@ -1610,6 +1702,10 @@  static const struct parse_data ssid_fields[] = {
 	{ STR(ht_mcs) },
 #endif /* CONFIG_HT_OVERRIDES */
 	{ INT(ap_max_inactivity) },
+	{ INT_RANGE(fixed_freq, 0, 1) },
+	{ INT_RANGE(beacon_interval, 0, 1000) },
+	{ FUNC(rates) },
+	{ FUNC(mcast_rate) },
 };
 
 #undef OFFSET
diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h
index 80d4382..8d152a4 100644
--- a/wpa_supplicant/config_ssid.h
+++ b/wpa_supplicant/config_ssid.h
@@ -11,6 +11,7 @@ 
 
 #include "common/defs.h"
 #include "eap_peer/eap_config.h"
+#include "drivers/nl80211_copy.h"
 
 #define MAX_SSID_LEN 32
 
@@ -499,6 +500,11 @@  struct wpa_ssid {
 	 * By default: 300 seconds.
 	 */
 	int ap_max_inactivity;
+
+	int fixed_freq;
+	int beacon_interval;
+	unsigned char rates[NL80211_MAX_SUPP_RATES];
+	double mcast_rate;
 };
 
 #endif /* CONFIG_SSID_H */
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
index 3cb954d..59efa16 100644
--- a/wpa_supplicant/wpa_supplicant.c
+++ b/wpa_supplicant/wpa_supplicant.c
@@ -1363,15 +1363,24 @@  void wpa_supplicant_associate(struct wpa_supplicant *wpa_s,
 		params.ssid_len = ssid->ssid_len;
 	}
 
-	if (ssid->mode == WPAS_MODE_IBSS && ssid->bssid_set &&
-	    wpa_s->conf->ap_scan == 2) {
-		params.bssid = ssid->bssid;
-		params.fixed_bssid = 1;
+	if (ssid->mode == WPAS_MODE_IBSS) {
+		if (ssid->bssid_set && wpa_s->conf->ap_scan == 2) {
+			params.bssid = ssid->bssid;
+			params.fixed_bssid = 1;
+		}
+		if (ssid->frequency > 0 && params.freq == 0)
+			/* Initial channel for IBSS */
+			params.freq = ssid->frequency;
+		params.fixed_freq = ssid->fixed_freq;
+		params.beacon_interval = ssid->beacon_interval;
+		i = 0;
+		while (i < NL80211_MAX_SUPP_RATES) {
+			params.rates[i] = ssid->rates[i];
+			i++;
+		}
+		params.mcast_rate = ssid->mcast_rate;
 	}
 
-	if (ssid->mode == WPAS_MODE_IBSS && ssid->frequency > 0 &&
-	    params.freq == 0)
-		params.freq = ssid->frequency; /* Initial channel for IBSS */
 	params.wpa_ie = wpa_ie;
 	params.wpa_ie_len = wpa_ie_len;
 	params.pairwise_suite = cipher_pairwise;