Message ID | 20140214191608.GA22228@w1.fi |
---|---|
State | Superseded |
Headers | show |
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
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;