| Message ID | 1399827284-4893-1-git-send-email-eduardo.abinader@openbossa.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote: > Added cancel timeout and issuing formation failed, in order > to improve detection for group formation failure, upon > non-zero status. > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res) > res->status); > wpas_notify_p2p_go_neg_completed(wpa_s, res); > wpas_p2p_remove_pending_group_interface(wpa_s); > + eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL); > + wpas_p2p_group_formation_failed(wpa_s); > return; > } I can understand the wpas_p2p_group_formation_failed() call, but what is the purpose of canceling wpas_p2p_long_listen_timeout here? Actually, that same question would really apply for the earlier commit 5eae87a7d61b053fb315c667f97954d004c195a5 ('P2P: Fix GO failed interface init') as well.. That was the only place where that eloop_cancel_timeout() was introduced without clearing wpa_s->p2p_long_listen to zero. The p2p_long_listen and its timeout mechanism are used to stop long listen operation so that it does not trigger during group formation steps. Doing that in failure cases looks a bit odd. I think I'd rather remove that previously added eloop_cancel_timeout() and this one here as well, unless there is a good reason for these that I cannot currently think of. These changes would also make patch 2/2 less useful (and well, if it should be added, I'd say it should also do wpa_s->p2p_long_listen = 0).
On Mon, May 12, 2014 at 11:18 AM, Jouni Malinen <j@w1.fi> wrote: > > On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote: > > Added cancel timeout and issuing formation failed, in order > > to improve detection for group formation failure, upon > > non-zero status. > > > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c > > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res) > > res->status); > > wpas_notify_p2p_go_neg_completed(wpa_s, res); > > wpas_p2p_remove_pending_group_interface(wpa_s); > > + eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL); > > + wpas_p2p_group_formation_failed(wpa_s); > > return; > > } > > I can understand the wpas_p2p_group_formation_failed() call, but what is > the purpose of canceling wpas_p2p_long_listen_timeout here? Actually, > that same question would really apply for the earlier commit > 5eae87a7d61b053fb315c667f97954d004c195a5 ('P2P: Fix GO failed interface > init') as well.. That was the only place where that > eloop_cancel_timeout() was introduced without clearing > wpa_s->p2p_long_listen to zero. You're right. We must ensure p2p_long_listen=0. I think we could keep this cancel_timeout, in order to earlier free the timeout queue and set this p2p_long_listen=0 in patch 2/2 (just moving after the formation_failed). This would comply with p2p_stop_find_oper and p2p_listen, that do the same. Either way, keeping the timeout everything would be fine also. > > The p2p_long_listen and its timeout mechanism are used to stop long > listen operation so that it does not trigger during group formation > steps. Doing that in failure cases looks a bit odd. I think I'd rather > remove that previously added eloop_cancel_timeout() and this one here as > well, unless there is a good reason for these that I cannot currently > think of. These changes would also make patch 2/2 less useful (and well, > if it should be added, I'd say it should also do wpa_s->p2p_long_listen > = 0). > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap
On Mon, May 12, 2014 at 11:42:55AM -0400, Eduardo Abinader wrote: > On Mon, May 12, 2014 at 11:18 AM, Jouni Malinen <j@w1.fi> wrote: > > > > On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote: > > > Added cancel timeout and issuing formation failed, in order > > > to improve detection for group formation failure, upon > > > non-zero status. Actually, where does this improve detection? > > > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c > > > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res) > > > res->status); > > > wpas_notify_p2p_go_neg_completed(wpa_s, res); > > > wpas_p2p_remove_pending_group_interface(wpa_s); > > > + eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL); > > > + wpas_p2p_group_formation_failed(wpa_s); > > > return; > > > } I was trying to go through the wpas_p2p_group_formation_failed() path here and it ends up doing number of things that do not have any relevance for this specific non-zero status case: - eloop_cancel_timeout for wpas_p2p_group_formation_timeout (which has not even been started yet) - p2p_group_formation_failed (which does not do anything new since p2p_go_neg_failed() is already taking care of that before calling this callback) - wpas_group_formation_completed (which does number of items related to clearing group interface or network block which have not even been added yet). In other words, I'm clearly missing something else about this.. What does not detect that group formation failed based on the already existing notification on GO Negotiation failure? > You're right. We must ensure p2p_long_listen=0. > > I think we could keep this cancel_timeout, in order to earlier free > the timeout queue and set this p2p_long_listen=0 in patch 2/2 (just > moving after the formation_failed). This would comply with > p2p_stop_find_oper and p2p_listen, that do the same. > > Either way, keeping the timeout everything would be fine also. I was thinking of going ahead with p2p_long_listen = 0 added here, but then got to the questions above.. Clearly this would do much more than what could potentially be needed here and as such, I don't think it is necessary to call wpas_p2p_group_formation_failed(). Instead, a subset of that functionality could be added here for GO Negotiation failure case if such subset can be identified.
On Mon, May 12, 2014 at 12:29 PM, Jouni Malinen <j@w1.fi> wrote: > On Mon, May 12, 2014 at 11:42:55AM -0400, Eduardo Abinader wrote: >> On Mon, May 12, 2014 at 11:18 AM, Jouni Malinen <j@w1.fi> wrote: >> > >> > On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote: >> > > Added cancel timeout and issuing formation failed, in order >> > > to improve detection for group formation failure, upon >> > > non-zero status. > > Actually, where does this improve detection? > >> > > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c >> > > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res) >> > > res->status); >> > > wpas_notify_p2p_go_neg_completed(wpa_s, res); >> > > wpas_p2p_remove_pending_group_interface(wpa_s); >> > > + eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL); >> > > + wpas_p2p_group_formation_failed(wpa_s); >> > > return; >> > > } > > I was trying to go through the wpas_p2p_group_formation_failed() path > here and it ends up doing number of things that do not have any > relevance for this specific non-zero status case: > - eloop_cancel_timeout for wpas_p2p_group_formation_timeout (which has > not even been started yet) Actually, it depends on the scenario. I have faced issues with wps pbc, where timeout is being set. > - p2p_group_formation_failed (which does not do anything new since > p2p_go_neg_failed() is already taking care of that before calling this > callback) p2p_group_formation_failed for this particular case, 4-way handshake is not completed, and status is non-zero, it speeds up the deauth. It acutally does: I got 4 to 5 seconds faster. > - wpas_group_formation_completed (which does number of items related to > clearing group interface or network block which have not even been > added yet). > > In other words, I'm clearly missing something else about this.. What > does not detect that group formation failed based on the already > existing notification on GO Negotiation failure? > As I said, the 4-way handshake may not complete, falling under non-zero status situation. Thus the GO-neg failure may not apply. >> You're right. We must ensure p2p_long_listen=0. >> >> I think we could keep this cancel_timeout, in order to earlier free >> the timeout queue and set this p2p_long_listen=0 in patch 2/2 (just >> moving after the formation_failed). This would comply with >> p2p_stop_find_oper and p2p_listen, that do the same. >> >> Either way, keeping the timeout everything would be fine also. > > I was thinking of going ahead with p2p_long_listen = 0 added here, but > then got to the questions above.. Clearly this would do much more than > what could potentially be needed here and as such, I don't think it is > necessary to call wpas_p2p_group_formation_failed(). Instead, a subset > of that functionality could be added here for GO Negotiation failure > case if such subset can be identified. > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap
On Mon, May 12, 2014 at 01:21:04PM -0400, Eduardo Abinader wrote: > >> > > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res) > >> > > res->status); > >> > > wpas_notify_p2p_go_neg_completed(wpa_s, res); > >> > > wpas_p2p_remove_pending_group_interface(wpa_s); > >> > > + eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL); > >> > > + wpas_p2p_group_formation_failed(wpa_s); > >> > > return; > >> > > } > Actually, it depends on the scenario. I have faced issues with wps > pbc, where timeout is being set. > p2p_group_formation_failed for this particular case, 4-way handshake > is not completed, and status is non-zero, it speeds up the deauth. It > acutally does: I got 4 to 5 seconds faster. I have tried to understand this couple of times, but always have ended up giving up and leaving the patch in the queue to find a better time to look a this.. Now that I'm trying to clean all the pending items, I cannot really follow this explanation at all.. This function is wpas_go_neg_complete(). It is called at the completion of GO Negotiation which happens before there is any chance of even starting WPS PBC. As such, this does not really make any sense to me. If there is a case where this change is needed, please provide wpa_supplicant debug logs with and without this change pointing out the improvement. For now, I'll drop this patch and the 2/2 cleanup.
diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c index 522d277..62e628d 100644 --- a/wpa_supplicant/p2p_supplicant.c +++ b/wpa_supplicant/p2p_supplicant.c @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res) res->status); wpas_notify_p2p_go_neg_completed(wpa_s, res); wpas_p2p_remove_pending_group_interface(wpa_s); + eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL); + wpas_p2p_group_formation_failed(wpa_s); return; }
Added cancel timeout and issuing formation failed, in order to improve detection for group formation failure, upon non-zero status. Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org> --- wpa_supplicant/p2p_supplicant.c | 2 ++ 1 file changed, 2 insertions(+)