[1/2] P2P: Addressing few issues seen with Broadcast SD

On Tue, Feb 04, 2014 at 02:39:00PM +0530, Jithu Jance wrote:
> 1) Suppose we have multiple peers and we have peers advertising
> SD capability, but no services registered for adverstising.
> In this case, even if there are mutliple broadcast queries set,
> we might end up sending only the lastly added Broadcast to the
> same device (Since SD_INFO won't get set for the first broadcast)
> 
> 2) Some times it is seen that before advancing to next device in the
> list, the scan results come and updates SD_SCHEDULE flag. This will
> result in sending the already sent query to the same device without
> giving chance to other devices. This issue again is seen with peer
> devices advertising SD capability without any services registered.

I agree that these issues need to be resolved, but there are still some
issues with the changes here. This breaks couple of the SD test cases:
http://buildbot.w1.fi/hwsim/results.php?run=1392400340
And as such, I cannot apply this.

Please note that I merged in 2/2 to make a single commit (there is no
point in introducing a bug that gets fixed in the second patch in the
same set). In addition, I cleaned up the partial removal of
P2P_DEV_SD_INFO BIT and P2P_DEV_SD_SCHEDULE by removing them completely
since they were either read-only or write-only and did not affect any
operations after the changes.

I have not yet tried to figure out why this breaks things, but anyway,
this cleaned up version would be a good next baseline to use to figure
out what needs to be changed to make sure this does not add any
regressions.


[PATCH] P2P: Address few issues seen with Broadcast SD

1) Suppose we have multiple peers and we have peers advertising SD
capability, but no services registered for advertising. In this case,
even if there are multiple broadcast queries set, we might end up
sending only the last added broadcast query to the same device (Since
SD_INFO won't get set for the first broadcast query).

2) Some times it is seen that before advancing to the next device in the
list, the scan results come and update SD_SCHEDULE flag. This will
result in sending the already sent query to the same device without
giving chance to other devices. This issue again is seen with peer
devices advertising SD capability without any services registered.

Signed-hostap: Jithu Jance <jithu@broadcom.com>
---
 src/p2p/p2p.c    |   22 +++++++++----------
 src/p2p/p2p_i.h  |    9 ++++++--
 src/p2p/p2p_sd.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 73 insertions(+), 22 deletions(-)

Hi Jouni,

>but there are still some
>issues with the changes here. This breaks couple of the SD test cases:
Strangely, all SD related tests were passing for me. But now I see
that two tests are failing. Looks like I messed up something while
testing.  I will check this.

> Please note that I merged in 2/2 to make a single commit
> P2P_DEV_SD_INFO BIT and P2P_DEV_SD_SCHEDULE by removing them completely
> this cleaned up version would be a good next baseline to use to figure
> out what needs to be changed to make sure this does not add any
> regressions.
Thanks a lot!. I will try check the regression with this base-line patch.


Jithu Jance





On Sat, Feb 15, 2014 at 12:46 AM, Jouni Malinen <j@w1.fi> wrote:
>
> On Tue, Feb 04, 2014 at 02:39:00PM +0530, Jithu Jance wrote:
> > 1) Suppose we have multiple peers and we have peers advertising
> > SD capability, but no services registered for adverstising.
> > In this case, even if there are mutliple broadcast queries set,
> > we might end up sending only the lastly added Broadcast to the
> > same device (Since SD_INFO won't get set for the first broadcast)
> >
> > 2) Some times it is seen that before advancing to next device in the
> > list, the scan results come and updates SD_SCHEDULE flag. This will
> > result in sending the already sent query to the same device without
> > giving chance to other devices. This issue again is seen with peer
> > devices advertising SD capability without any services registered.
>
> I agree that these issues need to be resolved, but there are still some
> issues with the changes here. This breaks couple of the SD test cases:
> http://buildbot.w1.fi/hwsim/results.php?run=1392400340
> And as such, I cannot apply this.
>
> Please note that I merged in 2/2 to make a single commit (there is no
> point in introducing a bug that gets fixed in the second patch in the
> same set). In addition, I cleaned up the partial removal of
> P2P_DEV_SD_INFO BIT and P2P_DEV_SD_SCHEDULE by removing them completely
> since they were either read-only or write-only and did not affect any
> operations after the changes.
>
> I have not yet tried to figure out why this breaks things, but anyway,
> this cleaned up version would be a good next baseline to use to figure
> out what needs to be changed to make sure this does not add any
> regressions.
>
>
> [PATCH] P2P: Address few issues seen with Broadcast SD
>
> 1) Suppose we have multiple peers and we have peers advertising SD
> capability, but no services registered for advertising. In this case,
> even if there are multiple broadcast queries set, we might end up
> sending only the last added broadcast query to the same device (Since
> SD_INFO won't get set for the first broadcast query).
>
> 2) Some times it is seen that before advancing to the next device in the
> list, the scan results come and update SD_SCHEDULE flag. This will
> result in sending the already sent query to the same device without
> giving chance to other devices. This issue again is seen with peer
> devices advertising SD capability without any services registered.
>
> Signed-hostap: Jithu Jance <jithu@broadcom.com>
> ---
>  src/p2p/p2p.c    |   22 +++++++++----------
>  src/p2p/p2p_i.h  |    9 ++++++--
>  src/p2p/p2p_sd.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 73 insertions(+), 22 deletions(-)
>
> diff --git a/src/p2p/p2p.c b/src/p2p/p2p.c
> index 3010377..5a33b64 100644
> --- a/src/p2p/p2p.c
> +++ b/src/p2p/p2p.c
> @@ -733,9 +733,6 @@ int p2p_add_device(struct p2p_data *p2p, const u8 *addr, int freq,
>
>         p2p_parse_free(&msg);
>
> -       if (p2p_pending_sd_req(p2p, dev))
> -               dev->flags |= P2P_DEV_SD_SCHEDULE;
> -
>         if (dev->flags & P2P_DEV_REPORTED)
>                 return 0;
>
> @@ -2392,6 +2389,7 @@ struct p2p_data * p2p_init(const struct p2p_config *cfg)
>
>         p2p->go_timeout = 100;
>         p2p->client_timeout = 20;
> +       p2p->num_p2p_sd_queries = 0;
>
>         p2p_dbg(p2p, "initialized");
>         p2p_channels_dump(p2p, "channels", &p2p->cfg->channels);
> @@ -2627,7 +2625,13 @@ void p2p_continue_find(struct p2p_data *p2p)
>         struct p2p_device *dev;
>         p2p_set_state(p2p, P2P_SEARCH);
>         dl_list_for_each(dev, &p2p->devices, struct p2p_device, list) {
> -               if (dev->flags & P2P_DEV_SD_SCHEDULE) {
> +               if (dev->sd_pending_bcast_queries == 0) {
> +                       /* Initialize with total number of registered broadcast
> +                        * SD queries. */
> +                       dev->sd_pending_bcast_queries = p2p->num_p2p_sd_queries;
> +               }
> +
> +               if (dev->sd_pending_bcast_queries > 0) {
>                         if (p2p_start_sd(p2p, dev) == 0)
>                                 return;
>                         else
> @@ -2654,10 +2658,8 @@ static void p2p_sd_cb(struct p2p_data *p2p, int success)
>         p2p->pending_action_state = P2P_NO_PENDING_ACTION;
>
>         if (!success) {
> -               if (p2p->sd_peer) {
> -                       p2p->sd_peer->flags &= ~P2P_DEV_SD_SCHEDULE;
> +               if (p2p->sd_peer)
>                         p2p->sd_peer = NULL;
> -               }
>                 p2p_continue_find(p2p);
>                 return;
>         }
> @@ -3202,7 +3204,6 @@ static void p2p_timeout_sd_during_find(struct p2p_data *p2p)
>         p2p_dbg(p2p, "Service Discovery Query timeout");
>         if (p2p->sd_peer) {
>                 p2p->cfg->send_action_done(p2p->cfg->cb_ctx);
> -               p2p->sd_peer->flags &= ~P2P_DEV_SD_SCHEDULE;
>                 p2p->sd_peer = NULL;
>         }
>         p2p_continue_find(p2p);
> @@ -3473,7 +3474,7 @@ int p2p_get_peer_info_txt(const struct p2p_peer_info *info,
>                           "country=%c%c\n"
>                           "oper_freq=%d\n"
>                           "req_config_methods=0x%x\n"
> -                         "flags=%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n"
> +                         "flags=%s%s%s%s%s%s%s%s%s%s%s%s%s\n"
>                           "status=%d\n"
>                           "wait_count=%u\n"
>                           "invitation_reqs=%u\n",
> @@ -3496,9 +3497,6 @@ int p2p_get_peer_info_txt(const struct p2p_peer_info *info,
>                           dev->flags & P2P_DEV_REPORTED ? "[REPORTED]" : "",
>                           dev->flags & P2P_DEV_NOT_YET_READY ?
>                           "[NOT_YET_READY]" : "",
> -                         dev->flags & P2P_DEV_SD_INFO ? "[SD_INFO]" : "",
> -                         dev->flags & P2P_DEV_SD_SCHEDULE ? "[SD_SCHEDULE]" :
> -                         "",
>                           dev->flags & P2P_DEV_PD_PEER_DISPLAY ?
>                           "[PD_PEER_DISPLAY]" : "",
>                           dev->flags & P2P_DEV_PD_PEER_KEYPAD ?
> diff --git a/src/p2p/p2p_i.h b/src/p2p/p2p_i.h
> index f105083..6de3461 100644
> --- a/src/p2p/p2p_i.h
> +++ b/src/p2p/p2p_i.h
> @@ -81,8 +81,6 @@ struct p2p_device {
>  #define P2P_DEV_PROBE_REQ_ONLY BIT(0)
>  #define P2P_DEV_REPORTED BIT(1)
>  #define P2P_DEV_NOT_YET_READY BIT(2)
> -#define P2P_DEV_SD_INFO BIT(3)
> -#define P2P_DEV_SD_SCHEDULE BIT(4)
>  #define P2P_DEV_PD_PEER_DISPLAY BIT(5)
>  #define P2P_DEV_PD_PEER_KEYPAD BIT(6)
>  #define P2P_DEV_USER_REJECTED BIT(7)
> @@ -110,6 +108,7 @@ struct p2p_device {
>
>         u8 go_timeout;
>         u8 client_timeout;
> +       int sd_pending_bcast_queries;
>  };
>
>  struct p2p_sd_query {
> @@ -256,6 +255,12 @@ struct p2p_data {
>          */
>         struct p2p_sd_query *sd_query;
>
> +       /**
> +        * num_p2p_sd_queries - Total number of broadcast SD queries present in
> +        * the list
> +        */
> +       int num_p2p_sd_queries;
> +
>         /* GO Negotiation data */
>
>         /**
> diff --git a/src/p2p/p2p_sd.c b/src/p2p/p2p_sd.c
> index 0e0c7f1..b7509da 100644
> --- a/src/p2p/p2p_sd.c
> +++ b/src/p2p/p2p_sd.c
> @@ -52,6 +52,7 @@ struct p2p_sd_query * p2p_pending_sd_req(struct p2p_data *p2p,
>  {
>         struct p2p_sd_query *q;
>         int wsd = 0;
> +       int count = 0;
>
>         if (!(dev->info.dev_capab & P2P_DEV_CAPAB_SERVICE_DISCOVERY))
>                 return NULL; /* peer does not support SD */
> @@ -64,8 +65,17 @@ struct p2p_sd_query * p2p_pending_sd_req(struct p2p_data *p2p,
>                 /* Use WSD only if the peer indicates support or it */
>                 if (q->wsd && !wsd)
>                         continue;
> -               if (q->for_all_peers && !(dev->flags & P2P_DEV_SD_INFO))
> -                       return q;
> +               /* if the query is a broadcast query */
> +               if (q->for_all_peers) {
> +                       /* check if there are any broadcast queries pending for
> +                        * this device */
> +                       if (dev->sd_pending_bcast_queries <= 0)
> +                               return NULL;
> +                       /* query number that needs to be send to the device */
> +                       if (count == dev->sd_pending_bcast_queries - 1)
> +                               return q;
> +                       count++;
> +               }
>                 if (!q->for_all_peers &&
>                     os_memcmp(q->peer, dev->info.p2p_device_addr, ETH_ALEN) ==
>                     0)
> @@ -76,14 +86,36 @@ struct p2p_sd_query * p2p_pending_sd_req(struct p2p_data *p2p,
>  }
>
>
> +static void p2p_decrease_sd_bc_queries(struct p2p_data *p2p, int query_number)
> +{
> +       struct p2p_device *dev;
> +
> +       p2p->num_p2p_sd_queries--;
> +       dl_list_for_each(dev, &p2p->devices, struct p2p_device, list) {
> +               if (query_number <= dev->sd_pending_bcast_queries - 1) {
> +                       /*
> +                        * Query not yet sent to the device and it is to be
> +                        * removed, so update the pending count.
> +                        */
> +                       dev->sd_pending_bcast_queries--;
> +               }
> +       }
> +}
> +
> +
>  static int p2p_unlink_sd_query(struct p2p_data *p2p,
>                                struct p2p_sd_query *query)
>  {
>         struct p2p_sd_query *q, *prev;
> +       int query_number = 0;
>         q = p2p->sd_queries;
>         prev = NULL;
>         while (q) {
>                 if (q == query) {
> +                       /* If the query is a broadcast query, decrease one from
> +                        * all the devices */
> +                       if (query->for_all_peers)
> +                               p2p_decrease_sd_bc_queries(p2p, query_number);
>                         if (prev)
>                                 prev->next = q->next;
>                         else
> @@ -92,6 +124,8 @@ static int p2p_unlink_sd_query(struct p2p_data *p2p,
>                                 p2p->sd_query = NULL;
>                         return 1;
>                 }
> +               if (q->for_all_peers)
> +                       query_number++;
>                 prev = q;
>                 q = q->next;
>         }
> @@ -118,6 +152,7 @@ void p2p_free_sd_queries(struct p2p_data *p2p)
>                 q = q->next;
>                 p2p_free_sd_query(prev);
>         }
> +       p2p->num_p2p_sd_queries = 0;
>  }
>
>
> @@ -262,6 +297,16 @@ int p2p_start_sd(struct p2p_data *p2p, struct p2p_device *dev)
>                 ret = -1;
>         }
>
> +       /* Update the pending broadcast SD query count for this device */
> +       dev->sd_pending_bcast_queries--;
> +
> +       /*
> +        * If there are no pending broadcast queries for this device, mark it as
> +        * done (-1).
> +        */
> +       if (dev->sd_pending_bcast_queries == 0)
> +               dev->sd_pending_bcast_queries = -1;
> +
>         wpabuf_free(req);
>
>         return ret;
> @@ -541,8 +586,6 @@ void p2p_rx_gas_initial_resp(struct p2p_data *p2p, const u8 *sa,
>         p2p_dbg(p2p, "Service Update Indicator: %u", update_indic);
>         pos += 2;
>
> -       p2p->sd_peer->flags |= P2P_DEV_SD_INFO;
> -       p2p->sd_peer->flags &= ~P2P_DEV_SD_SCHEDULE;
>         p2p->sd_peer = NULL;
>
>         if (p2p->sd_query) {
> @@ -787,8 +830,6 @@ skip_nqp_header:
>                 return;
>         }
>
> -       p2p->sd_peer->flags |= P2P_DEV_SD_INFO;
> -       p2p->sd_peer->flags &= ~P2P_DEV_SD_SCHEDULE;
>         p2p->sd_peer = NULL;
>
>         if (p2p->sd_query) {
> @@ -841,8 +882,15 @@ void * p2p_sd_request(struct p2p_data *p2p, const u8 *dst,
>
>         if (dst == NULL) {
>                 struct p2p_device *dev;
> -               dl_list_for_each(dev, &p2p->devices, struct p2p_device, list)
> -                       dev->flags &= ~P2P_DEV_SD_INFO;
> +               p2p->num_p2p_sd_queries++;
> +
> +               /* Update all the devices for the newly added broadcast query */
> +               dl_list_for_each(dev, &p2p->devices, struct p2p_device, list) {
> +                       if (dev->sd_pending_bcast_queries <= 0)
> +                               dev->sd_pending_bcast_queries = 1;
> +                       else
> +                               dev->sd_pending_bcast_queries++;
> +               }
>         }
>
>         return q;
> --
> 1.7.9.5
>
>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap