Patchwork P2P:Remove scheduling of p2p scan without handling the STA scan results

login
register
mail settings
Submitter Neeraj Garg
Date Oct. 24, 2012, 10:10 a.m.
Message ID <F764D07D6734DF489C733939E1A6F85A1BD86F5D@SJEXCHMB12.corp.ad.broadcom.com>
Download mbox | patch
Permalink /patch/193724/
State Under Review
Headers show

Comments

Neeraj Garg - Oct. 24, 2012, 10:10 a.m.
If STA scan is issued by UI/application (even after STA connection is made) and then immediately, if we issue p2p_find, p2p_search will get delayed. Upon completion of STA scan, we will check for p2p_search if pending. Since p2p_search is pending, p2p_search will be issued. Upon return of p2p_search successful, supplicant will then unnecessarily schedules a STA scan again. At this time, supplicant will also ignore the received scan results. Now once p2p_search will be over, it will check if STA scan is pending. And then it will issue a STA scan. Upon getting STA scan results, we will again check if p2p_search is pending. And after scheduling the p2p_search successfully, again we will issue a STA scan.

I am not sure, what is the intent of checking for p2p_search possibility after receiving the scan_results. We can let the supplicant, process the STA scan_results. And then after that, we already have a check for scheduling the p2p_search after wpa_supplicant_event_scan_results() is completed.

If the code (removed in the below patch) is to take care of STA connection scenario, then once the STA connection is successful, we will schedule the p2p_search from that context.

From: Neeraj Garg <neerajkg@broadcom.com>
Date: Wed, 24 Oct 2012 15:27:47 +0530
Subject: [PATCH] PATCH:P2P Remove scheduling of p2p scan without handling the STA scan results

Signed-off-by: Neeraj Garg <neerajkg@broadcom.com>
---
 wpa_supplicant/events.c |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)
 mode change 100644 => 100755 wpa_supplicant/events.c
Jouni Malinen - Nov. 3, 2012, 4:28 p.m.
On Wed, Oct 24, 2012 at 10:10:23AM +0000, Neeraj Kumar Garg wrote:
> If STA scan is issued by UI/application (even after STA connection is made) and then immediately, if we issue p2p_find, p2p_search will get delayed. Upon completion of STA scan, we will check for p2p_search if pending. Since p2p_search is pending, p2p_search will be issued. Upon return of p2p_search successful, supplicant will then unnecessarily schedules a STA scan again. At this time, supplicant will also ignore the received scan results. Now once p2p_search will be over, it will check if STA scan is pending. And then it will issue a STA scan. Upon getting STA scan results, we will again check if p2p_search is pending. And after scheduling the p2p_search successfully, again we will issue a STA scan.

The design here tries to follow the last command issued and if the upper
layers in the system request something like that more or less
concurrently, they should be prepared to see not exactly optimal use of
scans..

> I am not sure, what is the intent of checking for p2p_search possibility after receiving the scan_results. We can let the supplicant, process the STA scan_results. And then after that, we already have a check for scheduling the p2p_search after wpa_supplicant_event_scan_results() is completed.

I'm not completely sure I understood. Would you be able to send a
wpa_supplicant debug logs showing the behavior before and after your
patch?

> If the code (removed in the below patch) is to take care of STA connection scenario, then once the STA connection is successful, we will schedule the p2p_search from that context.

There were number of changes in the area of how to handle concurrent P2P
and non-P2P station operations. I'm somewhat hesitant to remove this
part without first fully understanding the effects this have for all the
different concurrent sequences since it would partially revert number of
commits that addressed issues with managing the concurrent requests for
scanning.

The design goal here is to have main focus on cases that happen without
conflicting ctrl_iface commands (e.g., P2P_FIND immediately after SCAN)
and to make sure that those conflicting cases go through eventually,
even if not in ideal order.
Neeraj Garg - Nov. 9, 2012, 1:04 p.m.
Hello Jouni,

Sorry for delay in reply. Please see my replies with signatures [NG].

> If STA scan is issued by UI/application (even after STA connection is made) and then immediately, if we issue p2p_find, p2p_search will get delayed. Upon completion of STA scan, we will check for p2p_search if pending. Since p2p_search is pending, p2p_search will be issued. Upon return of p2p_search successful, supplicant will then unnecessarily schedules a STA scan again. At this time, supplicant will also ignore the received scan results. Now once p2p_search will be over, it will check if STA scan is pending. And then it will issue a STA scan. Upon getting STA scan results, we will again check if p2p_search is pending. And after scheduling the p2p_search successfully, again we will issue a STA scan.

The design here tries to follow the last command issued and if the upper
layers in the system request something like that more or less
concurrently, they should be prepared to see not exactly optimal use of
scans..

[NG] last command issued is fine but STA scan was already successfully completed. We could have easily handled the scan results and schedule the p2p search after handling the STA scan results.

> I am not sure, what is the intent of checking for p2p_search possibility after receiving the scan_results. We can let the supplicant, process the STA scan_results. And then after that, we already have a check for scheduling the p2p_search after wpa_supplicant_event_scan_results() is completed.

I'm not completely sure I understood. Would you be able to send a
wpa_supplicant debug logs showing the behavior before and after your
patch?

[NG] I could not figure out the use of this scheduling of p2p_search just after receiving scan_results. I mean we have another code after handling scan_results to check if p2p_search can be scheduled or not. So I am not sure why we really need this code segment.

> If the code (removed in the below patch) is to take care of STA connection scenario, then once the STA connection is successful, we will schedule the p2p_search from that context.

There were number of changes in the area of how to handle concurrent P2P
and non-P2P station operations. I'm somewhat hesitant to remove this
part without first fully understanding the effects this have for all the
different concurrent sequences since it would partially revert number of
commits that addressed issues with managing the concurrent requests for
scanning.

The design goal here is to have main focus on cases that happen without
conflicting ctrl_iface commands (e.g., P2P_FIND immediately after SCAN)
and to make sure that those conflicting cases go through eventually,
even if not in ideal order.

[NG] Order is fine. The commands are working fine in any order. SCAN followed by p2p_find or p2p_find followed by SCAN. But in one race condition, once I had seen that we were continuously scheduling the STA scan unnecessarily. My point is which scenario, this particular code is taking care provided that we have the almost same code to schedule p2p_search after handling the scan results.

Regards,
-Neeraj

-----Original Message-----
From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Jouni Malinen
Sent: Saturday, November 03, 2012 9:58 PM
To: hostap@lists.shmoo.com
Subject: Re: [PATCH] P2P:Remove scheduling of p2p scan without handling the STA scan results

On Wed, Oct 24, 2012 at 10:10:23AM +0000, Neeraj Kumar Garg wrote:
> If STA scan is issued by UI/application (even after STA connection is made) and then immediately, if we issue p2p_find, p2p_search will get delayed. Upon completion of STA scan, we will check for p2p_search if pending. Since p2p_search is pending, p2p_search will be issued. Upon return of p2p_search successful, supplicant will then unnecessarily schedules a STA scan again. At this time, supplicant will also ignore the received scan results. Now once p2p_search will be over, it will check if STA scan is pending. And then it will issue a STA scan. Upon getting STA scan results, we will again check if p2p_search is pending. And after scheduling the p2p_search successfully, again we will issue a STA scan.

The design here tries to follow the last command issued and if the upper
layers in the system request something like that more or less
concurrently, they should be prepared to see not exactly optimal use of
scans..

> I am not sure, what is the intent of checking for p2p_search possibility after receiving the scan_results. We can let the supplicant, process the STA scan_results. And then after that, we already have a check for scheduling the p2p_search after wpa_supplicant_event_scan_results() is completed.

I'm not completely sure I understood. Would you be able to send a
wpa_supplicant debug logs showing the behavior before and after your
patch?

> If the code (removed in the below patch) is to take care of STA connection scenario, then once the STA connection is successful, we will schedule the p2p_search from that context.

There were number of changes in the area of how to handle concurrent P2P
and non-P2P station operations. I'm somewhat hesitant to remove this
part without first fully understanding the effects this have for all the
different concurrent sequences since it would partially revert number of
commits that addressed issues with managing the concurrent requests for
scanning.

The design goal here is to have main focus on cases that happen without
conflicting ctrl_iface commands (e.g., P2P_FIND immediately after SCAN)
and to make sure that those conflicting cases go through eventually,
even if not in ideal order.
Neeraj Garg - Dec. 11, 2012, 7:21 a.m.
Hello Jouni,

Any updates on this below email? My point is do we really need the code which I removed in my patch? I am not able to generate a problematic log now (where I could see that we were continuously scheduling the STA scan) but this segment of code is little problematic. In my understanding, I am not very clear why we need this segment of code. Same piece of code to schedule p2p_scan is already present after handling the STA scan results.

Sorry for delay in reply. Please see my replies with signatures [NG].

> If STA scan is issued by UI/application (even after STA connection is made) and then immediately, if we issue p2p_find, p2p_search will get delayed. Upon completion of STA scan, we will check for p2p_search if pending. Since p2p_search is pending, p2p_search will be issued. Upon return of p2p_search successful, supplicant will then unnecessarily schedules a STA scan again. At this time, supplicant will also ignore the received scan results. Now once p2p_search will be over, it will check if STA scan is pending. And then it will issue a STA scan. Upon getting STA scan results, we will again check if p2p_search is pending. And after scheduling the p2p_search successfully, again we will issue a STA scan.

The design here tries to follow the last command issued and if the upper
layers in the system request something like that more or less
concurrently, they should be prepared to see not exactly optimal use of
scans..

[NG] last command issued is fine but STA scan was already successfully completed. We could have easily handled the scan results and schedule the p2p search after handling the STA scan results.

> I am not sure, what is the intent of checking for p2p_search possibility after receiving the scan_results. We can let the supplicant, process the STA scan_results. And then after that, we already have a check for scheduling the p2p_search after wpa_supplicant_event_scan_results() is completed.

I'm not completely sure I understood. Would you be able to send a
wpa_supplicant debug logs showing the behavior before and after your
patch?

[NG] I could not figure out the use of this scheduling of p2p_search just after receiving scan_results. I mean we have another code after handling scan_results to check if p2p_search can be scheduled or not. So I am not sure why we really need this code segment.

> If the code (removed in the below patch) is to take care of STA connection scenario, then once the STA connection is successful, we will schedule the p2p_search from that context.

There were number of changes in the area of how to handle concurrent P2P
and non-P2P station operations. I'm somewhat hesitant to remove this
part without first fully understanding the effects this have for all the
different concurrent sequences since it would partially revert number of
commits that addressed issues with managing the concurrent requests for
scanning.

The design goal here is to have main focus on cases that happen without
conflicting ctrl_iface commands (e.g., P2P_FIND immediately after SCAN)
and to make sure that those conflicting cases go through eventually,
even if not in ideal order.

[NG] Order is fine. The commands are working fine in any order. SCAN followed by p2p_find or p2p_find followed by SCAN. But in one race condition, once I had seen that we were continuously scheduling the STA scan unnecessarily. My point is which scenario, this particular code is taking care provided that we have the almost same code to schedule p2p_search after handling the scan results.

Regards,
-Neeraj

-----Original Message-----
From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Jouni Malinen
Sent: Saturday, November 03, 2012 9:58 PM
To: hostap@lists.shmoo.com
Subject: Re: [PATCH] P2P:Remove scheduling of p2p scan without handling the STA scan results

On Wed, Oct 24, 2012 at 10:10:23AM +0000, Neeraj Kumar Garg wrote:
> If STA scan is issued by UI/application (even after STA connection is made) and then immediately, if we issue p2p_find, p2p_search will get delayed. Upon completion of STA scan, we will check for p2p_search if pending. Since p2p_search is pending, p2p_search will be issued. Upon return of p2p_search successful, supplicant will then unnecessarily schedules a STA scan again. At this time, supplicant will also ignore the received scan results. Now once p2p_search will be over, it will check if STA scan is pending. And then it will issue a STA scan. Upon getting STA scan results, we will again check if p2p_search is pending. And after scheduling the p2p_search successfully, again we will issue a STA scan.

The design here tries to follow the last command issued and if the upper
layers in the system request something like that more or less
concurrently, they should be prepared to see not exactly optimal use of
scans..

> I am not sure, what is the intent of checking for p2p_search possibility after receiving the scan_results. We can let the supplicant, process the STA scan_results. And then after that, we already have a check for scheduling the p2p_search after wpa_supplicant_event_scan_results() is completed.

I'm not completely sure I understood. Would you be able to send a
wpa_supplicant debug logs showing the behavior before and after your
patch?

> If the code (removed in the below patch) is to take care of STA connection scenario, then once the STA connection is successful, we will schedule the p2p_search from that context.

There were number of changes in the area of how to handle concurrent P2P
and non-P2P station operations. I'm somewhat hesitant to remove this
part without first fully understanding the effects this have for all the
different concurrent sequences since it would partially revert number of
commits that addressed issues with managing the concurrent requests for
scanning.

The design goal here is to have main focus on cases that happen without
conflicting ctrl_iface commands (e.g., P2P_FIND immediately after SCAN)
and to make sure that those conflicting cases go through eventually,
even if not in ideal order.
Jouni Malinen - Dec. 25, 2012, 10:55 a.m.
On Tue, Dec 11, 2012 at 07:21:01AM +0000, Neeraj Kumar Garg wrote:
> Any updates on this below email? My point is do we really need the code which I removed in my patch? I am not able to generate a problematic log now (where I could see that we were continuously scheduling the STA scan) but this segment of code is little problematic. In my understanding, I am not very clear why we need this segment of code. Same piece of code to schedule p2p_scan is already present after handling the STA scan results.

That code to stop station scans sounds like a reasonable thing to do to
act on the last ctrl_iface command. I would like to see a debug log or
clear description of why this causes harm before removing it or at
least a pair of debug logs with the current implementation and then with
the section in _wpa_supplicant_event_scan_results() removed for cases
where you see benefit from removing the skip-scan-results-to-start-P2P
step here.

> [NG] last command issued is fine but STA scan was already successfully completed. We could have easily handled the scan results and schedule the p2p search after handling the STA scan results.

Earlier scan command could result in additional operations like
association and authentication to be started. Those can take quite some
time to complete and wpa_supplicant tries to avoid the extra latency by
skipping that here if a new P2P operation has been initiated.

> [NG] Order is fine. The commands are working fine in any order. SCAN followed by p2p_find or p2p_find followed by SCAN. But in one race condition, once I had seen that we were continuously scheduling the STA scan unnecessarily. My point is which scenario, this particular code is taking care provided that we have the almost same code to schedule p2p_search after handling the scan results.

If you can reproduce this and provide a debug log, I'm obviously fine
with a fix to address such a case. I've yet to see a clear description
on how this could be triggered.

Patch

diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
old mode 100644
new mode 100755
index 53b8338..68bf3eb
--- a/wpa_supplicant/events.c
+++ b/wpa_supplicant/events.c
@@ -1070,19 +1070,6 @@  static int _wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
 	wpa_supplicant_notify_scanning(wpa_s, 0);
 
 #ifdef CONFIG_P2P
-	if (wpa_s->global->p2p_cb_on_scan_complete &&
-	    !wpa_s->global->p2p_disabled &&
-	    wpa_s->global->p2p != NULL && !wpa_s->sta_scan_pending &&
-	    !wpa_s->scan_res_handler) {
-		wpa_s->global->p2p_cb_on_scan_complete = 0;
-		if (p2p_other_scan_completed(wpa_s->global->p2p) == 1) {
-			wpa_dbg(wpa_s, MSG_DEBUG, "P2P: Pending P2P operation "
-				"stopped scan processing");
-			wpa_s->sta_scan_pending = 1;
-			wpa_supplicant_req_scan(wpa_s, 5, 0);
-			return -1;
-		}
-	}
 	wpa_s->sta_scan_pending = 0;
 #endif /* CONFIG_P2P */