[PATCH 1/2] P2P: Addressing few issues seen with Broadcast SD
Jithu Jance
jithujance
Tue Feb 18 05:37:26 PST 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