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

Hi Jouni,

>This seems to be causing failures in hwsim test cases. I'm not
>completely sure why, but the logs seems to imply that this case is hit
>and the P2P state machine is then stuck in waiting for something that
>never happens. It looks like this can happen at least when P2P_LISTEN is
>issued while a p2p_scan from a previously started P2P_FIND is in
>progress.

I ran hwsim and fixed a case where the listen was getting stuck (the
p2p_find case that you mentioned). Please
find the revised patch below. But when I run the hwsim there were other
failures which seems to occur with or without this patch.
I couldn't figure out the cause of the failure as the group formation seems
to succeed, but the result shows as failed. :(

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

  p2p_dbg(p2p, "Unknown regulatory class/channel");
@@ -260,14 +266,14 @@ static void p2p_listen_in_find(struct p2p_data *p2p,
int dev_disc)
  return;
  }

- p2p->pending_listen_freq = freq;
- p2p->pending_listen_sec = 0;
- p2p->pending_listen_usec = 1024 * tu;
-
  ies = p2p_build_probe_resp_ies(p2p);
  if (ies == NULL)
  return;

+ p2p->pending_listen_freq = freq;
+ p2p->pending_listen_sec = 0;
+ p2p->pending_listen_usec = 1024 * tu;
+
  if (p2p->cfg->start_listen(p2p->cfg->cb_ctx, freq, 1024 * tu / 1000,
     ies) < 0) {
  p2p_dbg(p2p, "Failed to start listen mode");
@@ -284,16 +290,18 @@ int p2p_listen(struct p2p_data *p2p, unsigned int
timeout)

  p2p_dbg(p2p, "Going to listen(only) state");

+ 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;
  }

- p2p->pending_listen_freq = freq;
- p2p->pending_listen_sec = timeout / 1000;
- p2p->pending_listen_usec = (timeout % 1000) * 1000;
-
  if (p2p->p2p_scan_running) {
  if (p2p->start_after_scan == P2P_AFTER_SCAN_CONNECT) {
  p2p_dbg(p2p, "p2p_scan running - connect is already pending - skip
listen");
@@ -308,6 +316,10 @@ int p2p_listen(struct p2p_data *p2p, unsigned int
timeout)
  if (ies == NULL)
  return -1;

+ p2p->pending_listen_freq = freq;
+ p2p->pending_listen_sec = timeout / 1000;
+ p2p->pending_listen_usec = (timeout % 1000) * 1000;
+
  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;
--
1.7.9.5





*> Jithu Jance*





On Mon, May 12, 2014 at 1:19 AM, Jouni Malinen <j@w1.fi> wrote:

> On Wed, May 07, 2014 at 02:02:06PM +0530, Jithu Jance wrote:
> > 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).
>
> > diff --git a/src/p2p/p2p.c b/src/p2p/p2p.c
> > @@ -284,6 +284,12 @@ int p2p_listen(struct p2p_data *p2p, unsigned int
> timeout)
> >       p2p_dbg(p2p, "Going to listen(only) state");
> >
> > +     if (p2p->pending_listen_freq) {
> > +             /* We have a pending p2p_listen request */
> > +             p2p_dbg(p2p, "p2p_listen command pending already");
> > +             return -1;
> > +     }
>
> This seems to be causing failures in hwsim test cases. I'm not
> completely sure why, but the logs seems to imply that this case is hit
> and the P2P state machine is then stuck in waiting for something that
> never happens. It looks like this can happen at least when P2P_LISTEN is
> issued while a p2p_scan from a previously started P2P_FIND is in
> progress.
>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
>

On Mon, May 19, 2014 at 06:04:15PM +0530, Jithu Jance wrote:
> I ran hwsim and fixed a case where the listen was getting stuck (the
> p2p_find case that you mentioned). Please
> find the revised patch below. But when I run the hwsim there were other
> failures which seems to occur with or without this patch.
> I couldn't figure out the cause of the failure as the group formation seems
> to succeed, but the result shows as failed. :(

I'm not sure whether the comments I have below addresses the failures
you saw, but there is something wrong with this patch. I did not run it
through the hwsim test cases, though.

> @@ -238,6 +238,12 @@ static void p2p_listen_in_find(struct p2p_data *p2p,
> int dev_disc)
>   p2p_dbg(p2p, "Starting short listen state (state=%s)",
>   p2p_state_txt(p2p->state));
> 
> + if (p2p->pending_listen_freq) {
> + /* We have a pending p2p_listen request */
> + p2p_dbg(p2p, "p2p_listen command pending already");
> + return;
> + }

This is whitespace damaged (tabs removed?).

> - p2p->pending_listen_freq = freq;
> - p2p->pending_listen_sec = 0;
> - p2p->pending_listen_usec = 1024 * tu;
> -
>   ies = p2p_build_probe_resp_ies(p2p);
>   if (ies == NULL)
>   return;
> 
> + p2p->pending_listen_freq = freq;
> + p2p->pending_listen_sec = 0;
> + p2p->pending_listen_usec = 1024 * tu;

This looks fine.

> @@ -284,16 +290,18 @@ int p2p_listen(struct p2p_data *p2p, unsigned int
>   p2p_dbg(p2p, "Going to listen(only) state");
> 
> + if (p2p->pending_listen_freq) {
> + /* We have a pending p2p_listen request */
> + p2p_dbg(p2p, "p2p_listen command pending already");
> + return -1;
> + }

This part would look reasonable as well on its own. However, the
following change results in issues:

> - p2p->pending_listen_freq = freq;
> - p2p->pending_listen_sec = timeout / 1000;
> - p2p->pending_listen_usec = (timeout % 1000) * 1000;
> -
>   if (p2p->p2p_scan_running) {
>   if (p2p->start_after_scan == P2P_AFTER_SCAN_CONNECT) {
>   p2p_dbg(p2p, "p2p_scan running - connect is already pending - skip
> listen");
> @@ -308,6 +316,10 @@ int p2p_listen(struct p2p_data *p2p, unsigned int
> timeout)
>   if (ies == NULL)
>   return -1;
> 
> + 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.