| Message ID | F764D07D6734DF489C733939E1A6F85A1BD86F5D@SJEXCHMB12.corp.ad.broadcom.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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.
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.
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.
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 */