Hi Christian,
On Fri, Jun 27, 2025 at 6:09 AM Christian Eggers <ceggers@xxxxxxx> wrote:
>
> Hi Luiz,
>
> after changing my test setup (I now only use Mesh, no "normal" advertising
> anymore), I get many of these errors:
>
> bluetooth-meshd[276]: @ MGMT Command: Mesh Send (0x0059) plen 40 {0x0001} [hci0] 43.846388
> Address: 00:00:00:00:00:00 (OUI 00-00-00)
> Addr Type: 2
> Instant: 0x0000000000000000
> Delay: 0
> Count: 1
> Data Length: 21
> Data[21]: 142b003a8b6fe779bd4385a94fed0a9cf611880000
> < HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036) plen 25 #479 [hci0] 43.846505
> Handle: 0x05
> Properties: 0x0010
> Use legacy advertising PDUs: ADV_NONCONN_IND
> Min advertising interval: 1280.000 msec (0x0800)
> Max advertising interval: 1280.000 msec (0x0800)
> Channel map: 37, 38, 39 (0x07)
> Own address type: Random (0x01)
> Peer address type: Public (0x00)
> Peer address: 00:00:00:00:00:00 (OUI 00-00-00)
> Filter policy: Allow Scan Request from Any, Allow Connect Request from Any (0x00)
> TX power: Host has no preference (0x7f)
> Primary PHY: LE 1M (0x01)
> Secondary max skip: 0x00
> Secondary PHY: LE 1M (0x01)
> SID: 0x00
> Scan request notifications: Disabled (0x00)
> > HCI Event: Command Complete (0x0e) plen 5 #480 [hci0] 43.847480
> LE Set Extended Advertising Parameters (0x08|0x0036) ncmd 2
> ---> Status: Command Disallowed (0x0c)
> TX power (selected): 0 dbm (0x00)
>
>
> From the btmon output it is obvious that advertising is not disabled before updating the parameters.
>
> I added the following debug line in hci_setup_ext_adv_instance_sync():
>
> printk(KERN_ERR "instance = %u, adv = %p, adv->pending = %d, adv->enabled = %d\n",
> instance, adv, adv ? adv->pending : -1, adv ? adv->enabled : -1);
>
> From the debug output I see that adv->pending is still true (so advertising is not disabled
> before setting the advertising params). After changing the check from
>
> if (adv && !adv->pending) {
>
> to
>
> if (adv && adv->enabled) {
>
> it seems to do the job correctly. What do you think?
Yeah, that is indeed a bug, in fact we can just leave for
hci_disable_ext_adv_instance_sync to detect if the instance is
enabled:
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 2e0e532384c3..13ebd1a380fd 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1228,7 +1228,7 @@ int hci_setup_ext_adv_instance_sync(struct
hci_dev *hdev, u8 instance)
* Command Disallowed error, so we must first disable the
* instance if it is active.
*/
- if (adv && !adv->pending) {
+ if (adv) {
err = hci_disable_ext_adv_instance_sync(hdev, instance);
if (err)
return err;
>
> 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;
> >
> >
>
>
>
>
--
Luiz Augusto von Dentz
