Message ID | 20230728053153.584222-6-simon.mikuda@streamunlimited.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Device pairing and discovery fixes | expand |
Context | Check | Description |
---|---|---|
tedd_an/pre-ci_am | success | Success |
tedd_an/CheckPatch | warning | WARNING:LONG_LINE_COMMENT: line length of 81 exceeds 80 columns #185: FILE: src/device.c:2795: + * device->state.browse is set and "InProgress" error is returned instead WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line #349: FILE: src/device.c:6406: + * timer */ /github/workspace/src/src/13331218.patch total: 0 errors, 2 warnings, 268 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13331218.patch has style problems, please review. NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
tedd_an/GitLint | success | Gitlint PASS |
tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Luiz. Do you plan to review and merge also this commit series? I already updated it according to your message: Perhaps in case of discovery we could do both in parallel, although if the remote side supports GATT services as part of SDP we may end up with redundant discovery but bt_gatt_client/gatt_db shall be able to handle that. Thank you. BR, Simon On 28. 7. 2023 7:31, Simon Mikuda wrote: > There can be situation that after connection to LE bearer we will try > to start reverse discovery on BR/EDR bearer even when it is not connected. > > This change separates SDP and GATT reverse services discoveries to their > respective bearers. SDP to BR/EDR and GATT to LE. > --- > src/device.c | 129 +++++++++++++++++++++++++++++++++------------------ > 1 file changed, 83 insertions(+), 46 deletions(-) > > diff --git a/src/device.c b/src/device.c > index 367e2f046..90d6a7615 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -158,6 +158,10 @@ struct bearer_state { > bool initiator; > bool connectable; > time_t last_seen; > + /* reverse service discovery timer */ > + unsigned int discov_timer; > + /* service discover request (SDP, GATT) */ > + struct browse_req *browse; > }; > > struct ltk_info { > @@ -236,9 +240,7 @@ struct btd_device { > bool temporary; > bool connectable; > unsigned int disconn_timer; > - unsigned int discov_timer; > unsigned int temporary_timer; /* Temporary/disappear timer */ > - struct browse_req *browse; /* service discover request */ > struct bonding_req *bonding; > struct authentication_req *authr; /* authentication request */ > GSList *disconnects; /* disconnects message */ > @@ -684,8 +686,10 @@ static void browse_request_free(struct browse_req *req) > { > struct btd_device *device = req->device; > > - if (device->browse == req) > - device->browse = NULL; > + if (device->bredr_state.browse == req) > + device->bredr_state.browse = NULL; > + if (device->le_state.browse == req) > + device->le_state.browse = NULL; > > if (req->listener_id) > g_dbus_remove_watch(dbus_conn, req->listener_id); > @@ -833,8 +837,10 @@ static void device_free(gpointer user_data) > if (device->disconn_timer) > timeout_remove(device->disconn_timer); > > - if (device->discov_timer) > - timeout_remove(device->discov_timer); > + if (device->bredr_state.discov_timer) > + timeout_remove(device->bredr_state.discov_timer); > + if (device->le_state.discov_timer) > + timeout_remove(device->le_state.discov_timer); > > if (device->temporary_timer) > timeout_remove(device->temporary_timer); > @@ -1848,8 +1854,10 @@ void device_request_disconnect(struct btd_device *device, DBusMessage *msg) > if (device->bonding) > bonding_request_cancel(device->bonding); > > - if (device->browse) > - browse_request_cancel(device->browse); > + if (device->bredr_state.browse) > + browse_request_cancel(device->bredr_state.browse); > + if (device->le_state.browse) > + browse_request_cancel(device->le_state.browse); > > if (device->att_io) { > g_io_channel_shutdown(device->att_io, FALSE, NULL); > @@ -2304,7 +2312,7 @@ void btd_device_update_allowed_services(struct btd_device *dev) > /* If service discovery is ongoing, let the service discovery complete > * callback call this function. > */ > - if (dev->browse) { > + if (dev->bredr_state.browse) { > ba2str(&dev->bdaddr, addr); > DBG("service discovery of %s is ongoing. Skip updating allowed " > "services", addr); > @@ -2370,7 +2378,7 @@ int btd_device_connect_services(struct btd_device *dev, GSList *services) > { > GSList *l; > > - if (dev->pending || dev->connect || dev->browse) > + if (dev->pending || dev->connect || dev->bredr_state.browse) > return -EBUSY; > > if (!btd_adapter_get_powered(dev->adapter)) > @@ -2401,7 +2409,7 @@ static DBusMessage *connect_profiles(struct btd_device *dev, uint8_t bdaddr_type > DBG("%s %s, client %s", dev->path, uuid ? uuid : "(all)", > dbus_message_get_sender(msg)); > > - if (dev->pending || dev->connect || dev->browse) > + if (dev->pending || dev->connect || dev->bredr_state.browse) > return btd_error_in_progress_str(msg, ERR_BREDR_CONN_BUSY); > > if (!btd_adapter_get_powered(dev->adapter)) { > @@ -2784,7 +2792,7 @@ static void browse_request_complete(struct browse_req *req, uint8_t type, > > /* if successfully resolved services we need to free browsing request > * before passing message back to connect functions, otherwise > - * device->browse is set and "InProgress" error is returned instead > + * device->state.browse is set and "InProgress" error is returned instead > * of actually connecting services > */ > msg = dbus_message_ref(req->msg); > @@ -2829,7 +2837,7 @@ static void device_svc_resolved(struct btd_device *dev, uint8_t browse_type, > uint8_t bdaddr_type, int err) > { > struct bearer_state *state = get_state(dev, bdaddr_type); > - struct browse_req *req = dev->browse; > + struct browse_req *req = state->browse; > > DBG("%s err %d", dev->path, err); > > @@ -3060,7 +3068,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg, > err = device_connect_le(device); > else > err = adapter_create_bonding(adapter, &device->bdaddr, > - device->bdaddr_type, > + bdaddr_type, > io_cap); > } else { > err = adapter_create_bonding(adapter, &device->bdaddr, > @@ -4657,8 +4665,10 @@ void device_remove(struct btd_device *device, gboolean remove_stored) > device_cancel_bonding(device, status); > } > > - if (device->browse) > - browse_request_cancel(device->browse); > + if (device->bredr_state.browse) > + browse_request_cancel(device->bredr_state.browse); > + if (device->le_state.browse) > + browse_request_cancel(device->le_state.browse); > > while (device->services != NULL) { > struct btd_service *service = device->services->data; > @@ -5317,7 +5327,7 @@ static void att_disconnected_cb(int err, void *user_data) > > DBG(""); > > - if (device->browse) > + if (device->le_state.browse) > goto done; > > DBG("%s (%d)", strerror(err), err); > @@ -5345,7 +5355,7 @@ done: > > static void register_gatt_services(struct btd_device *device) > { > - struct browse_req *req = device->browse; > + struct browse_req *req = device->le_state.browse; > GSList *services = NULL; > > if (!bt_gatt_client_is_ready(device->client)) > @@ -5636,8 +5646,8 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) > adapter_connect_list_add(device->adapter, device); > } > > - if (device->browse) > - browse_request_complete(device->browse, > + if (device->le_state.browse) > + browse_request_complete(device->le_state.browse, > BROWSE_GATT, > device->bdaddr_type, > -ECONNABORTED); > @@ -5751,15 +5761,24 @@ static struct browse_req *browse_request_new(struct btd_device *device, > DBusMessage *msg) > { > struct browse_req *req; > + struct bearer_state *state; > > - if (device->browse) > + switch (type) { > + case BROWSE_SDP: > + state = get_state(device, BDADDR_BREDR); > + break; > + case BROWSE_GATT: > + state = get_state(device, BDADDR_LE_PUBLIC); > + break; > + } > + if (state->browse) > return NULL; > > req = g_new0(struct browse_req, 1); > req->device = device; > req->type = type; > > - device->browse = req; > + state->browse = req; > > if (!msg) > return req; > @@ -5879,15 +5898,17 @@ int device_discover_services(struct btd_device *device, > uint8_t bdaddr_type, DBusMessage *msg) > { > int err; > + struct bearer_state *state; > > if (bdaddr_type == BDADDR_BREDR) > err = device_browse_sdp(device, msg); > else > err = device_browse_gatt(device, msg); > > - if (err == 0 && device->discov_timer) { > - timeout_remove(device->discov_timer); > - device->discov_timer = 0; > + state = get_state(device, bdaddr_type); > + if (err == 0 && state->discov_timer) { > + timeout_remove(state->discov_timer); > + state->discov_timer = 0; > } > > return err; > @@ -6208,16 +6229,22 @@ bool device_is_connectable(struct btd_device *device) > return state->connectable; > } > > -static bool start_discovery_cb(gpointer user_data) > +static bool start_sdp_discovery_cb(gpointer user_data) > { > struct btd_device *device = user_data; > > - if (device->bredr) > - device_browse_sdp(device, NULL); > - else > - device_browse_gatt(device, NULL); > + device->bredr_state.discov_timer = 0; > + device_browse_sdp(device, NULL); > > - device->discov_timer = 0; > + return FALSE; > +} > + > +static bool start_gatt_discovery_cb(gpointer user_data) > +{ > + struct btd_device *device = user_data; > + > + device->le_state.discov_timer = 0; > + device_browse_gatt(device, NULL); > > return FALSE; > } > @@ -6368,17 +6395,27 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type, > g_dbus_send_reply(dbus_conn, bonding->msg, DBUS_TYPE_INVALID); > } > bonding_request_free(bonding); > - } else if (!state->svc_resolved) { > - if (!device->browse && !device->discov_timer && > - btd_opts.reverse_discovery) { > - /* If we are not initiators and there is no currently > - * active discovery or discovery timer, set discovery > - * timer */ > - DBG("setting timer for reverse service discovery"); > - device->discov_timer = timeout_add_seconds( > - DISCOVERY_TIMER, > - start_discovery_cb, > - device, NULL); > + > + } else if (!state->svc_resolved > + && !state->browse > + && !state->discov_timer > + && btd_opts.reverse_discovery) { > + > + /* If we are not initiators and there is no currently > + * active discovery or discovery timer, set discovery > + * timer */ > + if (bdaddr_type == BDADDR_BREDR) { > + DBG("setting timer for reverse SDP service discovery"); > + state->discov_timer = timeout_add_seconds( > + DISCOVERY_TIMER, > + start_sdp_discovery_cb, > + device, NULL); > + } else { > + DBG("setting timer for reverse GATT service discovery"); > + state->discov_timer = timeout_add_seconds( > + DISCOVERY_TIMER, > + start_gatt_discovery_cb, > + device, NULL); > } > } > } > @@ -6416,11 +6453,11 @@ unsigned int device_wait_for_svc_complete(struct btd_device *dev, > > if (state->svc_resolved || !btd_opts.reverse_discovery) > cb->idle_id = g_idle_add(svc_idle_cb, cb); > - else if (dev->discov_timer > 0) { > - timeout_remove(dev->discov_timer); > - dev->discov_timer = timeout_add_seconds( > + else if (dev->bredr_state.discov_timer > 0) { > + timeout_remove(dev->bredr_state.discov_timer); > + dev->bredr_state.discov_timer = timeout_add_seconds( > 0, > - start_discovery_cb, > + start_sdp_discovery_cb, > dev, NULL); > } >
diff --git a/src/device.c b/src/device.c index 367e2f046..90d6a7615 100644 --- a/src/device.c +++ b/src/device.c @@ -158,6 +158,10 @@ struct bearer_state { bool initiator; bool connectable; time_t last_seen; + /* reverse service discovery timer */ + unsigned int discov_timer; + /* service discover request (SDP, GATT) */ + struct browse_req *browse; }; struct ltk_info { @@ -236,9 +240,7 @@ struct btd_device { bool temporary; bool connectable; unsigned int disconn_timer; - unsigned int discov_timer; unsigned int temporary_timer; /* Temporary/disappear timer */ - struct browse_req *browse; /* service discover request */ struct bonding_req *bonding; struct authentication_req *authr; /* authentication request */ GSList *disconnects; /* disconnects message */ @@ -684,8 +686,10 @@ static void browse_request_free(struct browse_req *req) { struct btd_device *device = req->device; - if (device->browse == req) - device->browse = NULL; + if (device->bredr_state.browse == req) + device->bredr_state.browse = NULL; + if (device->le_state.browse == req) + device->le_state.browse = NULL; if (req->listener_id) g_dbus_remove_watch(dbus_conn, req->listener_id); @@ -833,8 +837,10 @@ static void device_free(gpointer user_data) if (device->disconn_timer) timeout_remove(device->disconn_timer); - if (device->discov_timer) - timeout_remove(device->discov_timer); + if (device->bredr_state.discov_timer) + timeout_remove(device->bredr_state.discov_timer); + if (device->le_state.discov_timer) + timeout_remove(device->le_state.discov_timer); if (device->temporary_timer) timeout_remove(device->temporary_timer); @@ -1848,8 +1854,10 @@ void device_request_disconnect(struct btd_device *device, DBusMessage *msg) if (device->bonding) bonding_request_cancel(device->bonding); - if (device->browse) - browse_request_cancel(device->browse); + if (device->bredr_state.browse) + browse_request_cancel(device->bredr_state.browse); + if (device->le_state.browse) + browse_request_cancel(device->le_state.browse); if (device->att_io) { g_io_channel_shutdown(device->att_io, FALSE, NULL); @@ -2304,7 +2312,7 @@ void btd_device_update_allowed_services(struct btd_device *dev) /* If service discovery is ongoing, let the service discovery complete * callback call this function. */ - if (dev->browse) { + if (dev->bredr_state.browse) { ba2str(&dev->bdaddr, addr); DBG("service discovery of %s is ongoing. Skip updating allowed " "services", addr); @@ -2370,7 +2378,7 @@ int btd_device_connect_services(struct btd_device *dev, GSList *services) { GSList *l; - if (dev->pending || dev->connect || dev->browse) + if (dev->pending || dev->connect || dev->bredr_state.browse) return -EBUSY; if (!btd_adapter_get_powered(dev->adapter)) @@ -2401,7 +2409,7 @@ static DBusMessage *connect_profiles(struct btd_device *dev, uint8_t bdaddr_type DBG("%s %s, client %s", dev->path, uuid ? uuid : "(all)", dbus_message_get_sender(msg)); - if (dev->pending || dev->connect || dev->browse) + if (dev->pending || dev->connect || dev->bredr_state.browse) return btd_error_in_progress_str(msg, ERR_BREDR_CONN_BUSY); if (!btd_adapter_get_powered(dev->adapter)) { @@ -2784,7 +2792,7 @@ static void browse_request_complete(struct browse_req *req, uint8_t type, /* if successfully resolved services we need to free browsing request * before passing message back to connect functions, otherwise - * device->browse is set and "InProgress" error is returned instead + * device->state.browse is set and "InProgress" error is returned instead * of actually connecting services */ msg = dbus_message_ref(req->msg); @@ -2829,7 +2837,7 @@ static void device_svc_resolved(struct btd_device *dev, uint8_t browse_type, uint8_t bdaddr_type, int err) { struct bearer_state *state = get_state(dev, bdaddr_type); - struct browse_req *req = dev->browse; + struct browse_req *req = state->browse; DBG("%s err %d", dev->path, err); @@ -3060,7 +3068,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg, err = device_connect_le(device); else err = adapter_create_bonding(adapter, &device->bdaddr, - device->bdaddr_type, + bdaddr_type, io_cap); } else { err = adapter_create_bonding(adapter, &device->bdaddr, @@ -4657,8 +4665,10 @@ void device_remove(struct btd_device *device, gboolean remove_stored) device_cancel_bonding(device, status); } - if (device->browse) - browse_request_cancel(device->browse); + if (device->bredr_state.browse) + browse_request_cancel(device->bredr_state.browse); + if (device->le_state.browse) + browse_request_cancel(device->le_state.browse); while (device->services != NULL) { struct btd_service *service = device->services->data; @@ -5317,7 +5327,7 @@ static void att_disconnected_cb(int err, void *user_data) DBG(""); - if (device->browse) + if (device->le_state.browse) goto done; DBG("%s (%d)", strerror(err), err); @@ -5345,7 +5355,7 @@ done: static void register_gatt_services(struct btd_device *device) { - struct browse_req *req = device->browse; + struct browse_req *req = device->le_state.browse; GSList *services = NULL; if (!bt_gatt_client_is_ready(device->client)) @@ -5636,8 +5646,8 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) adapter_connect_list_add(device->adapter, device); } - if (device->browse) - browse_request_complete(device->browse, + if (device->le_state.browse) + browse_request_complete(device->le_state.browse, BROWSE_GATT, device->bdaddr_type, -ECONNABORTED); @@ -5751,15 +5761,24 @@ static struct browse_req *browse_request_new(struct btd_device *device, DBusMessage *msg) { struct browse_req *req; + struct bearer_state *state; - if (device->browse) + switch (type) { + case BROWSE_SDP: + state = get_state(device, BDADDR_BREDR); + break; + case BROWSE_GATT: + state = get_state(device, BDADDR_LE_PUBLIC); + break; + } + if (state->browse) return NULL; req = g_new0(struct browse_req, 1); req->device = device; req->type = type; - device->browse = req; + state->browse = req; if (!msg) return req; @@ -5879,15 +5898,17 @@ int device_discover_services(struct btd_device *device, uint8_t bdaddr_type, DBusMessage *msg) { int err; + struct bearer_state *state; if (bdaddr_type == BDADDR_BREDR) err = device_browse_sdp(device, msg); else err = device_browse_gatt(device, msg); - if (err == 0 && device->discov_timer) { - timeout_remove(device->discov_timer); - device->discov_timer = 0; + state = get_state(device, bdaddr_type); + if (err == 0 && state->discov_timer) { + timeout_remove(state->discov_timer); + state->discov_timer = 0; } return err; @@ -6208,16 +6229,22 @@ bool device_is_connectable(struct btd_device *device) return state->connectable; } -static bool start_discovery_cb(gpointer user_data) +static bool start_sdp_discovery_cb(gpointer user_data) { struct btd_device *device = user_data; - if (device->bredr) - device_browse_sdp(device, NULL); - else - device_browse_gatt(device, NULL); + device->bredr_state.discov_timer = 0; + device_browse_sdp(device, NULL); - device->discov_timer = 0; + return FALSE; +} + +static bool start_gatt_discovery_cb(gpointer user_data) +{ + struct btd_device *device = user_data; + + device->le_state.discov_timer = 0; + device_browse_gatt(device, NULL); return FALSE; } @@ -6368,17 +6395,27 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type, g_dbus_send_reply(dbus_conn, bonding->msg, DBUS_TYPE_INVALID); } bonding_request_free(bonding); - } else if (!state->svc_resolved) { - if (!device->browse && !device->discov_timer && - btd_opts.reverse_discovery) { - /* If we are not initiators and there is no currently - * active discovery or discovery timer, set discovery - * timer */ - DBG("setting timer for reverse service discovery"); - device->discov_timer = timeout_add_seconds( - DISCOVERY_TIMER, - start_discovery_cb, - device, NULL); + + } else if (!state->svc_resolved + && !state->browse + && !state->discov_timer + && btd_opts.reverse_discovery) { + + /* If we are not initiators and there is no currently + * active discovery or discovery timer, set discovery + * timer */ + if (bdaddr_type == BDADDR_BREDR) { + DBG("setting timer for reverse SDP service discovery"); + state->discov_timer = timeout_add_seconds( + DISCOVERY_TIMER, + start_sdp_discovery_cb, + device, NULL); + } else { + DBG("setting timer for reverse GATT service discovery"); + state->discov_timer = timeout_add_seconds( + DISCOVERY_TIMER, + start_gatt_discovery_cb, + device, NULL); } } } @@ -6416,11 +6453,11 @@ unsigned int device_wait_for_svc_complete(struct btd_device *dev, if (state->svc_resolved || !btd_opts.reverse_discovery) cb->idle_id = g_idle_add(svc_idle_cb, cb); - else if (dev->discov_timer > 0) { - timeout_remove(dev->discov_timer); - dev->discov_timer = timeout_add_seconds( + else if (dev->bredr_state.discov_timer > 0) { + timeout_remove(dev->bredr_state.discov_timer); + dev->bredr_state.discov_timer = timeout_add_seconds( 0, - start_discovery_cb, + start_sdp_discovery_cb, dev, NULL); }