* [Bluez PATCH v2 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs @ 2021-03-04 20:24 Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline Daniel Winkler ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Daniel Winkler @ 2021-03-04 20:24 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Daniel Winkler Hello Maintainers, We have discovered that when userspace registers a broadcast (non-connectable) advertisement with scan response data, it exposes a limitation in the new extended MGMT APIs. At the time that the parameters are registered with the controller, kernel does not yet have the advertising data and scan response (coming in a separate MGMT call), and will default to a non-scannable PDU. When the MGMT call for data/scan response is received, the controller will either fail when we request to set the scan response, or return success and not use it. This series along with another in kernel will allow userspace to pass a flag with the params request indicating if the advertisement contains a scan response. This allows kernel to register the parameters correctly with the controller. The patch is tested with a scannable broadcast advertisement on Hatch and Kukui chromebooks (ext and non-ext capabilities) and ensuring a peripheral device can detect the scan response. Best, Daniel Changes in v2: - Check kernel supports flag before setting it Daniel Winkler (3): advertising: Generate advertising data earlier in pipeline advertising: Create and use scannable adv param flag doc/mgmt-api: Update documentation for scan_rsp param flag doc/mgmt-api.txt | 5 +++ lib/mgmt.h | 1 + src/advertising.c | 87 ++++++++++++++++++++++++++--------------------- 3 files changed, 55 insertions(+), 38 deletions(-) -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline 2021-03-04 20:24 [Bluez PATCH v2 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs Daniel Winkler @ 2021-03-04 20:24 ` Daniel Winkler 2021-03-04 22:49 ` Luiz Augusto von Dentz 2021-03-04 20:24 ` [Bluez PATCH v2 2/3] advertising: Create and use scannable adv param flag Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 3/3] doc/mgmt-api: Update documentation for scan_rsp " Daniel Winkler 2 siblings, 1 reply; 8+ messages in thread From: Daniel Winkler @ 2021-03-04 20:24 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Daniel Winkler, Alain Michaud, Sonny Sasaka This change moves the advertising data generation to the beginning of the registration pipeline. This is necessary for the following patch, which will need to know whether the scan response data is existent so that the parameter request can be populated correctly. Reviewed-by: Alain Michaud <alainm@chromium.org> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> --- Changes in v2: None src/advertising.c | 79 +++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/src/advertising.c b/src/advertising.c index 15a343e52..f3dc357a1 100644 --- a/src/advertising.c +++ b/src/advertising.c @@ -80,6 +80,10 @@ struct btd_adv_client { uint32_t flags; struct bt_ad *data; struct bt_ad *scan; + uint8_t *adv_data; + uint8_t *scan_rsp; + size_t adv_data_len; + size_t scan_rsp_len; uint8_t instance; uint32_t min_interval; uint32_t max_interval; @@ -141,6 +145,16 @@ static void client_free(void *data) bt_ad_unref(client->data); bt_ad_unref(client->scan); + if (client->adv_data) { + free(client->adv_data); + client->adv_data = NULL; + } + + if (client->scan_rsp) { + free(client->scan_rsp); + client->scan_rsp = NULL; + } + g_dbus_proxy_unref(client->proxy); if (client->owner) @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, flags |= MGMT_ADV_PARAM_TX_POWER; } + client->adv_data = generate_adv_data(client, &flags, + &client->adv_data_len); + if (!client->adv_data || + (client->adv_data_len > calc_max_adv_len(client, flags))) { + error("Advertising data too long or couldn't be generated."); + return -EINVAL; + } + + client->scan_rsp = generate_scan_rsp(client, &flags, + &client->scan_rsp_len); + if (!client->scan_rsp && client->scan_rsp_len) { + error("Scan data couldn't be generated."); + free(client->adv_data); + return -EINVAL; + } + cp.flags = htobl(flags); mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, const struct mgmt_rp_add_ext_adv_params *rp = param; struct mgmt_cp_add_ext_adv_data *cp = NULL; uint8_t param_len; - uint8_t *adv_data = NULL; - size_t adv_data_len; - uint8_t *scan_rsp = NULL; - size_t scan_rsp_len = -1; - uint32_t flags = 0; unsigned int mgmt_ret; dbus_int16_t tx_power; @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, client->instance = rp->instance; - flags = get_adv_flags(client); - - adv_data = generate_adv_data(client, &flags, &adv_data_len); - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { - error("Advertising data too long or couldn't be generated."); - goto fail; - } - - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); - if ((!scan_rsp && scan_rsp_len) || - scan_rsp_len > rp->max_scan_rsp_len) { - error("Scan data couldn't be generated."); - goto fail; - } - - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + - scan_rsp_len; + param_len = sizeof(struct mgmt_cp_add_advertising) + + client->adv_data_len + client->scan_rsp_len; cp = malloc0(param_len); if (!cp) { @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, } cp->instance = client->instance; - cp->adv_data_len = adv_data_len; - cp->scan_rsp_len = scan_rsp_len; - memcpy(cp->data, adv_data, adv_data_len); - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); - - free(adv_data); - free(scan_rsp); - adv_data = NULL; - scan_rsp = NULL; + cp->adv_data_len = client->adv_data_len; + cp->scan_rsp_len = client->scan_rsp_len; + memcpy(cp->data, client->adv_data, client->adv_data_len); + memcpy(cp->data + client->adv_data_len, client->scan_rsp, + client->scan_rsp_len); /* Submit request to update instance data */ mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, return; fail: - if (adv_data) - free(adv_data); - - if (scan_rsp) - free(scan_rsp); - if (cp) free(cp); @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, if (!client->scan) goto fail; + client->adv_data = NULL; + client->scan_rsp = NULL; + client->adv_data_len = 0; + client->scan_rsp_len = 0; + client->manager = manager; client->appearance = UINT16_MAX; client->tx_power = ADV_TX_POWER_NO_PREFERENCE; -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline 2021-03-04 20:24 ` [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline Daniel Winkler @ 2021-03-04 22:49 ` Luiz Augusto von Dentz 2021-03-04 23:27 ` Daniel Winkler 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2021-03-04 22:49 UTC (permalink / raw) To: Daniel Winkler Cc: linux-bluetooth, ChromeOS Bluetooth Upstreaming, Alain Michaud, Sonny Sasaka Hi Daniel, On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > This change moves the advertising data generation to the beginning of > the registration pipeline. This is necessary for the following patch, > which will need to know whether the scan response data is existent so > that the parameter request can be populated correctly. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > --- > > Changes in v2: None > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > 1 file changed, 42 insertions(+), 37 deletions(-) > > diff --git a/src/advertising.c b/src/advertising.c > index 15a343e52..f3dc357a1 100644 > --- a/src/advertising.c > +++ b/src/advertising.c > @@ -80,6 +80,10 @@ struct btd_adv_client { > uint32_t flags; > struct bt_ad *data; > struct bt_ad *scan; > + uint8_t *adv_data; > + uint8_t *scan_rsp; > + size_t adv_data_len; > + size_t scan_rsp_len; I'm debating if we should really just encode it early or we could just introduce something like bt_ad_length so we don't have to have copies of the same data in 2 different formats since bt_ad already represents that. > uint8_t instance; > uint32_t min_interval; > uint32_t max_interval; > @@ -141,6 +145,16 @@ static void client_free(void *data) > bt_ad_unref(client->data); > bt_ad_unref(client->scan); > > + if (client->adv_data) { > + free(client->adv_data); > + client->adv_data = NULL; > + } > + > + if (client->scan_rsp) { > + free(client->scan_rsp); > + client->scan_rsp = NULL; > + } > + > g_dbus_proxy_unref(client->proxy); > > if (client->owner) > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > flags |= MGMT_ADV_PARAM_TX_POWER; > } > > + client->adv_data = generate_adv_data(client, &flags, > + &client->adv_data_len); > + if (!client->adv_data || > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > + error("Advertising data too long or couldn't be generated."); > + return -EINVAL; > + } > + > + client->scan_rsp = generate_scan_rsp(client, &flags, > + &client->scan_rsp_len); > + if (!client->scan_rsp && client->scan_rsp_len) { > + error("Scan data couldn't be generated."); > + free(client->adv_data); > + return -EINVAL; > + } > + > cp.flags = htobl(flags); > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > const struct mgmt_rp_add_ext_adv_params *rp = param; > struct mgmt_cp_add_ext_adv_data *cp = NULL; > uint8_t param_len; > - uint8_t *adv_data = NULL; > - size_t adv_data_len; > - uint8_t *scan_rsp = NULL; > - size_t scan_rsp_len = -1; > - uint32_t flags = 0; > unsigned int mgmt_ret; > dbus_int16_t tx_power; > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > client->instance = rp->instance; > > - flags = get_adv_flags(client); > - > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > - error("Advertising data too long or couldn't be generated."); > - goto fail; > - } > - > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > - if ((!scan_rsp && scan_rsp_len) || > - scan_rsp_len > rp->max_scan_rsp_len) { > - error("Scan data couldn't be generated."); > - goto fail; > - } > - > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > - scan_rsp_len; > + param_len = sizeof(struct mgmt_cp_add_advertising) + > + client->adv_data_len + client->scan_rsp_len; > > cp = malloc0(param_len); > if (!cp) { > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > } > > cp->instance = client->instance; > - cp->adv_data_len = adv_data_len; > - cp->scan_rsp_len = scan_rsp_len; > - memcpy(cp->data, adv_data, adv_data_len); > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > - > - free(adv_data); > - free(scan_rsp); > - adv_data = NULL; > - scan_rsp = NULL; > + cp->adv_data_len = client->adv_data_len; > + cp->scan_rsp_len = client->scan_rsp_len; > + memcpy(cp->data, client->adv_data, client->adv_data_len); > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > + client->scan_rsp_len); > > /* Submit request to update instance data */ > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > return; > > fail: > - if (adv_data) > - free(adv_data); > - > - if (scan_rsp) > - free(scan_rsp); > - > if (cp) > free(cp); > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > if (!client->scan) > goto fail; > > + client->adv_data = NULL; > + client->scan_rsp = NULL; > + client->adv_data_len = 0; > + client->scan_rsp_len = 0; > + > client->manager = manager; > client->appearance = UINT16_MAX; > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > -- > 2.30.1.766.gb4fecdf3b7-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline 2021-03-04 22:49 ` Luiz Augusto von Dentz @ 2021-03-04 23:27 ` Daniel Winkler 2021-03-04 23:43 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: Daniel Winkler @ 2021-03-04 23:27 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth, ChromeOS Bluetooth Upstreaming, Alain Michaud, Sonny Sasaka Hi Luiz, Can you please clarify your suggestion? The issue here is that some properties (local name, for instance) aren't incorporated into the bt_ad object until generate_scan_rsp is called. I decided to move the generation of data/scan response to be earlier because otherwise the logic to determine if the scan response was empty would require some repetitive logic that already exists in generate_scan_rsp. Or are you suggesting that we not store adv_data_len and scan_rsp_len in the btd_adv_client, but instead store it in the bt_ad object? This seems possible to me, but it might require a bit more effort to keep the length property in sync. I'll wait for your clarification. Thanks! Daniel On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > This change moves the advertising data generation to the beginning of > > the registration pipeline. This is necessary for the following patch, > > which will need to know whether the scan response data is existent so > > that the parameter request can be populated correctly. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > > > --- > > > > Changes in v2: None > > > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > diff --git a/src/advertising.c b/src/advertising.c > > index 15a343e52..f3dc357a1 100644 > > --- a/src/advertising.c > > +++ b/src/advertising.c > > @@ -80,6 +80,10 @@ struct btd_adv_client { > > uint32_t flags; > > struct bt_ad *data; > > struct bt_ad *scan; > > + uint8_t *adv_data; > > + uint8_t *scan_rsp; > > + size_t adv_data_len; > > + size_t scan_rsp_len; > > I'm debating if we should really just encode it early or we could just > introduce something like bt_ad_length so we don't have to have copies > of the same data in 2 different formats since bt_ad already represents > that. > > > uint8_t instance; > > uint32_t min_interval; > > uint32_t max_interval; > > @@ -141,6 +145,16 @@ static void client_free(void *data) > > bt_ad_unref(client->data); > > bt_ad_unref(client->scan); > > > > + if (client->adv_data) { > > + free(client->adv_data); > > + client->adv_data = NULL; > > + } > > + > > + if (client->scan_rsp) { > > + free(client->scan_rsp); > > + client->scan_rsp = NULL; > > + } > > + > > g_dbus_proxy_unref(client->proxy); > > > > if (client->owner) > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > > flags |= MGMT_ADV_PARAM_TX_POWER; > > } > > > > + client->adv_data = generate_adv_data(client, &flags, > > + &client->adv_data_len); > > + if (!client->adv_data || > > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > > + error("Advertising data too long or couldn't be generated."); > > + return -EINVAL; > > + } > > + > > + client->scan_rsp = generate_scan_rsp(client, &flags, > > + &client->scan_rsp_len); > > + if (!client->scan_rsp && client->scan_rsp_len) { > > + error("Scan data couldn't be generated."); > > + free(client->adv_data); > > + return -EINVAL; > > + } > > + > > cp.flags = htobl(flags); > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > const struct mgmt_rp_add_ext_adv_params *rp = param; > > struct mgmt_cp_add_ext_adv_data *cp = NULL; > > uint8_t param_len; > > - uint8_t *adv_data = NULL; > > - size_t adv_data_len; > > - uint8_t *scan_rsp = NULL; > > - size_t scan_rsp_len = -1; > > - uint32_t flags = 0; > > unsigned int mgmt_ret; > > dbus_int16_t tx_power; > > > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > client->instance = rp->instance; > > > > - flags = get_adv_flags(client); > > - > > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > > - error("Advertising data too long or couldn't be generated."); > > - goto fail; > > - } > > - > > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > > - if ((!scan_rsp && scan_rsp_len) || > > - scan_rsp_len > rp->max_scan_rsp_len) { > > - error("Scan data couldn't be generated."); > > - goto fail; > > - } > > - > > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > > - scan_rsp_len; > > + param_len = sizeof(struct mgmt_cp_add_advertising) + > > + client->adv_data_len + client->scan_rsp_len; > > > > cp = malloc0(param_len); > > if (!cp) { > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > } > > > > cp->instance = client->instance; > > - cp->adv_data_len = adv_data_len; > > - cp->scan_rsp_len = scan_rsp_len; > > - memcpy(cp->data, adv_data, adv_data_len); > > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > > - > > - free(adv_data); > > - free(scan_rsp); > > - adv_data = NULL; > > - scan_rsp = NULL; > > + cp->adv_data_len = client->adv_data_len; > > + cp->scan_rsp_len = client->scan_rsp_len; > > + memcpy(cp->data, client->adv_data, client->adv_data_len); > > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > > + client->scan_rsp_len); > > > > /* Submit request to update instance data */ > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > return; > > > > fail: > > - if (adv_data) > > - free(adv_data); > > - > > - if (scan_rsp) > > - free(scan_rsp); > > - > > if (cp) > > free(cp); > > > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > > if (!client->scan) > > goto fail; > > > > + client->adv_data = NULL; > > + client->scan_rsp = NULL; > > + client->adv_data_len = 0; > > + client->scan_rsp_len = 0; > > + > > client->manager = manager; > > client->appearance = UINT16_MAX; > > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > > -- > > 2.30.1.766.gb4fecdf3b7-goog > > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline 2021-03-04 23:27 ` Daniel Winkler @ 2021-03-04 23:43 ` Luiz Augusto von Dentz 2021-03-16 22:51 ` Daniel Winkler 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2021-03-04 23:43 UTC (permalink / raw) To: Daniel Winkler Cc: linux-bluetooth, ChromeOS Bluetooth Upstreaming, Alain Michaud, Sonny Sasaka Hi Daniel, On Thu, Mar 4, 2021 at 3:27 PM Daniel Winkler <danielwinkler@google.com> wrote: > > Hi Luiz, > > Can you please clarify your suggestion? The issue here is that some > properties (local name, for instance) aren't incorporated into the > bt_ad object until generate_scan_rsp is called. I decided to move the > generation of data/scan response to be earlier because otherwise the > logic to determine if the scan response was empty would require some > repetitive logic that already exists in generate_scan_rsp. > > Or are you suggesting that we not store adv_data_len and scan_rsp_len > in the btd_adv_client, but instead store it in the bt_ad object? This > seems possible to me, but it might require a bit more effort to keep > the length property in sync. I'll wait for your clarification. Yep, the included flags may have to be calculated separately, but I thought the whole point here is to be able to tell if there is any scan_rsp to be added so perhaps just return a bool would be enough to then set the flag so we can continue to generate the data itself at later stage. > Thanks! > Daniel > > On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Daniel, > > > > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > This change moves the advertising data generation to the beginning of > > > the registration pipeline. This is necessary for the following patch, > > > which will need to know whether the scan response data is existent so > > > that the parameter request can be populated correctly. > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > > > > > --- > > > > > > Changes in v2: None > > > > > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > > > diff --git a/src/advertising.c b/src/advertising.c > > > index 15a343e52..f3dc357a1 100644 > > > --- a/src/advertising.c > > > +++ b/src/advertising.c > > > @@ -80,6 +80,10 @@ struct btd_adv_client { > > > uint32_t flags; > > > struct bt_ad *data; > > > struct bt_ad *scan; > > > + uint8_t *adv_data; > > > + uint8_t *scan_rsp; > > > + size_t adv_data_len; > > > + size_t scan_rsp_len; > > > > I'm debating if we should really just encode it early or we could just > > introduce something like bt_ad_length so we don't have to have copies > > of the same data in 2 different formats since bt_ad already represents > > that. > > > > > uint8_t instance; > > > uint32_t min_interval; > > > uint32_t max_interval; > > > @@ -141,6 +145,16 @@ static void client_free(void *data) > > > bt_ad_unref(client->data); > > > bt_ad_unref(client->scan); > > > > > > + if (client->adv_data) { > > > + free(client->adv_data); > > > + client->adv_data = NULL; > > > + } > > > + > > > + if (client->scan_rsp) { > > > + free(client->scan_rsp); > > > + client->scan_rsp = NULL; > > > + } > > > + > > > g_dbus_proxy_unref(client->proxy); > > > > > > if (client->owner) > > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > > > flags |= MGMT_ADV_PARAM_TX_POWER; > > > } > > > > > > + client->adv_data = generate_adv_data(client, &flags, > > > + &client->adv_data_len); > > > + if (!client->adv_data || > > > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > > > + error("Advertising data too long or couldn't be generated."); > > > + return -EINVAL; > > > + } > > > + > > > + client->scan_rsp = generate_scan_rsp(client, &flags, > > > + &client->scan_rsp_len); > > > + if (!client->scan_rsp && client->scan_rsp_len) { > > > + error("Scan data couldn't be generated."); > > > + free(client->adv_data); > > > + return -EINVAL; > > > + } > > > + > > > cp.flags = htobl(flags); > > > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > const struct mgmt_rp_add_ext_adv_params *rp = param; > > > struct mgmt_cp_add_ext_adv_data *cp = NULL; > > > uint8_t param_len; > > > - uint8_t *adv_data = NULL; > > > - size_t adv_data_len; > > > - uint8_t *scan_rsp = NULL; > > > - size_t scan_rsp_len = -1; > > > - uint32_t flags = 0; > > > unsigned int mgmt_ret; > > > dbus_int16_t tx_power; > > > > > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > > > client->instance = rp->instance; > > > > > > - flags = get_adv_flags(client); > > > - > > > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > > > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > > > - error("Advertising data too long or couldn't be generated."); > > > - goto fail; > > > - } > > > - > > > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > > > - if ((!scan_rsp && scan_rsp_len) || > > > - scan_rsp_len > rp->max_scan_rsp_len) { > > > - error("Scan data couldn't be generated."); > > > - goto fail; > > > - } > > > - > > > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > > > - scan_rsp_len; > > > + param_len = sizeof(struct mgmt_cp_add_advertising) + > > > + client->adv_data_len + client->scan_rsp_len; > > > > > > cp = malloc0(param_len); > > > if (!cp) { > > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > } > > > > > > cp->instance = client->instance; > > > - cp->adv_data_len = adv_data_len; > > > - cp->scan_rsp_len = scan_rsp_len; > > > - memcpy(cp->data, adv_data, adv_data_len); > > > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > > > - > > > - free(adv_data); > > > - free(scan_rsp); > > > - adv_data = NULL; > > > - scan_rsp = NULL; > > > + cp->adv_data_len = client->adv_data_len; > > > + cp->scan_rsp_len = client->scan_rsp_len; > > > + memcpy(cp->data, client->adv_data, client->adv_data_len); > > > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > > > + client->scan_rsp_len); > > > > > > /* Submit request to update instance data */ > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > return; > > > > > > fail: > > > - if (adv_data) > > > - free(adv_data); > > > - > > > - if (scan_rsp) > > > - free(scan_rsp); > > > - > > > if (cp) > > > free(cp); > > > > > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > > > if (!client->scan) > > > goto fail; > > > > > > + client->adv_data = NULL; > > > + client->scan_rsp = NULL; > > > + client->adv_data_len = 0; > > > + client->scan_rsp_len = 0; > > > + > > > client->manager = manager; > > > client->appearance = UINT16_MAX; > > > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > > > -- > > > 2.30.1.766.gb4fecdf3b7-goog > > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline 2021-03-04 23:43 ` Luiz Augusto von Dentz @ 2021-03-16 22:51 ` Daniel Winkler 0 siblings, 0 replies; 8+ messages in thread From: Daniel Winkler @ 2021-03-16 22:51 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth, ChromeOS Bluetooth Upstreaming, Alain Michaud, Sonny Sasaka Hi Luiz, Sorry for the delay. I think I understand your reservations now, and have just uploaded a V3 that uses helpers to determine if the instance will be scannable, rather than storing a duplicated copy of adv data and scan response. Please take a look at your convenience. Best, Daniel On Thu, Mar 4, 2021 at 3:43 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Thu, Mar 4, 2021 at 3:27 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > Hi Luiz, > > > > Can you please clarify your suggestion? The issue here is that some > > properties (local name, for instance) aren't incorporated into the > > bt_ad object until generate_scan_rsp is called. I decided to move the > > generation of data/scan response to be earlier because otherwise the > > logic to determine if the scan response was empty would require some > > repetitive logic that already exists in generate_scan_rsp. > > > > Or are you suggesting that we not store adv_data_len and scan_rsp_len > > in the btd_adv_client, but instead store it in the bt_ad object? This > > seems possible to me, but it might require a bit more effort to keep > > the length property in sync. I'll wait for your clarification. > > Yep, the included flags may have to be calculated separately, but I > thought the whole point here is to be able to tell if there is any > scan_rsp to be added so perhaps just return a bool would be enough to > then set the flag so we can continue to generate the data itself at > later stage. > > > Thanks! > > Daniel > > > > On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Daniel, > > > > > > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > > > This change moves the advertising data generation to the beginning of > > > > the registration pipeline. This is necessary for the following patch, > > > > which will need to know whether the scan response data is existent so > > > > that the parameter request can be populated correctly. > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > > > > > > > --- > > > > > > > > Changes in v2: None > > > > > > > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > > > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > > > > > diff --git a/src/advertising.c b/src/advertising.c > > > > index 15a343e52..f3dc357a1 100644 > > > > --- a/src/advertising.c > > > > +++ b/src/advertising.c > > > > @@ -80,6 +80,10 @@ struct btd_adv_client { > > > > uint32_t flags; > > > > struct bt_ad *data; > > > > struct bt_ad *scan; > > > > + uint8_t *adv_data; > > > > + uint8_t *scan_rsp; > > > > + size_t adv_data_len; > > > > + size_t scan_rsp_len; > > > > > > I'm debating if we should really just encode it early or we could just > > > introduce something like bt_ad_length so we don't have to have copies > > > of the same data in 2 different formats since bt_ad already represents > > > that. > > > > > > > uint8_t instance; > > > > uint32_t min_interval; > > > > uint32_t max_interval; > > > > @@ -141,6 +145,16 @@ static void client_free(void *data) > > > > bt_ad_unref(client->data); > > > > bt_ad_unref(client->scan); > > > > > > > > + if (client->adv_data) { > > > > + free(client->adv_data); > > > > + client->adv_data = NULL; > > > > + } > > > > + > > > > + if (client->scan_rsp) { > > > > + free(client->scan_rsp); > > > > + client->scan_rsp = NULL; > > > > + } > > > > + > > > > g_dbus_proxy_unref(client->proxy); > > > > > > > > if (client->owner) > > > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > > > > flags |= MGMT_ADV_PARAM_TX_POWER; > > > > } > > > > > > > > + client->adv_data = generate_adv_data(client, &flags, > > > > + &client->adv_data_len); > > > > + if (!client->adv_data || > > > > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > > > > + error("Advertising data too long or couldn't be generated."); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + client->scan_rsp = generate_scan_rsp(client, &flags, > > > > + &client->scan_rsp_len); > > > > + if (!client->scan_rsp && client->scan_rsp_len) { > > > > + error("Scan data couldn't be generated."); > > > > + free(client->adv_data); > > > > + return -EINVAL; > > > > + } > > > > + > > > > cp.flags = htobl(flags); > > > > > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > > > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > const struct mgmt_rp_add_ext_adv_params *rp = param; > > > > struct mgmt_cp_add_ext_adv_data *cp = NULL; > > > > uint8_t param_len; > > > > - uint8_t *adv_data = NULL; > > > > - size_t adv_data_len; > > > > - uint8_t *scan_rsp = NULL; > > > > - size_t scan_rsp_len = -1; > > > > - uint32_t flags = 0; > > > > unsigned int mgmt_ret; > > > > dbus_int16_t tx_power; > > > > > > > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > > > > > client->instance = rp->instance; > > > > > > > > - flags = get_adv_flags(client); > > > > - > > > > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > > > > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > > > > - error("Advertising data too long or couldn't be generated."); > > > > - goto fail; > > > > - } > > > > - > > > > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > > > > - if ((!scan_rsp && scan_rsp_len) || > > > > - scan_rsp_len > rp->max_scan_rsp_len) { > > > > - error("Scan data couldn't be generated."); > > > > - goto fail; > > > > - } > > > > - > > > > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > > > > - scan_rsp_len; > > > > + param_len = sizeof(struct mgmt_cp_add_advertising) + > > > > + client->adv_data_len + client->scan_rsp_len; > > > > > > > > cp = malloc0(param_len); > > > > if (!cp) { > > > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > } > > > > > > > > cp->instance = client->instance; > > > > - cp->adv_data_len = adv_data_len; > > > > - cp->scan_rsp_len = scan_rsp_len; > > > > - memcpy(cp->data, adv_data, adv_data_len); > > > > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > > > > - > > > > - free(adv_data); > > > > - free(scan_rsp); > > > > - adv_data = NULL; > > > > - scan_rsp = NULL; > > > > + cp->adv_data_len = client->adv_data_len; > > > > + cp->scan_rsp_len = client->scan_rsp_len; > > > > + memcpy(cp->data, client->adv_data, client->adv_data_len); > > > > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > > > > + client->scan_rsp_len); > > > > > > > > /* Submit request to update instance data */ > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > > > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > return; > > > > > > > > fail: > > > > - if (adv_data) > > > > - free(adv_data); > > > > - > > > > - if (scan_rsp) > > > > - free(scan_rsp); > > > > - > > > > if (cp) > > > > free(cp); > > > > > > > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > > > > if (!client->scan) > > > > goto fail; > > > > > > > > + client->adv_data = NULL; > > > > + client->scan_rsp = NULL; > > > > + client->adv_data_len = 0; > > > > + client->scan_rsp_len = 0; > > > > + > > > > client->manager = manager; > > > > client->appearance = UINT16_MAX; > > > > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > > > > -- > > > > 2.30.1.766.gb4fecdf3b7-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bluez PATCH v2 2/3] advertising: Create and use scannable adv param flag 2021-03-04 20:24 [Bluez PATCH v2 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline Daniel Winkler @ 2021-03-04 20:24 ` Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 3/3] doc/mgmt-api: Update documentation for scan_rsp " Daniel Winkler 2 siblings, 0 replies; 8+ messages in thread From: Daniel Winkler @ 2021-03-04 20:24 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Daniel Winkler, Alain Michaud, Sonny Sasaka In order for the advertising parameters hci request to indicate that an advertising set uses a scannable PDU, we pass a scannable flag along with the initial parameters MGMT request. Without this patch, a broadcast advertisement with a scan response will either be rejected by the controller, or will ignore the requested scan response. The patch is tested by performing the above and confirming that the scan response is retrievable from a peer as expected. Reviewed-by: Alain Michaud <alainm@chromium.org> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> --- Changes in v2: - Check kernel supports flag before setting it lib/mgmt.h | 1 + src/advertising.c | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/mgmt.h b/lib/mgmt.h index 76a03c9c2..7b1b9ab54 100644 --- a/lib/mgmt.h +++ b/lib/mgmt.h @@ -507,6 +507,7 @@ struct mgmt_rp_add_advertising { #define MGMT_ADV_PARAM_TIMEOUT (1 << 13) #define MGMT_ADV_PARAM_INTERVALS (1 << 14) #define MGMT_ADV_PARAM_TX_POWER (1 << 15) +#define MGMT_ADV_PARAM_SCAN_RSP (1 << 16) #define MGMT_OP_REMOVE_ADVERTISING 0x003F struct mgmt_cp_remove_advertising { diff --git a/src/advertising.c b/src/advertising.c index f3dc357a1..cdf06154d 100644 --- a/src/advertising.c +++ b/src/advertising.c @@ -945,7 +945,13 @@ static int refresh_extended_adv(struct btd_adv_client *client, return -EINVAL; } - cp.flags = htobl(flags); + /* Indicate that this instance will be configured as scannable */ + if (client->scan_rsp_len && + client->manager->supported_flags & MGMT_ADV_PARAM_SCAN_RSP) { + flags |= MGMT_ADV_PARAM_SCAN_RSP; + } + + cp.flags = cpu_to_le32(flags); mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, client->manager->mgmt_index, sizeof(cp), &cp, -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Bluez PATCH v2 3/3] doc/mgmt-api: Update documentation for scan_rsp param flag 2021-03-04 20:24 [Bluez PATCH v2 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 2/3] advertising: Create and use scannable adv param flag Daniel Winkler @ 2021-03-04 20:24 ` Daniel Winkler 2 siblings, 0 replies; 8+ messages in thread From: Daniel Winkler @ 2021-03-04 20:24 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Daniel Winkler, Alain Michaud, Sonny Sasaka This patch adds the new scannable flag to the Add Extended Advertising Parameters MGMT API documentation. Reviewed-by: Alain Michaud <alainm@chromium.org> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> --- Changes in v2: None doc/mgmt-api.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt index 1736ef009..cab1fffc5 100644 --- a/doc/mgmt-api.txt +++ b/doc/mgmt-api.txt @@ -3632,6 +3632,7 @@ Add Extended Advertising Parameters Command 13 The Timeout parameter should be used 14 The Interval parameters should be used 15 The Tx Power parameter should be used + 16 The advertisement will contain a scan response When the connectable flag is set, then the controller will use undirected connectable advertising. The value of the connectable @@ -3708,6 +3709,10 @@ Add Extended Advertising Parameters Command chosen by the controller. If the requested Tx Power is outside the valid range, the registration will fail. + When flag bit 16 is enabled, it indicates that the subsequent request + to set advertising data will contain a scan response, and that the + parameters should set a PDU type that is scannable. + Re-adding an already existing instance (i.e. issuing the Add Extended Advertising Parameters command with an Instance identifier of an existing instance) will update that instance's configuration. In this -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-16 22:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-04 20:24 [Bluez PATCH v2 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline Daniel Winkler 2021-03-04 22:49 ` Luiz Augusto von Dentz 2021-03-04 23:27 ` Daniel Winkler 2021-03-04 23:43 ` Luiz Augusto von Dentz 2021-03-16 22:51 ` Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 2/3] advertising: Create and use scannable adv param flag Daniel Winkler 2021-03-04 20:24 ` [Bluez PATCH v2 3/3] doc/mgmt-api: Update documentation for scan_rsp " Daniel Winkler
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.