For extended advertising capable controllers, hci_start_ext_adv_sync() at the moment synchronously calls SET_EXT_ADV_PARAMS [1], SET_ADV_SET_RAND_ADDR [2], SET_EXT_SCAN_RSP_DATA [3](optional) and SET_EXT_ADV_ENABLE [4]. After all synchronous commands are finished, SET_EXT_ADV_DATA is called from the async response handler of SET_EXT_ADV_PARAMS [5] (via hci_update_adv_data).
So the current implementation sets the advertising data AFTER enabling the advertising instance. The BT Core specification explicitly allows for this [6]:
If advertising is currently enabled for the specified advertising set, the Controller shall use the new data in subsequent extended advertising events for this advertising set. If an extended advertising event is in progress when this command is issued, the Controller may use the old or new data for that event.
In case of the Realtek RTL8761BU chip (almost all contemporary BT USB dongles are built on it), updating the advertising data after enabling the instance produces (at least one) corrupted advertising message. Under normal conditions, a single corrupted advertising message would probably not attract much attention, but during MESH provisioning (via MGMT I/O / mesh_send(_sync)), up to 3 different messages (BEACON, ACK, CAPS) are sent within a loop which causes corruption of ALL provisioning messages.
I have no idea whether this could be fixed in the firmware of the USB dongles (I didn't even find the chip on the Realtek homepage), but generally I would suggest changing the order of the HCI commands as this matches the command order for "non-extended adv capable" controllers and simply is more natural.
This patch only considers advertising instances with handle > 0, I don't know whether this should be extended to further cases.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [6] https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-6...
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 --- include/net/bluetooth/hci_core.h | 1 + include/net/bluetooth/hci_sync.h | 1 + net/bluetooth/hci_event.c | 33 +++++++++++++++++++++++++++++ net/bluetooth/hci_sync.c | 36 ++++++++++++++++++++++++++------ 4 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 9fc8f544e20e..8d37f127ddba 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -237,6 +237,7 @@ struct oob_data {
struct adv_info { struct list_head list; + bool enable_after_set_ext_data; bool enabled; bool pending; bool periodic; diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index 5224f57f6af2..00eceffeec87 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -112,6 +112,7 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance, int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance); int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance); int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance); +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance); int hci_enable_advertising_sync(struct hci_dev *hdev); int hci_enable_advertising(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 66052d6aaa1d..eb018d8a3c4b 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -2184,6 +2184,37 @@ static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data, return rp->status; }
+static u8 hci_cc_le_set_ext_adv_data(struct hci_dev *hdev, void *data, + struct sk_buff *skb) +{ + struct hci_cp_le_set_ext_adv_data *cp; + struct hci_ev_status *rp = data; + 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_DATA); + if (!cp) + return rp->status; + + hci_dev_lock(hdev); + + if (cp->handle) { + adv_instance = hci_find_adv_instance(hdev, cp->handle); + if (adv_instance) { + if (adv_instance->enable_after_set_ext_data) + hci_enable_ext_advertising(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) { @@ -4166,6 +4197,8 @@ static const struct hci_cc { 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_DATA, + hci_cc_le_set_ext_adv_data), 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..da0e39cce721 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -1262,6 +1262,7 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance) hci_cpu_to_le24(adv->max_interval, cp.max_interval); cp.tx_power = adv->tx_power; cp.sid = adv->sid; + adv->enable_after_set_ext_data = true; } else { hci_cpu_to_le24(hdev->le_adv_min_interval, cp.min_interval); hci_cpu_to_le24(hdev->le_adv_max_interval, cp.max_interval); @@ -1456,6 +1457,23 @@ int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance) data, HCI_CMD_TIMEOUT); }
+static int enable_ext_advertising_sync(struct hci_dev *hdev, void *data) +{ + u8 instance = PTR_UINT(data); + + return hci_enable_ext_advertising_sync(hdev, instance); +} + +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance) +{ + if (!hci_dev_test_flag(hdev, HCI_ADVERTISING) && + list_empty(&hdev->adv_instances)) + return 0; + + return hci_cmd_sync_queue(hdev, enable_ext_advertising_sync, + UINT_PTR(instance), NULL); +} + int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) { int err; @@ -1464,11 +1482,11 @@ int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) if (err) return err;
- err = hci_set_ext_scan_rsp_data_sync(hdev, instance); - if (err) - return err; - - return hci_enable_ext_advertising_sync(hdev, instance); + /* SET_EXT_ADV_DATA and SET_EXT_ADV_ENABLE are called in the + * asynchronous response chain of set_ext_adv_params in order to + * set the advertising data first prior enabling it. + */ + return hci_set_ext_scan_rsp_data_sync(hdev, instance); }
int hci_disable_per_advertising_sync(struct hci_dev *hdev, u8 instance) @@ -1832,8 +1850,14 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
if (instance) { adv = hci_find_adv_instance(hdev, instance); - if (!adv || !adv->adv_data_changed) + if (!adv) return 0; + if (!adv->adv_data_changed) { + if (adv->enable_after_set_ext_data) + hci_enable_ext_advertising_sync(hdev, + adv->handle); + return 0; + } }
len = eir_create_adv_data(hdev, instance, pdu->data,
Hi Christian,
On Wed, Jun 25, 2025 at 9:05 AM Christian Eggers ceggers@arri.de wrote:
For extended advertising capable controllers, hci_start_ext_adv_sync() at the moment synchronously calls SET_EXT_ADV_PARAMS [1], SET_ADV_SET_RAND_ADDR [2], SET_EXT_SCAN_RSP_DATA [3](optional) and SET_EXT_ADV_ENABLE [4]. After all synchronous commands are finished, SET_EXT_ADV_DATA is called from the async response handler of SET_EXT_ADV_PARAMS [5] (via hci_update_adv_data).
So the current implementation sets the advertising data AFTER enabling the advertising instance. The BT Core specification explicitly allows for this [6]:
If advertising is currently enabled for the specified advertising set, the Controller shall use the new data in subsequent extended advertising events for this advertising set. If an extended advertising event is in progress when this command is issued, the Controller may use the old or new data for that event.
Ok, lets stop right here, if the controller deviates from the spec it needs a quirk and not make the whole stack work around a bug in the firmware.
In case of the Realtek RTL8761BU chip (almost all contemporary BT USB dongles are built on it), updating the advertising data after enabling the instance produces (at least one) corrupted advertising message. Under normal conditions, a single corrupted advertising message would probably not attract much attention, but during MESH provisioning (via MGMT I/O / mesh_send(_sync)), up to 3 different messages (BEACON, ACK, CAPS) are sent within a loop which causes corruption of ALL provisioning messages.
I have no idea whether this could be fixed in the firmware of the USB dongles (I didn't even find the chip on the Realtek homepage), but generally I would suggest changing the order of the HCI commands as this matches the command order for "non-extended adv capable" controllers and simply is more natural.
This patch only considers advertising instances with handle > 0, I don't know whether this should be extended to further cases.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [6] https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-6...
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
include/net/bluetooth/hci_core.h | 1 + include/net/bluetooth/hci_sync.h | 1 + net/bluetooth/hci_event.c | 33 +++++++++++++++++++++++++++++ net/bluetooth/hci_sync.c | 36 ++++++++++++++++++++++++++------ 4 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 9fc8f544e20e..8d37f127ddba 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -237,6 +237,7 @@ struct oob_data {
struct adv_info { struct list_head list;
bool enable_after_set_ext_data; bool enabled; bool pending; bool periodic;diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index 5224f57f6af2..00eceffeec87 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -112,6 +112,7 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance, int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance); int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance); int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance); +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance); int hci_enable_advertising_sync(struct hci_dev *hdev); int hci_enable_advertising(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 66052d6aaa1d..eb018d8a3c4b 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -2184,6 +2184,37 @@ static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data, return rp->status; }
+static u8 hci_cc_le_set_ext_adv_data(struct hci_dev *hdev, void *data,
struct sk_buff *skb)+{
struct hci_cp_le_set_ext_adv_data *cp;struct hci_ev_status *rp = data;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_DATA);if (!cp)return rp->status;hci_dev_lock(hdev);if (cp->handle) {adv_instance = hci_find_adv_instance(hdev, cp->handle);if (adv_instance) {if (adv_instance->enable_after_set_ext_data)hci_enable_ext_advertising(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) { @@ -4166,6 +4197,8 @@ static const struct hci_cc { 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_DATA,hci_cc_le_set_ext_adv_data), 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..da0e39cce721 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -1262,6 +1262,7 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance) hci_cpu_to_le24(adv->max_interval, cp.max_interval); cp.tx_power = adv->tx_power; cp.sid = adv->sid;
adv->enable_after_set_ext_data = true; } else { hci_cpu_to_le24(hdev->le_adv_min_interval, cp.min_interval); hci_cpu_to_le24(hdev->le_adv_max_interval, cp.max_interval);@@ -1456,6 +1457,23 @@ int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance) data, HCI_CMD_TIMEOUT); }
+static int enable_ext_advertising_sync(struct hci_dev *hdev, void *data) +{
u8 instance = PTR_UINT(data);return hci_enable_ext_advertising_sync(hdev, instance);+}
+int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance) +{
if (!hci_dev_test_flag(hdev, HCI_ADVERTISING) &&list_empty(&hdev->adv_instances))return 0;return hci_cmd_sync_queue(hdev, enable_ext_advertising_sync,UINT_PTR(instance), NULL);+}
int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) { int err; @@ -1464,11 +1482,11 @@ int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) if (err) return err;
err = hci_set_ext_scan_rsp_data_sync(hdev, instance);if (err)return err;return hci_enable_ext_advertising_sync(hdev, instance);
/* SET_EXT_ADV_DATA and SET_EXT_ADV_ENABLE are called in the* asynchronous response chain of set_ext_adv_params in order to* set the advertising data first prior enabling it.*/
Doing things asynchronously is known to create problems, which is why we introduced the cmd_sync infra to handle a chain of commands like this, so Id suggest sticking to the synchronous way, if the order needs to be changed then use a quirk to detect it and then make sure the instance is disabled on hci_set_ext_adv_data_sync and then re-enable after updating it.
return hci_set_ext_scan_rsp_data_sync(hdev, instance);}
int hci_disable_per_advertising_sync(struct hci_dev *hdev, u8 instance) @@ -1832,8 +1850,14 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
if (instance) { adv = hci_find_adv_instance(hdev, instance);
if (!adv || !adv->adv_data_changed)
if (!adv) return 0;if (!adv->adv_data_changed) {if (adv->enable_after_set_ext_data)hci_enable_ext_advertising_sync(hdev,adv->handle);return 0;} } len = eir_create_adv_data(hdev, instance, pdu->data,-- 2.43.0
Hi Luiz,
On Wednesday, 25 June 2025, 15:26:58 CEST, Luiz Augusto von Dentz wrote:
Hi Christian,
On Wed, Jun 25, 2025 at 9:05 AM Christian Eggers ceggers@arri.de wrote:
For extended advertising capable controllers, hci_start_ext_adv_sync() at the moment synchronously calls SET_EXT_ADV_PARAMS [1], SET_ADV_SET_RAND_ADDR [2], SET_EXT_SCAN_RSP_DATA [3](optional) and SET_EXT_ADV_ENABLE [4]. After all synchronous commands are finished, SET_EXT_ADV_DATA is called from the async response handler of SET_EXT_ADV_PARAMS [5] (via hci_update_adv_data).
So the current implementation sets the advertising data AFTER enabling the advertising instance. The BT Core specification explicitly allows for this [6]:
If advertising is currently enabled for the specified advertising set, the Controller shall use the new data in subsequent extended advertising events for this advertising set. If an extended advertising event is in progress when this command is issued, the Controller may use the old or new data for that event.
Ok, lets stop right here, if the controller deviates from the spec it needs a quirk and not make the whole stack work around a bug in the firmware.
I generally agree! In this particular case, I think that the current order of advertising commands may be the result of "random" and was probably not intended this way. While the command order of the "legacy" advertising commands looks perfectly logical for me, the order of the "extended" commands seems to be broken by setting the advertising data in the asynchronous response handler of set_ext_adv_params.
In case of the Realtek RTL8761BU chip (almost all contemporary BT USB dongles are built on it), updating the advertising data after enabling the instance produces (at least one) corrupted advertising message. Under normal conditions, a single corrupted advertising message would probably not attract much attention, but during MESH provisioning (via MGMT I/O / mesh_send(_sync)), up to 3 different messages (BEACON, ACK, CAPS) are sent within a loop which causes corruption of ALL provisioning messages.
I have no idea whether this could be fixed in the firmware of the USB dongles (I didn't even find the chip on the Realtek homepage), but generally I would suggest changing the order of the HCI commands as this matches the command order for "non-extended adv capable" controllers and simply is more natural.
This patch only considers advertising instances with handle > 0, I don't know whether this should be extended to further cases.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [6] https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-6...
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
include/net/bluetooth/hci_core.h | 1 + include/net/bluetooth/hci_sync.h | 1 + net/bluetooth/hci_event.c | 33 +++++++++++++++++++++++++++++ net/bluetooth/hci_sync.c | 36 ++++++++++++++++++++++++++------ 4 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 9fc8f544e20e..8d37f127ddba 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -237,6 +237,7 @@ struct oob_data {
struct adv_info { struct list_head list;
bool enable_after_set_ext_data; bool enabled; bool pending; bool periodic;diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index 5224f57f6af2..00eceffeec87 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -112,6 +112,7 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance, int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance); int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance); int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance); +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance); int hci_enable_advertising_sync(struct hci_dev *hdev); int hci_enable_advertising(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 66052d6aaa1d..eb018d8a3c4b 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -2184,6 +2184,37 @@ static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data, return rp->status; }
+static u8 hci_cc_le_set_ext_adv_data(struct hci_dev *hdev, void *data,
struct sk_buff *skb)+{
struct hci_cp_le_set_ext_adv_data *cp;struct hci_ev_status *rp = data;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_DATA);if (!cp)return rp->status;hci_dev_lock(hdev);if (cp->handle) {adv_instance = hci_find_adv_instance(hdev, cp->handle);if (adv_instance) {if (adv_instance->enable_after_set_ext_data)hci_enable_ext_advertising(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) { @@ -4166,6 +4197,8 @@ static const struct hci_cc { 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_DATA,hci_cc_le_set_ext_adv_data), 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..da0e39cce721 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -1262,6 +1262,7 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance) hci_cpu_to_le24(adv->max_interval, cp.max_interval); cp.tx_power = adv->tx_power; cp.sid = adv->sid;
adv->enable_after_set_ext_data = true; } else { hci_cpu_to_le24(hdev->le_adv_min_interval, cp.min_interval); hci_cpu_to_le24(hdev->le_adv_max_interval, cp.max_interval);@@ -1456,6 +1457,23 @@ int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance) data, HCI_CMD_TIMEOUT); }
+static int enable_ext_advertising_sync(struct hci_dev *hdev, void *data) +{
u8 instance = PTR_UINT(data);return hci_enable_ext_advertising_sync(hdev, instance);+}
+int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance) +{
if (!hci_dev_test_flag(hdev, HCI_ADVERTISING) &&list_empty(&hdev->adv_instances))return 0;return hci_cmd_sync_queue(hdev, enable_ext_advertising_sync,UINT_PTR(instance), NULL);+}
int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) { int err; @@ -1464,11 +1482,11 @@ int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) if (err) return err;
err = hci_set_ext_scan_rsp_data_sync(hdev, instance);if (err)return err;return hci_enable_ext_advertising_sync(hdev, instance);
/* SET_EXT_ADV_DATA and SET_EXT_ADV_ENABLE are called in the* asynchronous response chain of set_ext_adv_params in order to* set the advertising data first prior enabling it.*/Doing things asynchronously is known to create problems, which is why we introduced the cmd_sync infra to handle a chain of commands like this, so Id suggest sticking to the synchronous way, if the order needs to be changed then use a quirk to detect it and then make sure the instance is disabled on hci_set_ext_adv_data_sync and then re-enable after updating it.
Directly after creation, the instance is disabled (which is fine). In my opinion, the problem is then caused by enabling the instance _before_ setting the advertisement data.
If the synchronous API is preferred, setting the advertisement data should probably also be done synchronously (e.g. by calling hci_set_ext_adv_data_sync() from hci_start_ext_adv_sync() rather than calling hci_update_adv_data() from hci_cc_set_ext_adv_param()). But I guess that the "tx power" value is only known after hci_cc_set_ext_adv_param() has been run (queued?) and this is probably too late for the synchronous stuff called by hci_start_ext_adv_sync().
return hci_set_ext_scan_rsp_data_sync(hdev, instance);}
int hci_disable_per_advertising_sync(struct hci_dev *hdev, u8 instance) @@ -1832,8 +1850,14 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
if (instance) { adv = hci_find_adv_instance(hdev, instance);
if (!adv || !adv->adv_data_changed)
if (!adv) return 0;if (!adv->adv_data_changed) {if (adv->enable_after_set_ext_data)hci_enable_ext_advertising_sync(hdev,adv->handle);return 0;} } len = eir_create_adv_data(hdev, instance, pdu->data,-- 2.43.0
regards, Christian
regards, Christian
Hi Christian,
On Wed, Jun 25, 2025 at 10:46 AM Christian Eggers ceggers@arri.de wrote:
Hi Luiz,
On Wednesday, 25 June 2025, 15:26:58 CEST, Luiz Augusto von Dentz wrote:
Hi Christian,
On Wed, Jun 25, 2025 at 9:05 AM Christian Eggers ceggers@arri.de wrote:
For extended advertising capable controllers, hci_start_ext_adv_sync() at the moment synchronously calls SET_EXT_ADV_PARAMS [1], SET_ADV_SET_RAND_ADDR [2], SET_EXT_SCAN_RSP_DATA [3](optional) and SET_EXT_ADV_ENABLE [4]. After all synchronous commands are finished, SET_EXT_ADV_DATA is called from the async response handler of SET_EXT_ADV_PARAMS [5] (via hci_update_adv_data).
So the current implementation sets the advertising data AFTER enabling the advertising instance. The BT Core specification explicitly allows for this [6]:
If advertising is currently enabled for the specified advertising set, the Controller shall use the new data in subsequent extended advertising events for this advertising set. If an extended advertising event is in progress when this command is issued, the Controller may use the old or new data for that event.
Ok, lets stop right here, if the controller deviates from the spec it needs a quirk and not make the whole stack work around a bug in the firmware.
I generally agree! In this particular case, I think that the current order of advertising commands may be the result of "random" and was probably not intended this way. While the command order of the "legacy" advertising commands looks perfectly logical for me, the order of the "extended" commands seems to be broken by setting the advertising data in the asynchronous response handler of set_ext_adv_params.
Yeah, the advertising data shall be set synchronously as well, if you need anything from the command response there are variants that return the response as skb so it can be processed.
In case of the Realtek RTL8761BU chip (almost all contemporary BT USB dongles are built on it), updating the advertising data after enabling the instance produces (at least one) corrupted advertising message. Under normal conditions, a single corrupted advertising message would probably not attract much attention, but during MESH provisioning (via MGMT I/O / mesh_send(_sync)), up to 3 different messages (BEACON, ACK, CAPS) are sent within a loop which causes corruption of ALL provisioning messages.
I have no idea whether this could be fixed in the firmware of the USB dongles (I didn't even find the chip on the Realtek homepage), but generally I would suggest changing the order of the HCI commands as this matches the command order for "non-extended adv capable" controllers and simply is more natural.
This patch only considers advertising instances with handle > 0, I don't know whether this should be extended to further cases.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... [6] https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-6...
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
include/net/bluetooth/hci_core.h | 1 + include/net/bluetooth/hci_sync.h | 1 + net/bluetooth/hci_event.c | 33 +++++++++++++++++++++++++++++ net/bluetooth/hci_sync.c | 36 ++++++++++++++++++++++++++------ 4 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 9fc8f544e20e..8d37f127ddba 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -237,6 +237,7 @@ struct oob_data {
struct adv_info { struct list_head list;
bool enable_after_set_ext_data; bool enabled; bool pending; bool periodic;diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index 5224f57f6af2..00eceffeec87 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -112,6 +112,7 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance, int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance); int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance); int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance); +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance); int hci_enable_advertising_sync(struct hci_dev *hdev); int hci_enable_advertising(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 66052d6aaa1d..eb018d8a3c4b 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -2184,6 +2184,37 @@ static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data, return rp->status; }
+static u8 hci_cc_le_set_ext_adv_data(struct hci_dev *hdev, void *data,
struct sk_buff *skb)+{
struct hci_cp_le_set_ext_adv_data *cp;struct hci_ev_status *rp = data;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_DATA);if (!cp)return rp->status;hci_dev_lock(hdev);if (cp->handle) {adv_instance = hci_find_adv_instance(hdev, cp->handle);if (adv_instance) {if (adv_instance->enable_after_set_ext_data)hci_enable_ext_advertising(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) { @@ -4166,6 +4197,8 @@ static const struct hci_cc { 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_DATA,hci_cc_le_set_ext_adv_data), 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..da0e39cce721 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -1262,6 +1262,7 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance) hci_cpu_to_le24(adv->max_interval, cp.max_interval); cp.tx_power = adv->tx_power; cp.sid = adv->sid;
adv->enable_after_set_ext_data = true; } else { hci_cpu_to_le24(hdev->le_adv_min_interval, cp.min_interval); hci_cpu_to_le24(hdev->le_adv_max_interval, cp.max_interval);@@ -1456,6 +1457,23 @@ int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance) data, HCI_CMD_TIMEOUT); }
+static int enable_ext_advertising_sync(struct hci_dev *hdev, void *data) +{
u8 instance = PTR_UINT(data);return hci_enable_ext_advertising_sync(hdev, instance);+}
+int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance) +{
if (!hci_dev_test_flag(hdev, HCI_ADVERTISING) &&list_empty(&hdev->adv_instances))return 0;return hci_cmd_sync_queue(hdev, enable_ext_advertising_sync,UINT_PTR(instance), NULL);+}
int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) { int err; @@ -1464,11 +1482,11 @@ int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance) if (err) return err;
err = hci_set_ext_scan_rsp_data_sync(hdev, instance);if (err)return err;return hci_enable_ext_advertising_sync(hdev, instance);
/* SET_EXT_ADV_DATA and SET_EXT_ADV_ENABLE are called in the* asynchronous response chain of set_ext_adv_params in order to* set the advertising data first prior enabling it.*/Doing things asynchronously is known to create problems, which is why we introduced the cmd_sync infra to handle a chain of commands like this, so Id suggest sticking to the synchronous way, if the order needs to be changed then use a quirk to detect it and then make sure the instance is disabled on hci_set_ext_adv_data_sync and then re-enable after updating it.
Directly after creation, the instance is disabled (which is fine). In my opinion, the problem is then caused by enabling the instance _before_ setting the advertisement data.
If the synchronous API is preferred, setting the advertisement data should probably also be done synchronously (e.g. by calling hci_set_ext_adv_data_sync() from hci_start_ext_adv_sync() rather than calling hci_update_adv_data() from hci_cc_set_ext_adv_param()). But I guess that the "tx power" value is only known after hci_cc_set_ext_adv_param() has been run (queued?) and this is probably too late for the synchronous stuff called by hci_start_ext_adv_sync().
Not really, like I said there is the likes of __hci_cmd_sync if you want to process the response directly, so the logic on hci_cc_set_ext_adv_param is not really necessary if we do that, this might explain why it may seems out of order since hci_update_adv_data will queue the command to be run after the cmd_sync is done programming the existing instance.
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.
return hci_set_ext_scan_rsp_data_sync(hdev, instance);}
int hci_disable_per_advertising_sync(struct hci_dev *hdev, u8 instance) @@ -1832,8 +1850,14 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
if (instance) { adv = hci_find_adv_instance(hdev, instance);
if (!adv || !adv->adv_data_changed)
if (!adv) return 0;if (!adv->adv_data_changed) {if (adv->enable_after_set_ext_data)hci_enable_ext_advertising_sync(hdev,adv->handle);return 0;} } len = eir_create_adv_data(hdev, instance, pdu->data,-- 2.43.0
regards, Christian
regards, Christian
linux-stable-mirror@lists.linaro.org
-
Christian Eggers -
Luiz Augusto von Dentz