if (adv && !adv->pending) {
to
if (adv && adv->enabled) {
it seems to do the job correctly. What do you think?
regards,
Christian
On Friday, 27 June 2025, 09:05:08 CEST, Christian Eggers wrote:
> Currently, for controllers with extended advertising, the advertising
> data is set in the asynchronous response handler for extended
> adverstising params. As most advertising settings are performed in a
> synchronous context, the (asynchronous) setting of the advertising data
> is done too late (after enabling the advertising).
>
> Move setting of adverstising data from asynchronous response handler
> into synchronous context to fix ordering of HCI commands.
>
> Signed-off-by: Christian Eggers <ceggers@xxxxxxx>
> Fixes: a0fb3726ba55 ("Bluetooth: Use Set ext adv/scan rsp data if controller supports")
> Cc: stable@xxxxxxxxxxxxxxx
> v2: https://lore.kernel.org/linux-bluetooth/20250626115209.17839-1-ceggers@xxxxxxx/
> ---
> v3: refactor: store adv_addr_type/tx_power within hci_set_ext_adv_params_sync()
>
> v2: convert setting of adv data into synchronous context (rather than moving
> more methods into asynchronous response handlers).
> - hci_set_ext_adv_params_sync: new method
> - hci_set_ext_adv_data_sync: move within source file (no changes)
> - hci_set_adv_data_sync: dito
> - hci_update_adv_data_sync: dito
> - hci_cc_set_ext_adv_param: remove (performed synchronously now)
>
> net/bluetooth/hci_event.c | 36 -------
> net/bluetooth/hci_sync.c | 207 ++++++++++++++++++++++++--------------
> 2 files changed, 130 insertions(+), 113 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 66052d6aaa1d..4d5ace9d245d 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2150,40 +2150,6 @@ static u8 hci_cc_set_adv_param(struct hci_dev *hdev, void *data,
> return rp->status;
> }
>
> -static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data,
> - struct sk_buff *skb)
> -{
> - struct hci_rp_le_set_ext_adv_params *rp = data;
> - struct hci_cp_le_set_ext_adv_params *cp;
> - struct adv_info *adv_instance;
> -
> - bt_dev_dbg(hdev, "status 0x%2.2x", rp->status);
> -
> - if (rp->status)
> - return rp->status;
> -
> - cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS);
> - if (!cp)
> - return rp->status;
> -
> - hci_dev_lock(hdev);
> - hdev->adv_addr_type = cp->own_addr_type;
> - if (!cp->handle) {
> - /* Store in hdev for instance 0 */
> - hdev->adv_tx_power = rp->tx_power;
> - } else {
> - adv_instance = hci_find_adv_instance(hdev, cp->handle);
> - if (adv_instance)
> - adv_instance->tx_power = rp->tx_power;
> - }
> - /* Update adv data as tx power is known now */
> - hci_update_adv_data(hdev, cp->handle);
> -
> - hci_dev_unlock(hdev);
> -
> - return rp->status;
> -}
> -
> static u8 hci_cc_read_rssi(struct hci_dev *hdev, void *data,
> struct sk_buff *skb)
> {
> @@ -4164,8 +4130,6 @@ static const struct hci_cc {
> HCI_CC(HCI_OP_LE_READ_NUM_SUPPORTED_ADV_SETS,
> hci_cc_le_read_num_adv_sets,
> sizeof(struct hci_rp_le_read_num_supported_adv_sets)),
> - HCI_CC(HCI_OP_LE_SET_EXT_ADV_PARAMS, hci_cc_set_ext_adv_param,
> - sizeof(struct hci_rp_le_set_ext_adv_params)),
> HCI_CC_STATUS(HCI_OP_LE_SET_EXT_ADV_ENABLE,
> hci_cc_le_set_ext_adv_enable),
> HCI_CC_STATUS(HCI_OP_LE_SET_ADV_SET_RAND_ADDR,
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 1f8806dfa556..563614b53485 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1205,9 +1205,126 @@ static int hci_set_adv_set_random_addr_sync(struct hci_dev *hdev, u8 instance,
> sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> }
>
> +static int
> +hci_set_ext_adv_params_sync(struct hci_dev *hdev, struct adv_info *adv,
> + const struct hci_cp_le_set_ext_adv_params *cp,
> + struct hci_rp_le_set_ext_adv_params *rp)
> +{
> + struct sk_buff *skb;
> +
> + skb = __hci_cmd_sync(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS, sizeof(*cp),
> + cp, HCI_CMD_TIMEOUT);
> +
> + /* If command return a status event, skb will be set to -ENODATA */
> + if (skb == ERR_PTR(-ENODATA))
> + return 0;
> +
> + if (IS_ERR(skb)) {
> + bt_dev_err(hdev, "Opcode 0x%4.4x failed: %ld",
> + HCI_OP_LE_SET_EXT_ADV_PARAMS, PTR_ERR(skb));
> + return PTR_ERR(skb);
> + }
> +
> + if (skb->len != sizeof(*rp)) {
> + bt_dev_err(hdev, "Invalid response length for "
> + "HCI_OP_LE_SET_EXT_ADV_PARAMS: %u", skb->len);
> + kfree_skb(skb);
> + return -EIO;
> + }
> +
> + memcpy(rp, skb->data, sizeof(*rp));
> + kfree_skb(skb);
> +
> + if (!rp->status) {
> + hdev->adv_addr_type = cp->own_addr_type;
> + if (!cp->handle) {
> + /* Store in hdev for instance 0 */
> + hdev->adv_tx_power = rp->tx_power;
> + } else if (adv) {
> + adv->tx_power = rp->tx_power;
> + }
> + }
> +
> + return rp->status;
> +}
> +
> +static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
> +{
> + DEFINE_FLEX(struct hci_cp_le_set_ext_adv_data, pdu, data, length,
> + HCI_MAX_EXT_AD_LENGTH);
> + u8 len;
> + struct adv_info *adv = NULL;
> + int err;
> +
> + if (instance) {
> + adv = hci_find_adv_instance(hdev, instance);
> + if (!adv || !adv->adv_data_changed)
> + return 0;
> + }
> +
> + len = eir_create_adv_data(hdev, instance, pdu->data,
> + HCI_MAX_EXT_AD_LENGTH);
> +
> + pdu->length = len;
> + pdu->handle = adv ? adv->handle : instance;
> + pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
> + pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG;
> +
> + err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
> + struct_size(pdu, data, len), pdu,
> + HCI_CMD_TIMEOUT);
> + if (err)
> + return err;
> +
> + /* Update data if the command succeed */
> + if (adv) {
> + adv->adv_data_changed = false;
> + } else {
> + memcpy(hdev->adv_data, pdu->data, len);
> + hdev->adv_data_len = len;
> + }
> +
> + return 0;
> +}
> +
> +static int hci_set_adv_data_sync(struct hci_dev *hdev, u8 instance)
> +{
> + struct hci_cp_le_set_adv_data cp;
> + u8 len;
> +
> + memset(&cp, 0, sizeof(cp));
> +
> + len = eir_create_adv_data(hdev, instance, cp.data, sizeof(cp.data));
> +
> + /* There's nothing to do if the data hasn't changed */
> + if (hdev->adv_data_len == len &&
> + memcmp(cp.data, hdev->adv_data, len) == 0)
> + return 0;
> +
> + memcpy(hdev->adv_data, cp.data, sizeof(cp.data));
> + hdev->adv_data_len = len;
> +
> + cp.length = len;
> +
> + return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_ADV_DATA,
> + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> +}
> +
> +int hci_update_adv_data_sync(struct hci_dev *hdev, u8 instance)
> +{
> + if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED))
> + return 0;
> +
> + if (ext_adv_capable(hdev))
> + return hci_set_ext_adv_data_sync(hdev, instance);
> +
> + return hci_set_adv_data_sync(hdev, instance);
> +}
> +
> int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
> {
> struct hci_cp_le_set_ext_adv_params cp;
> + struct hci_rp_le_set_ext_adv_params rp;
> bool connectable;
> u32 flags;
> bdaddr_t random_addr;
> @@ -1316,8 +1433,12 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
> cp.secondary_phy = HCI_ADV_PHY_1M;
> }
>
> - err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS,
> - sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> + err = hci_set_ext_adv_params_sync(hdev, adv, &cp, &rp);
> + if (err)
> + return err;
> +
> + /* Update adv data as tx power is known now */
> + err = hci_set_ext_adv_data_sync(hdev, cp.handle);
> if (err)
> return err;
>
> @@ -1822,79 +1943,6 @@ int hci_le_terminate_big_sync(struct hci_dev *hdev, u8 handle, u8 reason)
> sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> }
>
> -static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
> -{
> - DEFINE_FLEX(struct hci_cp_le_set_ext_adv_data, pdu, data, length,
> - HCI_MAX_EXT_AD_LENGTH);
> - u8 len;
> - struct adv_info *adv = NULL;
> - int err;
> -
> - if (instance) {
> - adv = hci_find_adv_instance(hdev, instance);
> - if (!adv || !adv->adv_data_changed)
> - return 0;
> - }
> -
> - len = eir_create_adv_data(hdev, instance, pdu->data,
> - HCI_MAX_EXT_AD_LENGTH);
> -
> - pdu->length = len;
> - pdu->handle = adv ? adv->handle : instance;
> - pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
> - pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG;
> -
> - err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
> - struct_size(pdu, data, len), pdu,
> - HCI_CMD_TIMEOUT);
> - if (err)
> - return err;
> -
> - /* Update data if the command succeed */
> - if (adv) {
> - adv->adv_data_changed = false;
> - } else {
> - memcpy(hdev->adv_data, pdu->data, len);
> - hdev->adv_data_len = len;
> - }
> -
> - return 0;
> -}
> -
> -static int hci_set_adv_data_sync(struct hci_dev *hdev, u8 instance)
> -{
> - struct hci_cp_le_set_adv_data cp;
> - u8 len;
> -
> - memset(&cp, 0, sizeof(cp));
> -
> - len = eir_create_adv_data(hdev, instance, cp.data, sizeof(cp.data));
> -
> - /* There's nothing to do if the data hasn't changed */
> - if (hdev->adv_data_len == len &&
> - memcmp(cp.data, hdev->adv_data, len) == 0)
> - return 0;
> -
> - memcpy(hdev->adv_data, cp.data, sizeof(cp.data));
> - hdev->adv_data_len = len;
> -
> - cp.length = len;
> -
> - return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_ADV_DATA,
> - sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> -}
> -
> -int hci_update_adv_data_sync(struct hci_dev *hdev, u8 instance)
> -{
> - if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED))
> - return 0;
> -
> - if (ext_adv_capable(hdev))
> - return hci_set_ext_adv_data_sync(hdev, instance);
> -
> - return hci_set_adv_data_sync(hdev, instance);
> -}
> -
> int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance,
> bool force)
> {
> @@ -6269,6 +6317,7 @@ static int hci_le_ext_directed_advertising_sync(struct hci_dev *hdev,
> struct hci_conn *conn)
> {
> struct hci_cp_le_set_ext_adv_params cp;
> + struct hci_rp_le_set_ext_adv_params rp;
> int err;
> bdaddr_t random_addr;
> u8 own_addr_type;
> @@ -6310,8 +6359,12 @@ static int hci_le_ext_directed_advertising_sync(struct hci_dev *hdev,
> if (err)
> return err;
>
> - err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS,
> - sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> + err = hci_set_ext_adv_params_sync(hdev, NULL, &cp, &rp);
> + if (err)
> + return err;
> +
> + /* Update adv data as tx power is known now */
> + err = hci_set_ext_adv_data_sync(hdev, cp.handle);
> if (err)
> return err;
>
>
