Hi Christian, On Wed, Jun 25, 2025 at 9:05 AM Christian Eggers <ceggers@xxxxxxx> 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/bluetooth/hci_sync.c#n1319 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1204 > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1471 > [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1469 > [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_event.c#n2180 > [6] https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-60/out/en/host-controller-interface/host-controller-interface-functional-specification.html#UUID-d4f36cb5-f26c-d053-1034-e7a547ed6a13 > > Signed-off-by: Christian Eggers <ceggers@xxxxxxx> > Fixes: a0fb3726ba55 ("Bluetooth: Use Set ext adv/scan rsp data if controller supports") > Cc: stable@xxxxxxxxxxxxxxx > --- > 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 > -- Luiz Augusto von Dentz
