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

Jithu Jance jithujance at gmail.com
Tue Feb 18 08:37:26 EST 2014


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 at 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 at 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 at lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap


More information about the HostAP mailing list