Patchwork P2P: For concurrency scenarios we should not update the scan results without P2PIE to p2p group interface

login
register
mail settings
Submitter Neeraj Garg
Date April 17, 2012, 5:12 a.m.
Message ID <F764D07D6734DF489C733939E1A6F85A02379F@SJEXCHMB12.corp.ad.broadcom.com>
Download mbox | patch
Permalink /patch/153057/
State Superseded
Headers show

Comments

Neeraj Garg - April 17, 2012, 5:12 a.m.
Hello Jouni,
Yes, actually sharing scan results from STA interface to p2p interfaces are not required (because STA scan results shall be without P2P IE). In my opinion, we still need to share the scan results from p2p interfaces to STA interface to help roaming scenarios.

I have come-up with a better patch to take care of another racing issue in group-formation failure because of sharing of scan results in concurrency scenarios. Please let us know if the attached patch is good.

From 2d847c70d40e8babcec6d9f8e74136f416c4e8f5 Mon Sep 17 00:00:00 2001
From: Neeraj Garg <neerajkg@broadcom.com>
Date: Tue, 17 Apr 2012 10:26:59 +0530
Subject: [PATCH] Do not share the scan results from STA interface to p2p interfaces
Signed-off-by: Neeraj Garg <neerajkg@broadcom.com>
---
 wpa_supplicant/bss.c    |   12 ------------
 wpa_supplicant/events.c |    9 ++++++++-
 2 files changed, 8 insertions(+), 13 deletions(-)
 mode change 100644 => 100755 wpa_supplicant/bss.c
 mode change 100644 => 100755 wpa_supplicant/events.c
Jouni Malinen - April 22, 2012, 5:51 p.m.
On Tue, Apr 17, 2012 at 05:12:37AM +0000, Neeraj Kumar Garg wrote:
> Yes, actually sharing scan results from STA interface to p2p interfaces are not required (because STA scan results shall be without P2P IE). In my opinion, we still need to share the scan results from p2p interfaces to STA interface to help roaming scenarios.

Sounds fine.

> I have come-up with a better patch to take care of another racing issue in group-formation failure because of sharing of scan results in concurrency scenarios. Please let us know if the attached patch is good.

I'm not sure I fully understand why this is needed. What kind of issue
does this solve?

Is there a specific reason for removing the previous version, i.e., this
part in bss.c?

> diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
> @@ -379,18 +379,6 @@ void wpa_bss_update_scan_res(struct wpa_supplicant *wpa_s,
>  	p2p = wpa_scan_get_vendor_ie(res, P2P_IE_VENDOR_TYPE);
> -#ifdef CONFIG_P2P
> -	if (p2p == NULL &&
> -	    wpa_s->p2p_group_interface != NOT_P2P_GROUP_INTERFACE) {
> -		/*
> -		 * If it's a P2P specific interface, then don't update
> -		 * the scan result without a P2P IE.
> -		 */
> -		wpa_printf(MSG_DEBUG, "BSS: No P2P IE - skipping BSS " MACSTR
> -			   " update for P2P interface", MAC2STR(res->bssid));
> -		return;
> -	}
> -#endif /* CONFIG_P2P */

This saves some memory by not recording non-P2P BSSes for a P2P group
interface. I would have assumed that this part can be kept even if the
following change is applied.

> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> @@ -1181,7 +1181,14 @@ static void wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
>  		if (rn2 && os_strcmp(rn, rn2) == 0) {
>  			wpa_printf(MSG_DEBUG, "%s: Updating scan results from "
>  				   "sibling", ifs->ifname);
> -			_wpa_supplicant_event_scan_results(ifs, data);
> +			if ( (ifs->global->p2p) || (ifs->p2p_group_interface != NOT_P2P_GROUP_INTERFACE)) {
> +				/* Do not update the scan results from STA interface to p2p interfaces */
> +				wpa_printf(MSG_DEBUG, "Not Updating scan results on interface %s from "
> +					   "sibling %s", ifs->ifname, wpa_s->ifname);
> +				continue;
> +			}
> +			else
> +				_wpa_supplicant_event_scan_results(ifs, data);

This looks fine, though, I would like to better understand what kind of
issues did show up without this.

Patch

diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
old mode 100644
new mode 100755
index 792316d..b79510e
--- a/wpa_supplicant/bss.c
+++ b/wpa_supplicant/bss.c
@@ -379,18 +379,6 @@  void wpa_bss_update_scan_res(struct wpa_supplicant *wpa_s,
 	}
 
 	p2p = wpa_scan_get_vendor_ie(res, P2P_IE_VENDOR_TYPE);
-#ifdef CONFIG_P2P
-	if (p2p == NULL &&
-	    wpa_s->p2p_group_interface != NOT_P2P_GROUP_INTERFACE) {
-		/*
-		 * If it's a P2P specific interface, then don't update
-		 * the scan result without a P2P IE.
-		 */
-		wpa_printf(MSG_DEBUG, "BSS: No P2P IE - skipping BSS " MACSTR
-			   " update for P2P interface", MAC2STR(res->bssid));
-		return;
-	}
-#endif /* CONFIG_P2P */
 	if (p2p && ssid[1] == P2P_WILDCARD_SSID_LEN &&
 	    os_memcmp(ssid + 2, P2P_WILDCARD_SSID, P2P_WILDCARD_SSID_LEN) == 0)
 		return; /* Skip P2P listen discovery results here */
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
old mode 100644
new mode 100755
index 8fdc544..6a58ed1
--- a/wpa_supplicant/events.c
+++ b/wpa_supplicant/events.c
@@ -1181,7 +1181,14 @@  static void wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
 		if (rn2 && os_strcmp(rn, rn2) == 0) {
 			wpa_printf(MSG_DEBUG, "%s: Updating scan results from "
 				   "sibling", ifs->ifname);
-			_wpa_supplicant_event_scan_results(ifs, data);
+			if ( (ifs->global->p2p) || (ifs->p2p_group_interface != NOT_P2P_GROUP_INTERFACE)) {
+				/* Do not update the scan results from STA interface to p2p interfaces */
+				wpa_printf(MSG_DEBUG, "Not Updating scan results on interface %s from "
+					   "sibling %s", ifs->ifname, wpa_s->ifname);
+				continue;
+			}
+			else
+				_wpa_supplicant_event_scan_results(ifs, data);
 		}
 	}
 }