Hi Christian,
On Thu, Jun 26, 2025 at 7:52 AM Christian Eggers ceggers@arri.de 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@arri.de Fixes: a0fb3726ba55 ("Bluetooth: Use Set ext adv/scan rsp data if controller supports") Cc: stable@vger.kernel.org v1: https://lore.kernel.org/linux-bluetooth/20250625130510.18382-1-ceggers@arri....
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)
On Wednesday, 25 June 2025, 15:26:58 CEST, Luiz Augusto von Dentz wrote:
That said for the likes of MGMT_OP_ADD_EXT_ADV_DATA you will still need to detect if the instance has already been enabled then do disable/re-enable logic if the quirk is set.
The critical opcode (HCI_OP_LE_SET_EXT_ADV_DATA) is only used in hci_set_ext_adv_data_sync(). Two of the callers already ensure that the advertising instance is disabled, so only hci_update_adv_data_sync() may need a quirk. I suggest doing this in a separate patch.
regards, Christian
net/bluetooth/hci_event.c | 36 ------- net/bluetooth/hci_sync.c | 209 ++++++++++++++++++++++++-------------- 2 files changed, 132 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..2a09b2cb983e 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -1205,9 +1205,116 @@ 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,
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);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 +1423,20 @@ 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, &cp, &rp);if (err)return err;hdev->adv_addr_type = 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;}
We can probably move the above code into hci_set_ext_adv_params_sync so we guarantee the tx_power is updated whenever it is used, if there are differences between the likes of directed advertisements, etc, that can probably be handled internally as well, although I think it doesn't seem to need a special handling since we restrict directected advertisements to handle 0x00 only.
/* 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 +1941,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 +6315,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 +6357,16 @@ 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, &cp, &rp);if (err)return err;hdev->adv_addr_type = own_addr_type;/* Store in hdev for instance 0 */hdev->adv_tx_power = rp.tx_power;/* Update adv data as tx power is known now */err = hci_set_ext_adv_data_sync(hdev, cp.handle); if (err) return err;-- 2.44.1