Patchwork Add P2P IEs to probe requests only when in P2P

login
register
mail settings
Submitter Eyal Shapira
Date Aug. 13, 2012, 1:59 p.m.
Message ID <1344866384-12147-1-git-send-email-eyal@wizery.com>
Download mbox | patch
Permalink /patch/176945/
State Rejected
Headers show

Comments

Eyal Shapira - Aug. 13, 2012, 1:59 p.m.
The current code caused P2P and consquently WPS IEs to be added
to any probe as the check was for non null global->p2p.
global->p2p gets set whenever P2P is initialized globally
and that occurs on the first interface init.
Fix this by indicating wps is in use only when in active P2P
discovery or connection establishement.
(i.e. not P2P_IDLE - either doing search, neg or provisioning)

Signed-hostap: Eyal Shapira <eyal@wizery.com>
---
 src/p2p/p2p.c                   |    7 +++++++
 src/p2p/p2p.h                   |    9 +++++++++
 wpa_supplicant/p2p_supplicant.c |    7 +++++++
 wpa_supplicant/p2p_supplicant.h |    1 +
 wpa_supplicant/scan.c           |    2 +-
 5 files changed, 25 insertions(+), 1 deletion(-)
Jouni Malinen - Aug. 13, 2012, 6:53 p.m.
On Mon, Aug 13, 2012 at 04:59:44PM +0300, Eyal Shapira wrote:
> The current code caused P2P and consquently WPS IEs to be added
> to any probe as the check was for non null global->p2p.
> global->p2p gets set whenever P2P is initialized globally
> and that occurs on the first interface init.

This is done on purpose since "normal scans" can be used to discover P2P
groups and to allow other devices discover us. The P2P element (and WPS
IE if no active WPS operation is in progress) are removed if P2P is
disabled.

> Fix this by indicating wps is in use only when in active P2P
> discovery or connection establishement.
> (i.e. not P2P_IDLE - either doing search, neg or provisioning)

What does this fix?
Eyal Shapira - Aug. 13, 2012, 9:38 p.m.
On 13 August 2012 21:53, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Aug 13, 2012 at 04:59:44PM +0300, Eyal Shapira wrote:
>> The current code caused P2P and consquently WPS IEs to be added
>> to any probe as the check was for non null global->p2p.
>> global->p2p gets set whenever P2P is initialized globally
>> and that occurs on the first interface init.
>
> This is done on purpose since "normal scans" can be used to discover P2P
> groups and to allow other devices discover us. The P2P element (and WPS
> IE if no active WPS operation is in progress) are removed if P2P is
> disabled.
>
>> Fix this by indicating wps is in use only when in active P2P
>> discovery or connection establishement.
>> (i.e. not P2P_IDLE - either doing search, neg or provisioning)
>
> What does this fix?

ok. thanks. I wasn't aware this was by design and therefore referred
to it as a problem. Anyways, the reasoning here was:

1. Adding P2P+WPS IEs to all probes makes them significantly bigger
(the actual size is dependent on variable size parameters in the WPS IE
like model name/device name/model number/manufacturer). On our
default dev platform it goes from probes of 70 bytes to 250 bytes.
Given that these go out in 1Mbps multiple times on all channels this
has an impact
on power and total scan time. The effect is worse once you're probing multiple
networks with SSID specific probes (scan_ssid networks) as there are
more probes in that case.

2. We've seen some old APs having issues with big probe requests.

3. When multiple interfaces are used (like in Android JB - wlan0 and p2p0)
since P2P struct is "global" having P2P initialized on one interface (e.g. p2p0)
affect probes going out on other interfaces
Does this make sense given the different MACs ?

4. Focusing on Android - we're looking at Galaxy Nexus as the reference behavior
for STA + P2P. Looks like only probes related to P2P (i.e. once you go into
WiFi Direct UI and P2P_FIND begins) go out with P2P+WPS IE. Normal STA scans
don't include the P2P IE. I know it's not necessarily a good argument
here but still
worth noting as a reference.
Maybe make this an Android specific patch ?

Thanks,
Eyal

>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
Jouni Malinen - Aug. 17, 2012, 5:39 p.m.
On Tue, Aug 14, 2012 at 12:38:13AM +0300, Eyal Shapira wrote:
> 1. Adding P2P+WPS IEs to all probes makes them significantly bigger
> (the actual size is dependent on variable size parameters in the WPS IE
> like model name/device name/model number/manufacturer). On our
> default dev platform it goes from probes of 70 bytes to 250 bytes.
> Given that these go out in 1Mbps multiple times on all channels this
> has an impact
> on power and total scan time. The effect is worse once you're probing multiple
> networks with SSID specific probes (scan_ssid networks) as there are
> more probes in that case.

It should be fine to not include P2P+WPS IE from the Probe Request
frames for specific SSIDs that are known to not be P2P devices. Though,
we do not really have a mechanism in nl80211 for specifying that for a
single scan trigger, so wpa_supplicant would need to split the scans
into separate commands in a way that keeps the non-P2P specific SSID
scan_ssid=1 cases in their own commands. While this would reduce the
channel time a bit, I'm not sure whether this is really worth
complexity, though.

> 2. We've seen some old APs having issues with big probe requests.

Would you be able to identify any of these issues? I've heard of a
single case where a WPS AP has some issues (delayed Probe Response) due
to the WPS element, but that is the only issue I remember having heard
of in this context.

> 3. When multiple interfaces are used (like in Android JB - wlan0 and p2p0)
> since P2P struct is "global" having P2P initialized on one interface (e.g. p2p0)
> affect probes going out on other interfaces
> Does this make sense given the different MACs ?

If there is a specific interface that is used only for non-P2P
operations, it should be fine to leave out the P2P+WPS elements.
However, this is not really supported with cfg80211/mac80211 very
cleanly yet (Johannes has some ongoing work on this with the separate
P2P device instance).

If "different MACs" is referring to different MAC addresses, then yes,
including WPS+P2P makes sense. The key point for me is in whether the
interface (or address, I'd guess) can be used for P2P. In the current
mac80211 behavior, the initial wlan0 interface is used both for non-P2P
STA and P2P device operations while the p2p-wlan0-0 interface is used
only for P2P. With this design, both wlan0 and p2p-wlan0-0 should
include P2P+WPS elements.

> 4. Focusing on Android - we're looking at Galaxy Nexus as the reference behavior
> for STA + P2P. Looks like only probes related to P2P (i.e. once you go into
> WiFi Direct UI and P2P_FIND begins) go out with P2P+WPS IE. Normal STA scans
> don't include the P2P IE. I know it's not necessarily a good argument
> here but still
> worth noting as a reference.

I would say Android is quite a bit opposite of a good example of how to
support P2P properly with cfg80211 and wpa_supplicant.. It is getting
better, but I do not want to repeat the mess with ICS and vendor
specific patches that conflict with upstream design.

That said, if the station interface is indeed separate allocated for
non-P2P use and is separate from the P2P device operations, it sounds
reasonable to consider not including WPS+P2P elements in Probe Request
frames. However, I would like to support this only with a design that
complies with the upstream cfg80211 design for handling the P2P device
operations with a MAC address that does not match with the station
interface MAC address.

> Maybe make this an Android specific patch ?

Absolutely not. Android should not be a special case and it needs to
work in the way as any other Linux-based system would work with
cfg80211. The mess from ICS has cost countless hours in support and in
coming up with pointless workarounds to meet silly product requirements
to match Android design over upstream Linux wireless design when all
this could have been avoided by working properly with upstream in the
first place.

Patch

diff --git a/src/p2p/p2p.c b/src/p2p/p2p.c
index 80b345a..0d8fcf4 100644
--- a/src/p2p/p2p.c
+++ b/src/p2p/p2p.c
@@ -3959,3 +3959,10 @@  int p2p_in_progress(struct p2p_data *p2p)
 		return 0;
 	return p2p->state != P2P_IDLE && p2p->state != P2P_PROVISIONING;
 }
+
+int p2p_non_idle(struct p2p_data *p2p)
+{
+	if (p2p == NULL)
+		return 0;
+	return p2p->state != P2P_IDLE;
+}
diff --git a/src/p2p/p2p.h b/src/p2p/p2p.h
index 7e47270..81f72c4 100644
--- a/src/p2p/p2p.h
+++ b/src/p2p/p2p.h
@@ -1688,6 +1688,15 @@  int p2p_set_pref_chan(struct p2p_data *p2p, unsigned int num_pref_chan,
 int p2p_in_progress(struct p2p_data *p2p);
 
 /**
+ * p2p_non_idle - Check whether P2P is not in P2P_IDLE. That
+ * means we're in either search, GO neg or provisioing. Once connected
+ * it's back to idle. p2p_in_progress excludes provisioing.
+ * @p2p: P2P module context from p2p_init()
+ * Returns: 0 if P2P module is idle or 1 if an operation is in progress
+ */
+int p2p_non_idle(struct p2p_data *p2p);
+
+/**
  * p2p_other_scan_completed - Notify completion of non-P2P scan
  * @p2p: P2P module context from p2p_init()
  * Returns: 0 if P2P module is idle or 1 if an operation was started
diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index df65de9..adc799c 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -4828,6 +4828,13 @@  int wpas_p2p_in_progress(struct wpa_supplicant *wpa_s)
 	return p2p_in_progress(wpa_s->global->p2p);
 }
 
+int wpas_p2p_non_idle(struct wpa_supplicant *wpa_s)
+{
+	if (wpa_s->global->p2p_disabled || wpa_s->global->p2p == NULL)
+		return 0;
+
+	return p2p_non_idle(wpa_s->global->p2p);
+}
 
 void wpas_p2p_network_removed(struct wpa_supplicant *wpa_s,
 			      struct wpa_ssid *ssid)
diff --git a/wpa_supplicant/p2p_supplicant.h b/wpa_supplicant/p2p_supplicant.h
index e2fe259..dc9f8a9 100644
--- a/wpa_supplicant/p2p_supplicant.h
+++ b/wpa_supplicant/p2p_supplicant.h
@@ -133,6 +133,7 @@  int wpas_p2p_disconnect(struct wpa_supplicant *wpa_s);
 void wpas_p2p_wps_failed(struct wpa_supplicant *wpa_s,
 			 struct wps_event_fail *fail);
 int wpas_p2p_in_progress(struct wpa_supplicant *wpa_s);
+int wpas_p2p_non_idle(struct wpa_supplicant *wpa_s);
 void wpas_p2p_network_removed(struct wpa_supplicant *wpa_s,
 			      struct wpa_ssid *ssid);
 struct wpa_ssid * wpas_p2p_get_persistent(struct wpa_supplicant *wpa_s,
diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
index 6c3f625..386a3fb 100644
--- a/wpa_supplicant/scan.c
+++ b/wpa_supplicant/scan.c
@@ -66,7 +66,7 @@  static int wpas_wps_in_use(struct wpa_supplicant *wpa_s,
 	}
 
 #ifdef CONFIG_P2P
-	if (!wpa_s->global->p2p_disabled && wpa_s->global->p2p) {
+	if (wpas_p2p_non_idle(wpa_s)) {
 		wpa_s->wps->dev.p2p = 1;
 		if (!wps) {
 			wps = 1;