[1/1] P2P: Avoid resetting pending_listen_freq if p2p_listen is pending

> > + p2p->pending_listen_freq = freq;
> > + p2p->pending_listen_sec = timeout / 1000;
> > + p2p->pending_listen_usec = (timeout % 1000) * 1000;
>
> It's not shown in the context here, but this moving of
> p2p->pending_listen_{sec,usec} would lose information from the
> p2p->p2p_scan_running case. That uses p2p->start_after_scan to
> postpone
> start of the listen operation and p2p_run_after_scan() expects
> p2p->pending_listen_{sec,usec} to be set fror the
> P2P_AFTER_SCAN_LISTEN
> case.
>

I re-organised the code to take care of this case. Hope this is
fine now.


If p2p_listen is called while previous listen command's
remain_on_channel event is pending, the p2p_listen would fail
and it used to clear pending_listen_freq. Now when the remain-
on-channel event comes from the driver, the pending_listen_freq
doesn't match and gets ignored. This was leading to a case
where listen state was getting stuck (in case of WAIT_PEER_CONNECT
state).

Signed-off-by: Jithu Jance <jithu@broadcom.com>
---
 src/p2p/p2p.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

--
1.7.9.5




- Jithu Jance

On Thu, May 22, 2014 at 12:25:53PM +0530, Jithu Jance wrote:
> I re-organised the code to take care of this case. Hope this is
> fine now.

While this can pass all the hwsim test cases one-by-one separately,
there are still some errors being triggered with "P2P: p2p_listen
command pending already" dropping a P2P_LISTEN command and
wpa_supplicant ending up in state where it is not doing anything after
the previous started operation (e.g., remain-on-channel) completes.

> diff --git a/src/p2p/p2p.c b/src/p2p/p2p.c
> @@ -284,16 +290,21 @@ int p2p_listen(struct p2p_data *p2p, unsigned int timeout)
> 
>  	p2p_dbg(p2p, "Going to listen(only) state");
> 
> +	p2p->pending_listen_sec = timeout / 1000;
> +	p2p->pending_listen_usec = (timeout % 1000) * 1000;
> +
> +	if (p2p->pending_listen_freq) {
> +		/* We have a pending p2p_listen request */
> +		p2p_dbg(p2p, "p2p_listen command pending already");
> +		return -1;
> +	}
> +
>  	freq = p2p_channel_to_freq(p2p->cfg->reg_class, p2p->cfg->channel);
>  	if (freq < 0) {
>  		p2p_dbg(p2p, "Unknown regulatory class/channel");
>  		return -1;
>  	}

This looks a bit odd. Why would either of those return cases update
pending_listen_{sec,usec}? I moved those two back here:

> -	p2p->pending_listen_freq = freq;
> -	p2p->pending_listen_sec = timeout / 1000;
> -	p2p->pending_listen_usec = (timeout % 1000) * 1000;

In other words, only pending_listen_freq moves down to here:

> @@ -308,6 +319,8 @@ int p2p_listen(struct p2p_data *p2p, unsigned int timeout)
>  	if (ies == NULL)
>  		return -1;
> 
> +	p2p->pending_listen_freq = freq;
> +
>  	if (p2p->cfg->start_listen(p2p->cfg->cb_ctx, freq, timeout, ies) < 0) {
>  		p2p_dbg(p2p, "Failed to start listen mode");
>  		p2p->pending_listen_freq = 0;


That said, something is clearly still leaving p2p->pending_listen_freq
set to non-zero value while allowing the previous operation to
terminate. One example that I'm looking at is from
grpform_no_5ghz_add_cli failure case where P2P_FIND is first started and
P2P_LISTEN is then used to move listen-only state. This results in the
previously started p2p_find getting stopped, but p2p_listen execution is
still skipping scheduling of a new operation due to pending_listen_freq
(which is likely left set by an earlier test case).

Maybe p2p_stop_find() would need to clear pending_listen_freq to avoid
this? In this particular error case, the previous find operation is
indeed stopped by wpa_p2p_listen() calling p2p_stop_find just before
calling p2p_listen().

On Thu, May 22, 2014 at 03:54:30PM +0300, Jouni Malinen wrote:
> Maybe p2p_stop_find() would need to clear pending_listen_freq to avoid
> this? In this particular error case, the previous find operation is
> indeed stopped by wpa_p2p_listen() calling p2p_stop_find just before
> calling p2p_listen().

That seems to have done the trick - two full test runs completed without
any failures. I have this in the pending branch now:
http://w1.fi/cgit/hostap/commit/?h=pending&id=5661bd0f706b542bdc3b35df5b2b391f277e1275

Hi Jouni,

> On Thu, May 22, 2014 at 03:54:30PM +0300, Jouni Malinen wrote:
>> Maybe p2p_stop_find() would need to clear pending_listen_freq to avoid
>> this? In this particular error case, the previous find operation is
>> indeed stopped by wpa_p2p_listen() calling p2p_stop_find just before
>> calling p2p_listen().
>
> That seems to have done the trick - two full test runs completed without
> any failures. I have this in the pending branch now:
> http://w1.fi/cgit/hostap/commit/?h=pending&id=5661bd0f706b542bdc3b35df5b2b391f277e1275
>
Thanks a lot for nailing this down!!