| Message ID | 1344866384-12147-1-git-send-email-eyal@wizery.com |
|---|---|
| State | Rejected |
| Headers | show |
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?
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
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.
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;