Hi Maintainers,
This patch series defines the new two-call MGMT interface for adding
new advertising instances. Similarly to the hci advertising commands, a
mgmt call to set parameters is expected to be first, followed by a mgmt
call to set advertising data/scan response. The members of the
parameters request are optional; the caller defines a "params" bitfield
in the structure that indicates which parameters were intentionally set,
and others are set to defaults.
The main feature here is the introduction of min/max parameters and tx
power that can be requested by the client. Min/max parameters will be
used both with and without extended advertising support, and tx power
will be used with extended advertising support. After a call for hci
advertising parameters, a new TX_POWER_SELECTED event will be emitted to
alert userspace to the actual chosen tx power.
Additionally, to inform userspace of the controller LE Tx power
capabilities for the client's benefit, this series also changes the
security info MGMT command to more flexibly contain other capabilities,
such as LE min and max tx power.
All changes have been tested on hatch (extended advertising) and kukui
(no extended advertising) chromebooks with manual testing verifying
correctness of parameters/data in btmon traces, and our automated test
suite of 25 single- and multi-advertising usage scenarios.
A separate patch series will add support in bluetoothd. Thanks in
advance for your feedback!
Daniel Winkler
Changes in v4:
- Add remaining data and scan response length to MGMT params response
- Moving optional params into 'flags' field of MGMT command
- Combine LE tx range into a single EIR field for MGMT capabilities cmd
Changes in v3:
- Adding selected tx power to adv params mgmt response, removing event
- Re-using security info MGMT command to carry controller capabilities
Changes in v2:
- Fixed sparse error in Capabilities MGMT command
Daniel Winkler (5):
Bluetooth: Add helper to set adv data
Bluetooth: Break add adv into two mgmt commands
Bluetooth: Use intervals and tx power from mgmt cmds
Bluetooth: Query LE tx power on startup
Bluetooth: Change MGMT security info CMD to be more generic
include/net/bluetooth/hci.h | 7 +
include/net/bluetooth/hci_core.h | 12 +-
include/net/bluetooth/mgmt.h | 49 +++-
net/bluetooth/hci_core.c | 47 +++-
net/bluetooth/hci_event.c | 19 ++
net/bluetooth/hci_request.c | 29 ++-
net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++--
7 files changed, 542 insertions(+), 45 deletions(-)
--
2.28.0.709.gb0816b6eb0-goog
We wish to handle advertising data separately from advertising
parameters in our new MGMT requests. This change adds a helper that
allows the advertising data and scan response to be updated for an
existing advertising instance.
Reviewed-by: Sonny Sasaka <[email protected]>
Signed-off-by: Daniel Winkler <[email protected]>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
include/net/bluetooth/hci_core.h | 3 +++
net/bluetooth/hci_core.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9873e1c8cd163b..300b3572d479e1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1291,6 +1291,9 @@ int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
u16 adv_data_len, u8 *adv_data,
u16 scan_rsp_len, u8 *scan_rsp_data,
u16 timeout, u16 duration);
+int hci_set_adv_instance_data(struct hci_dev *hdev, u8 instance,
+ u16 adv_data_len, u8 *adv_data,
+ u16 scan_rsp_len, u8 *scan_rsp_data);
int hci_remove_adv_instance(struct hci_dev *hdev, u8 instance);
void hci_adv_instances_set_rpa_expired(struct hci_dev *hdev, bool rpa_expired);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8a2645a8330137..3f73f147826409 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3005,6 +3005,37 @@ int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
return 0;
}
+/* This function requires the caller holds hdev->lock */
+int hci_set_adv_instance_data(struct hci_dev *hdev, u8 instance,
+ u16 adv_data_len, u8 *adv_data,
+ u16 scan_rsp_len, u8 *scan_rsp_data)
+{
+ struct adv_info *adv_instance;
+
+ adv_instance = hci_find_adv_instance(hdev, instance);
+
+ /* If advertisement doesn't exist, we can't modify its data */
+ if (!adv_instance)
+ return -ENOENT;
+
+ if (adv_data_len) {
+ memset(adv_instance->adv_data, 0,
+ sizeof(adv_instance->adv_data));
+ memcpy(adv_instance->adv_data, adv_data, adv_data_len);
+ adv_instance->adv_data_len = adv_data_len;
+ }
+
+ if (scan_rsp_len) {
+ memset(adv_instance->scan_rsp_data, 0,
+ sizeof(adv_instance->scan_rsp_data));
+ memcpy(adv_instance->scan_rsp_data,
+ scan_rsp_data, scan_rsp_len);
+ adv_instance->scan_rsp_len = scan_rsp_len;
+ }
+
+ return 0;
+}
+
/* This function requires the caller holds hdev->lock */
void hci_adv_monitors_clear(struct hci_dev *hdev)
{
--
2.28.0.709.gb0816b6eb0-goog
This patch adds support for the new advertising add interface, with the
first command setting advertising parameters and the second to set
advertising data. The set parameters command allows the caller to leave
some fields "unset", with a params bitfield defining which params were
purposefully set. Unset parameters will be given defaults when calling
hci_add_adv_instance. The data passed to the param mgmt command is
allowed to be flexible, so in the future if bluetoothd passes a larger
structure with new params, the mgmt command will ignore the unknown
members at the end.
This change has been validated on both hatch (extended advertising) and
kukui (no extended advertising) chromebooks running bluetoothd that
support this new interface. I ran the following manual tests:
- Set several (3) advertisements using modified test_advertisement.py
- For each, validate correct data and parameters in btmon trace
- Verified both for software rotation and extended adv
Automatic test suite also run, testing many (25) scenarios of single and
multi-advertising for data/parameter correctness.
Reviewed-by: Sonny Sasaka <[email protected]>
Signed-off-by: Daniel Winkler <[email protected]>
---
Changes in v4:
- Add remaining data and scan response length to MGMT params response
- Moving optional params into 'flags' field of MGMT command
Changes in v3:
- Adding selected tx power to adv params mgmt response, removing event
Changes in v2: None
include/net/bluetooth/hci_core.h | 2 +
include/net/bluetooth/mgmt.h | 34 +++
net/bluetooth/hci_event.c | 1 +
net/bluetooth/mgmt.c | 379 ++++++++++++++++++++++++++++++-
4 files changed, 405 insertions(+), 11 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 300b3572d479e1..48d144ae8b57d6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -238,6 +238,8 @@ struct adv_info {
#define HCI_MAX_ADV_INSTANCES 5
#define HCI_DEFAULT_ADV_DURATION 2
+#define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F
+
struct adv_pattern {
struct list_head list;
__u8 ad_type;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 6b55155e05e977..117578bb88743c 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -574,6 +574,10 @@ struct mgmt_rp_add_advertising {
#define MGMT_ADV_FLAG_SEC_CODED BIT(9)
#define MGMT_ADV_FLAG_CAN_SET_TX_POWER BIT(10)
#define MGMT_ADV_FLAG_HW_OFFLOAD BIT(11)
+#define MGMT_ADV_PARAM_DURATION BIT(12)
+#define MGMT_ADV_PARAM_TIMEOUT BIT(13)
+#define MGMT_ADV_PARAM_INTERVALS BIT(14)
+#define MGMT_ADV_PARAM_TX_POWER BIT(15)
#define MGMT_ADV_FLAG_SEC_MASK (MGMT_ADV_FLAG_SEC_1M | MGMT_ADV_FLAG_SEC_2M | \
MGMT_ADV_FLAG_SEC_CODED)
@@ -782,6 +786,36 @@ struct mgmt_rp_remove_adv_monitor {
__le16 monitor_handle;
} __packed;
+#define MGMT_OP_ADD_EXT_ADV_PARAMS 0x0054
+struct mgmt_cp_add_ext_adv_params {
+ __u8 instance;
+ __le32 flags;
+ __le16 duration;
+ __le16 timeout;
+ __le32 min_interval;
+ __le32 max_interval;
+ __s8 tx_power;
+} __packed;
+#define MGMT_ADD_EXT_ADV_PARAMS_MIN_SIZE 18
+struct mgmt_rp_add_ext_adv_params {
+ __u8 instance;
+ __s8 tx_power;
+ __u8 max_adv_data_len;
+ __u8 max_scan_rsp_len;
+} __packed;
+
+#define MGMT_OP_ADD_EXT_ADV_DATA 0x0055
+struct mgmt_cp_add_ext_adv_data {
+ __u8 instance;
+ __u8 adv_data_len;
+ __u8 scan_rsp_len;
+ __u8 data[];
+} __packed;
+#define MGMT_ADD_EXT_ADV_DATA_SIZE 3
+struct mgmt_rp_add_ext_adv_data {
+ __u8 instance;
+} __packed;
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bd306ba3ade545..4281f4ac6700d4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1749,6 +1749,7 @@ static void hci_cc_set_ext_adv_param(struct hci_dev *hdev, struct sk_buff *skb)
}
/* Update adv data as tx power is known now */
hci_req_update_adv_data(hdev, hdev->cur_adv_instance);
+
hci_dev_unlock(hdev);
}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0b711ad80f6bd1..425d04722aa962 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -122,6 +122,8 @@ static const u16 mgmt_commands[] = {
MGMT_OP_READ_ADV_MONITOR_FEATURES,
MGMT_OP_ADD_ADV_PATTERNS_MONITOR,
MGMT_OP_REMOVE_ADV_MONITOR,
+ MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_OP_ADD_EXT_ADV_DATA,
};
static const u16 mgmt_events[] = {
@@ -7198,6 +7200,10 @@ static u32 get_supported_adv_flags(struct hci_dev *hdev)
flags |= MGMT_ADV_FLAG_MANAGED_FLAGS;
flags |= MGMT_ADV_FLAG_APPEARANCE;
flags |= MGMT_ADV_FLAG_LOCAL_NAME;
+ flags |= MGMT_ADV_PARAM_DURATION;
+ flags |= MGMT_ADV_PARAM_TIMEOUT;
+ flags |= MGMT_ADV_PARAM_INTERVALS;
+ flags |= MGMT_ADV_PARAM_TX_POWER;
/* In extended adv TX_POWER returned from Set Adv Param
* will be always valid.
@@ -7372,6 +7378,31 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
return true;
}
+static bool requested_adv_flags_are_valid(struct hci_dev *hdev, u32 adv_flags)
+{
+ u32 supported_flags, phy_flags;
+
+ /* The current implementation only supports a subset of the specified
+ * flags. Also need to check mutual exclusiveness of sec flags.
+ */
+ supported_flags = get_supported_adv_flags(hdev);
+ phy_flags = adv_flags & MGMT_ADV_FLAG_SEC_MASK;
+ if (adv_flags & ~supported_flags ||
+ ((phy_flags && (phy_flags ^ (phy_flags & -phy_flags)))))
+ return false;
+
+ return true;
+}
+
+static bool adv_busy(struct hci_dev *hdev)
+{
+ return (pending_find(MGMT_OP_ADD_ADVERTISING, hdev) ||
+ pending_find(MGMT_OP_REMOVE_ADVERTISING, hdev) ||
+ pending_find(MGMT_OP_SET_LE, hdev) ||
+ pending_find(MGMT_OP_ADD_EXT_ADV_PARAMS, hdev) ||
+ pending_find(MGMT_OP_ADD_EXT_ADV_DATA, hdev));
+}
+
static void add_advertising_complete(struct hci_dev *hdev, u8 status,
u16 opcode)
{
@@ -7386,6 +7417,8 @@ static void add_advertising_complete(struct hci_dev *hdev, u8 status,
hci_dev_lock(hdev);
cmd = pending_find(MGMT_OP_ADD_ADVERTISING, hdev);
+ if (!cmd)
+ cmd = pending_find(MGMT_OP_ADD_EXT_ADV_DATA, hdev);
list_for_each_entry_safe(adv_instance, n, &hdev->adv_instances, list) {
if (!adv_instance->pending)
@@ -7430,7 +7463,6 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
struct mgmt_cp_add_advertising *cp = data;
struct mgmt_rp_add_advertising rp;
u32 flags;
- u32 supported_flags, phy_flags;
u8 status;
u16 timeout, duration;
unsigned int prev_instance_cnt = hdev->adv_instance_cnt;
@@ -7466,13 +7498,7 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
timeout = __le16_to_cpu(cp->timeout);
duration = __le16_to_cpu(cp->duration);
- /* The current implementation only supports a subset of the specified
- * flags. Also need to check mutual exclusiveness of sec flags.
- */
- supported_flags = get_supported_adv_flags(hdev);
- phy_flags = flags & MGMT_ADV_FLAG_SEC_MASK;
- if (flags & ~supported_flags ||
- ((phy_flags && (phy_flags ^ (phy_flags & -phy_flags)))))
+ if (!requested_adv_flags_are_valid(hdev, flags))
return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
MGMT_STATUS_INVALID_PARAMS);
@@ -7484,9 +7510,7 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
goto unlock;
}
- if (pending_find(MGMT_OP_ADD_ADVERTISING, hdev) ||
- pending_find(MGMT_OP_REMOVE_ADVERTISING, hdev) ||
- pending_find(MGMT_OP_SET_LE, hdev)) {
+ if (adv_busy(hdev)) {
err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
MGMT_STATUS_BUSY);
goto unlock;
@@ -7577,6 +7601,335 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
return err;
}
+static void add_ext_adv_params_complete(struct hci_dev *hdev, u8 status,
+ u16 opcode)
+{
+ struct mgmt_pending_cmd *cmd;
+ struct mgmt_cp_add_ext_adv_params *cp;
+ struct mgmt_rp_add_ext_adv_params rp;
+ struct adv_info *adv_instance;
+ u32 flags;
+
+ BT_DBG("%s", hdev->name);
+
+ hci_dev_lock(hdev);
+
+ cmd = pending_find(MGMT_OP_ADD_EXT_ADV_PARAMS, hdev);
+ if (!cmd)
+ goto unlock;
+
+ cp = cmd->param;
+ adv_instance = hci_find_adv_instance(hdev, cp->instance);
+ if (!adv_instance)
+ goto unlock;
+
+ rp.instance = cp->instance;
+ rp.tx_power = adv_instance->tx_power;
+
+ /* While we're at it, inform userspace of the available space for this
+ * advertisement, given the flags that will be used.
+ */
+ flags = __le32_to_cpu(cp->flags);
+ rp.max_adv_data_len = tlv_data_max_len(hdev, flags, true);
+ rp.max_scan_rsp_len = tlv_data_max_len(hdev, flags, false);
+
+ if (status) {
+ /* If this advertisement was previously advertising and we
+ * failed to update it, we signal that it has been removed and
+ * delete its structure
+ */
+ if (!adv_instance->pending)
+ mgmt_advertising_removed(cmd->sk, hdev, cp->instance);
+
+ hci_remove_adv_instance(hdev, cp->instance);
+
+ mgmt_cmd_status(cmd->sk, cmd->index, cmd->opcode,
+ mgmt_status(status));
+
+ } else {
+ mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
+ mgmt_status(status), &rp, sizeof(rp));
+ }
+
+unlock:
+ if (cmd)
+ mgmt_pending_remove(cmd);
+
+ hci_dev_unlock(hdev);
+}
+
+static int add_ext_adv_params(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 data_len)
+{
+ struct mgmt_cp_add_ext_adv_params *cp = data;
+ struct mgmt_rp_add_ext_adv_params rp;
+ struct mgmt_pending_cmd *cmd = NULL;
+ struct adv_info *adv_instance;
+ struct hci_request req;
+ u32 flags, min_interval, max_interval;
+ u16 timeout, duration;
+ u8 status;
+ s8 tx_power;
+ int err;
+
+ BT_DBG("%s", hdev->name);
+
+ status = mgmt_le_support(hdev);
+ if (status)
+ return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ status);
+
+ if (cp->instance < 1 || cp->instance > hdev->le_num_of_adv_sets)
+ return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_STATUS_INVALID_PARAMS);
+
+ /* The purpose of breaking add_advertising into two separate MGMT calls
+ * for params and data is to allow more parameters to be added to this
+ * structure in the future. For this reason, we verify that we have the
+ * bare minimum structure we know of when the interface was defined. Any
+ * extra parameters we don't know about will be ignored in this request.
+ */
+ if (data_len < MGMT_ADD_EXT_ADV_PARAMS_MIN_SIZE)
+ return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
+ MGMT_STATUS_INVALID_PARAMS);
+
+ flags = __le32_to_cpu(cp->flags);
+
+ if (!requested_adv_flags_are_valid(hdev, flags))
+ return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_STATUS_INVALID_PARAMS);
+
+ hci_dev_lock(hdev);
+
+ /* In new interface, we require that we are powered to register */
+ if (!hdev_is_powered(hdev)) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_STATUS_REJECTED);
+ goto unlock;
+ }
+
+ if (adv_busy(hdev)) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_STATUS_BUSY);
+ goto unlock;
+ }
+
+ /* Parse defined parameters from request, use defaults otherwise */
+ timeout = (flags & MGMT_ADV_PARAM_TIMEOUT) ?
+ __le16_to_cpu(cp->timeout) : 0;
+
+ duration = (flags & MGMT_ADV_PARAM_DURATION) ?
+ __le16_to_cpu(cp->duration) :
+ hdev->def_multi_adv_rotation_duration;
+
+ min_interval = (flags & MGMT_ADV_PARAM_INTERVALS) ?
+ __le32_to_cpu(cp->min_interval) :
+ hdev->le_adv_min_interval;
+
+ max_interval = (flags & MGMT_ADV_PARAM_INTERVALS) ?
+ __le32_to_cpu(cp->max_interval) :
+ hdev->le_adv_max_interval;
+
+ tx_power = (flags & MGMT_ADV_PARAM_TX_POWER) ?
+ cp->tx_power :
+ HCI_ADV_TX_POWER_NO_PREFERENCE;
+
+ /* Create advertising instance with no advertising or response data */
+ err = hci_add_adv_instance(hdev, cp->instance, flags,
+ 0, NULL, 0, NULL, timeout, duration);
+
+ if (err < 0) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_STATUS_FAILED);
+ goto unlock;
+ }
+
+ hdev->cur_adv_instance = cp->instance;
+ /* Submit request for advertising params if ext adv available */
+ if (ext_adv_capable(hdev)) {
+ hci_req_init(&req, hdev);
+ adv_instance = hci_find_adv_instance(hdev, cp->instance);
+
+ /* Updating parameters of an active instance will return a
+ * Command Disallowed error, so we must first disable the
+ * instance if it is active.
+ */
+ if (!adv_instance->pending)
+ __hci_req_disable_ext_adv_instance(&req, cp->instance);
+
+ __hci_req_setup_ext_adv_instance(&req, cp->instance);
+
+ err = hci_req_run(&req, add_ext_adv_params_complete);
+
+ if (!err)
+ cmd = mgmt_pending_add(sk, MGMT_OP_ADD_EXT_ADV_PARAMS,
+ hdev, data, data_len);
+ if (!cmd) {
+ err = -ENOMEM;
+ hci_remove_adv_instance(hdev, cp->instance);
+ goto unlock;
+ }
+
+ } else {
+ rp.instance = cp->instance;
+ rp.tx_power = HCI_ADV_TX_POWER_NO_PREFERENCE;
+ err = mgmt_cmd_complete(sk, hdev->id,
+ MGMT_OP_ADD_EXT_ADV_PARAMS,
+ MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
+ }
+
+unlock:
+ hci_dev_unlock(hdev);
+
+ return err;
+}
+
+static int add_ext_adv_data(struct sock *sk, struct hci_dev *hdev, void *data,
+ u16 data_len)
+{
+ struct mgmt_cp_add_ext_adv_data *cp = data;
+ struct mgmt_rp_add_ext_adv_data rp;
+ u8 schedule_instance = 0;
+ struct adv_info *next_instance;
+ struct adv_info *adv_instance;
+ int err = 0;
+ struct mgmt_pending_cmd *cmd;
+ struct hci_request req;
+
+ BT_DBG("%s", hdev->name);
+
+ hci_dev_lock(hdev);
+
+ adv_instance = hci_find_adv_instance(hdev, cp->instance);
+
+ if (!adv_instance) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_DATA,
+ MGMT_STATUS_INVALID_PARAMS);
+ goto unlock;
+ }
+
+ /* In new interface, we require that we are powered to register */
+ if (!hdev_is_powered(hdev)) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_DATA,
+ MGMT_STATUS_REJECTED);
+ goto clear_new_instance;
+ }
+
+ if (adv_busy(hdev)) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_DATA,
+ MGMT_STATUS_BUSY);
+ goto clear_new_instance;
+ }
+
+ /* Validate new data */
+ if (!tlv_data_is_valid(hdev, adv_instance->flags, cp->data,
+ cp->adv_data_len, true) ||
+ !tlv_data_is_valid(hdev, adv_instance->flags, cp->data +
+ cp->adv_data_len, cp->scan_rsp_len, false)) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_DATA,
+ MGMT_STATUS_INVALID_PARAMS);
+ goto clear_new_instance;
+ }
+
+ /* Set the data in the advertising instance */
+ hci_set_adv_instance_data(hdev, cp->instance, cp->adv_data_len,
+ cp->data, cp->scan_rsp_len,
+ cp->data + cp->adv_data_len);
+
+ /* We're good to go, update advertising data, parameters, and start
+ * advertising.
+ */
+
+ hci_req_init(&req, hdev);
+
+ hci_req_add(&req, HCI_OP_READ_LOCAL_NAME, 0, NULL);
+
+ if (ext_adv_capable(hdev)) {
+ __hci_req_update_adv_data(&req, cp->instance);
+ __hci_req_update_scan_rsp_data(&req, cp->instance);
+ __hci_req_enable_ext_advertising(&req, cp->instance);
+
+ } else {
+ /* If using software rotation, determine next instance to use */
+
+ if (hdev->cur_adv_instance == cp->instance) {
+ /* If the currently advertised instance is being changed
+ * then cancel the current advertising and schedule the
+ * next instance. If there is only one instance then the
+ * overridden advertising data will be visible right
+ * away
+ */
+ cancel_adv_timeout(hdev);
+
+ next_instance = hci_get_next_instance(hdev,
+ cp->instance);
+ if (next_instance)
+ schedule_instance = next_instance->instance;
+ } else if (!hdev->adv_instance_timeout) {
+ /* Immediately advertise the new instance if no other
+ * instance is currently being advertised.
+ */
+ schedule_instance = cp->instance;
+ }
+
+ /* If the HCI_ADVERTISING flag is set or there is no instance to
+ * be advertised then we have no HCI communication to make.
+ * Simply return.
+ */
+ if (hci_dev_test_flag(hdev, HCI_ADVERTISING) ||
+ !schedule_instance) {
+ if (adv_instance->pending) {
+ mgmt_advertising_added(sk, hdev, cp->instance);
+ adv_instance->pending = false;
+ }
+ rp.instance = cp->instance;
+ err = mgmt_cmd_complete(sk, hdev->id,
+ MGMT_OP_ADD_EXT_ADV_DATA,
+ MGMT_STATUS_SUCCESS, &rp,
+ sizeof(rp));
+ goto unlock;
+ }
+
+ err = __hci_req_schedule_adv_instance(&req, schedule_instance,
+ true);
+ }
+
+ cmd = mgmt_pending_add(sk, MGMT_OP_ADD_EXT_ADV_DATA, hdev, data,
+ data_len);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto clear_new_instance;
+ }
+
+ if (!err)
+ err = hci_req_run(&req, add_advertising_complete);
+
+ if (err < 0) {
+ err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_DATA,
+ MGMT_STATUS_FAILED);
+ mgmt_pending_remove(cmd);
+ goto clear_new_instance;
+ }
+
+ /* We were successful in updating data, so trigger advertising_added
+ * event if this is an instance that wasn't previously advertising. If
+ * a failure occurs in the requests we initiated, we will remove the
+ * instance again in add_advertising_complete
+ */
+ if (adv_instance->pending)
+ mgmt_advertising_added(sk, hdev, cp->instance);
+
+ goto unlock;
+
+clear_new_instance:
+ hci_remove_adv_instance(hdev, cp->instance);
+
+unlock:
+ hci_dev_unlock(hdev);
+
+ return err;
+}
+
static void remove_advertising_complete(struct hci_dev *hdev, u8 status,
u16 opcode)
{
@@ -7851,6 +8204,10 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
{ add_adv_patterns_monitor,MGMT_ADD_ADV_PATTERNS_MONITOR_SIZE,
HCI_MGMT_VAR_LEN },
{ remove_adv_monitor, MGMT_REMOVE_ADV_MONITOR_SIZE },
+ { add_ext_adv_params, MGMT_ADD_EXT_ADV_PARAMS_MIN_SIZE,
+ HCI_MGMT_VAR_LEN },
+ { add_ext_adv_data, MGMT_ADD_EXT_ADV_DATA_SIZE,
+ HCI_MGMT_VAR_LEN },
};
void mgmt_index_added(struct hci_dev *hdev)
--
2.28.0.709.gb0816b6eb0-goog
For advertising, we wish to know the LE tx power capabilities of the
controller in userspace, so this patch edits the Security Info MGMT
command to be more generic, such that other various controller
capabilities can be included in the EIR data. This change also includes
the LE min and max tx power into this newly-named command.
The change was tested by manually verifying that the MGMT command
returns the tx power range as expected in userspace.
Reviewed-by: Sonny Sasaka <[email protected]>
Signed-off-by: Daniel Winkler <[email protected]>
---
Changes in v4:
- Combine LE tx range into a single EIR field for MGMT capabilities cmd
Changes in v3:
- Re-using security info MGMT command to carry controller capabilities
Changes in v2:
- Fixed sparse error in Capabilities MGMT command
include/net/bluetooth/mgmt.h | 15 +++++++++-----
net/bluetooth/mgmt.c | 39 +++++++++++++++++++++++-------------
2 files changed, 35 insertions(+), 19 deletions(-)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 117578bb88743c..42c3ece9931936 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -686,11 +686,16 @@ struct mgmt_cp_set_blocked_keys {
#define MGMT_OP_SET_WIDEBAND_SPEECH 0x0047
-#define MGMT_OP_READ_SECURITY_INFO 0x0048
-#define MGMT_READ_SECURITY_INFO_SIZE 0
-struct mgmt_rp_read_security_info {
- __le16 sec_len;
- __u8 sec[];
+#define MGMT_CAP_SEC_FLAGS 0x01
+#define MGMT_CAP_MAX_ENC_KEY_SIZE 0x02
+#define MGMT_CAP_SMP_MAX_ENC_KEY_SIZE 0x03
+#define MGMT_CAP_LE_TX_PWR 0x04
+
+#define MGMT_OP_READ_CONTROLLER_CAP 0x0048
+#define MGMT_READ_CONTROLLER_CAP_SIZE 0
+struct mgmt_rp_read_controller_cap {
+ __le16 cap_len;
+ __u8 cap[0];
} __packed;
#define MGMT_OP_READ_EXP_FEATURES_INFO 0x0049
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 092ac7fffb4124..3a35385017f568 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -110,7 +110,7 @@ static const u16 mgmt_commands[] = {
MGMT_OP_SET_APPEARANCE,
MGMT_OP_SET_BLOCKED_KEYS,
MGMT_OP_SET_WIDEBAND_SPEECH,
- MGMT_OP_READ_SECURITY_INFO,
+ MGMT_OP_READ_CONTROLLER_CAP,
MGMT_OP_READ_EXP_FEATURES_INFO,
MGMT_OP_SET_EXP_FEATURE,
MGMT_OP_READ_DEF_SYSTEM_CONFIG,
@@ -176,7 +176,7 @@ static const u16 mgmt_untrusted_commands[] = {
MGMT_OP_READ_CONFIG_INFO,
MGMT_OP_READ_EXT_INDEX_LIST,
MGMT_OP_READ_EXT_INFO,
- MGMT_OP_READ_SECURITY_INFO,
+ MGMT_OP_READ_CONTROLLER_CAP,
MGMT_OP_READ_EXP_FEATURES_INFO,
MGMT_OP_READ_DEF_SYSTEM_CONFIG,
MGMT_OP_READ_DEF_RUNTIME_CONFIG,
@@ -3705,13 +3705,14 @@ static int set_wideband_speech(struct sock *sk, struct hci_dev *hdev,
return err;
}
-static int read_security_info(struct sock *sk, struct hci_dev *hdev,
- void *data, u16 data_len)
+static int read_controller_cap(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 data_len)
{
- char buf[16];
- struct mgmt_rp_read_security_info *rp = (void *)buf;
- u16 sec_len = 0;
+ char buf[20];
+ struct mgmt_rp_read_controller_cap *rp = (void *)buf;
+ u16 cap_len = 0;
u8 flags = 0;
+ u8 tx_power_range[2];
bt_dev_dbg(hdev, "sock %p", sk);
@@ -3735,23 +3736,33 @@ static int read_security_info(struct sock *sk, struct hci_dev *hdev,
flags |= 0x08; /* Encryption key size enforcement (LE) */
- sec_len = eir_append_data(rp->sec, sec_len, 0x01, &flags, 1);
+ cap_len = eir_append_data(rp->cap, cap_len, MGMT_CAP_SEC_FLAGS,
+ &flags, 1);
/* When the Read Simple Pairing Options command is supported, then
* also max encryption key size information is provided.
*/
if (hdev->commands[41] & 0x08)
- sec_len = eir_append_le16(rp->sec, sec_len, 0x02,
+ cap_len = eir_append_le16(rp->cap, cap_len,
+ MGMT_CAP_MAX_ENC_KEY_SIZE,
hdev->max_enc_key_size);
- sec_len = eir_append_le16(rp->sec, sec_len, 0x03, SMP_MAX_ENC_KEY_SIZE);
+ cap_len = eir_append_le16(rp->cap, cap_len,
+ MGMT_CAP_SMP_MAX_ENC_KEY_SIZE,
+ SMP_MAX_ENC_KEY_SIZE);
+
+ /* Append the min/max LE tx power parameters */
+ memcpy(&tx_power_range[0], &hdev->min_le_tx_power, 1);
+ memcpy(&tx_power_range[1], &hdev->max_le_tx_power, 1);
+ cap_len = eir_append_data(rp->cap, cap_len, MGMT_CAP_LE_TX_PWR,
+ tx_power_range, 2);
- rp->sec_len = cpu_to_le16(sec_len);
+ rp->cap_len = cpu_to_le16(cap_len);
hci_dev_unlock(hdev);
- return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_READ_SECURITY_INFO, 0,
- rp, sizeof(*rp) + sec_len);
+ return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_READ_CONTROLLER_CAP, 0,
+ rp, sizeof(*rp) + cap_len);
}
#ifdef CONFIG_BT_FEATURE_DEBUG
@@ -8186,7 +8197,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
{ set_blocked_keys, MGMT_OP_SET_BLOCKED_KEYS_SIZE,
HCI_MGMT_VAR_LEN },
{ set_wideband_speech, MGMT_SETTING_SIZE },
- { read_security_info, MGMT_READ_SECURITY_INFO_SIZE,
+ { read_controller_cap, MGMT_READ_CONTROLLER_CAP_SIZE,
HCI_MGMT_UNTRUSTED },
{ read_exp_features_info, MGMT_READ_EXP_FEATURES_INFO_SIZE,
HCI_MGMT_UNTRUSTED |
--
2.28.0.709.gb0816b6eb0-goog
This patch takes the min/max intervals and tx power optionally provided
in mgmt interface, stores them in the advertisement struct, and uses
them when configuring the hci requests. While tx power is not used if
extended advertising is unavailable, software rotation will use the min
and max advertising intervals specified by the client.
This change is validated manually by ensuring the min/max intervals are
propagated to the controller on both hatch (extended advertising) and
kukui (no extended advertising) chromebooks, and that tx power is
propagated correctly on hatch. These tests are performed with multiple
advertisements simultaneously.
Reviewed-by: Sonny Sasaka <[email protected]>
Signed-off-by: Daniel Winkler <[email protected]>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
include/net/bluetooth/hci_core.h | 5 ++++-
net/bluetooth/hci_core.c | 8 +++++---
net/bluetooth/hci_request.c | 29 +++++++++++++++++++----------
net/bluetooth/mgmt.c | 8 ++++++--
4 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 48d144ae8b57d6..ab168f46b6d909 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -230,6 +230,8 @@ struct adv_info {
__u16 scan_rsp_len;
__u8 scan_rsp_data[HCI_MAX_AD_LENGTH];
__s8 tx_power;
+ __u32 min_interval;
+ __u32 max_interval;
bdaddr_t random_addr;
bool rpa_expired;
struct delayed_work rpa_expired_cb;
@@ -1292,7 +1294,8 @@ struct adv_info *hci_get_next_instance(struct hci_dev *hdev, u8 instance);
int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
u16 adv_data_len, u8 *adv_data,
u16 scan_rsp_len, u8 *scan_rsp_data,
- u16 timeout, u16 duration);
+ u16 timeout, u16 duration, s8 tx_power,
+ u32 min_interval, u32 max_interval);
int hci_set_adv_instance_data(struct hci_dev *hdev, u8 instance,
u16 adv_data_len, u8 *adv_data,
u16 scan_rsp_len, u8 *scan_rsp_data);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3f73f147826409..3a2332f4a9bba2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2951,7 +2951,8 @@ static void adv_instance_rpa_expired(struct work_struct *work)
int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
u16 adv_data_len, u8 *adv_data,
u16 scan_rsp_len, u8 *scan_rsp_data,
- u16 timeout, u16 duration)
+ u16 timeout, u16 duration, s8 tx_power,
+ u32 min_interval, u32 max_interval)
{
struct adv_info *adv_instance;
@@ -2979,6 +2980,9 @@ int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
adv_instance->flags = flags;
adv_instance->adv_data_len = adv_data_len;
adv_instance->scan_rsp_len = scan_rsp_len;
+ adv_instance->min_interval = min_interval;
+ adv_instance->max_interval = max_interval;
+ adv_instance->tx_power = tx_power;
if (adv_data_len)
memcpy(adv_instance->adv_data, adv_data, adv_data_len);
@@ -2995,8 +2999,6 @@ int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
else
adv_instance->duration = duration;
- adv_instance->tx_power = HCI_TX_POWER_INVALID;
-
INIT_DELAYED_WORK(&adv_instance->rpa_expired_cb,
adv_instance_rpa_expired);
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 413e3a5aabf544..bd984b32e07553 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1425,6 +1425,7 @@ static bool is_advertising_allowed(struct hci_dev *hdev, bool connectable)
void __hci_req_enable_advertising(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
+ struct adv_info *adv_instance;
struct hci_cp_le_set_adv_param cp;
u8 own_addr_type, enable = 0x01;
bool connectable;
@@ -1432,6 +1433,7 @@ void __hci_req_enable_advertising(struct hci_request *req)
u32 flags;
flags = get_adv_instance_flags(hdev, hdev->cur_adv_instance);
+ adv_instance = hci_find_adv_instance(hdev, hdev->cur_adv_instance);
/* If the "connectable" instance flag was not set, then choose between
* ADV_IND and ADV_NONCONN_IND based on the global connectable setting.
@@ -1463,11 +1465,16 @@ void __hci_req_enable_advertising(struct hci_request *req)
memset(&cp, 0, sizeof(cp));
- if (connectable) {
- cp.type = LE_ADV_IND;
-
+ if (adv_instance) {
+ adv_min_interval = adv_instance->min_interval;
+ adv_max_interval = adv_instance->max_interval;
+ } else {
adv_min_interval = hdev->le_adv_min_interval;
adv_max_interval = hdev->le_adv_max_interval;
+ }
+
+ if (connectable) {
+ cp.type = LE_ADV_IND;
} else {
if (get_cur_adv_instance_scan_rsp_len(hdev))
cp.type = LE_ADV_SCAN_IND;
@@ -1478,9 +1485,6 @@ void __hci_req_enable_advertising(struct hci_request *req)
hci_dev_test_flag(hdev, HCI_LIMITED_DISCOVERABLE)) {
adv_min_interval = DISCOV_LE_FAST_ADV_INT_MIN;
adv_max_interval = DISCOV_LE_FAST_ADV_INT_MAX;
- } else {
- adv_min_interval = hdev->le_adv_min_interval;
- adv_max_interval = hdev->le_adv_max_interval;
}
}
@@ -1997,9 +2001,15 @@ int __hci_req_setup_ext_adv_instance(struct hci_request *req, u8 instance)
memset(&cp, 0, sizeof(cp));
- /* In ext adv set param interval is 3 octets */
- hci_cpu_to_le24(hdev->le_adv_min_interval, cp.min_interval);
- hci_cpu_to_le24(hdev->le_adv_max_interval, cp.max_interval);
+ if (adv_instance) {
+ hci_cpu_to_le24(adv_instance->min_interval, cp.min_interval);
+ hci_cpu_to_le24(adv_instance->max_interval, cp.max_interval);
+ cp.tx_power = adv_instance->tx_power;
+ } 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);
+ cp.tx_power = HCI_ADV_TX_POWER_NO_PREFERENCE;
+ }
secondary_adv = (flags & MGMT_ADV_FLAG_SEC_MASK);
@@ -2022,7 +2032,6 @@ int __hci_req_setup_ext_adv_instance(struct hci_request *req, u8 instance)
cp.own_addr_type = own_addr_type;
cp.channel_map = hdev->le_adv_channel_map;
- cp.tx_power = 127;
cp.handle = instance;
if (flags & MGMT_ADV_FLAG_SEC_2M) {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 425d04722aa962..092ac7fffb4124 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7528,7 +7528,10 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
cp->adv_data_len, cp->data,
cp->scan_rsp_len,
cp->data + cp->adv_data_len,
- timeout, duration);
+ timeout, duration,
+ HCI_ADV_TX_POWER_NO_PREFERENCE,
+ hdev->le_adv_min_interval,
+ hdev->le_adv_max_interval);
if (err < 0) {
err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
MGMT_STATUS_FAILED);
@@ -7736,7 +7739,8 @@ static int add_ext_adv_params(struct sock *sk, struct hci_dev *hdev,
/* Create advertising instance with no advertising or response data */
err = hci_add_adv_instance(hdev, cp->instance, flags,
- 0, NULL, 0, NULL, timeout, duration);
+ 0, NULL, 0, NULL, timeout, duration,
+ tx_power, min_interval, max_interval);
if (err < 0) {
err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_EXT_ADV_PARAMS,
--
2.28.0.709.gb0816b6eb0-goog
Hello Maintainers,
Just a friendly reminder to review this kernel patch series. I may
have accidentally named this series the same as the userspace series,
so I apologize if it has caused the set to be hidden in anybody's
inbox. I'll be sure not to do this in the future.
Thanks in advance for your time!
Best regards,
Daniel Winkler
On Thu, Oct 1, 2020 at 4:04 PM Daniel Winkler <[email protected]> wrote:
>
> Hi Maintainers,
>
> This patch series defines the new two-call MGMT interface for adding
> new advertising instances. Similarly to the hci advertising commands, a
> mgmt call to set parameters is expected to be first, followed by a mgmt
> call to set advertising data/scan response. The members of the
> parameters request are optional; the caller defines a "params" bitfield
> in the structure that indicates which parameters were intentionally set,
> and others are set to defaults.
>
> The main feature here is the introduction of min/max parameters and tx
> power that can be requested by the client. Min/max parameters will be
> used both with and without extended advertising support, and tx power
> will be used with extended advertising support. After a call for hci
> advertising parameters, a new TX_POWER_SELECTED event will be emitted to
> alert userspace to the actual chosen tx power.
>
> Additionally, to inform userspace of the controller LE Tx power
> capabilities for the client's benefit, this series also changes the
> security info MGMT command to more flexibly contain other capabilities,
> such as LE min and max tx power.
>
> All changes have been tested on hatch (extended advertising) and kukui
> (no extended advertising) chromebooks with manual testing verifying
> correctness of parameters/data in btmon traces, and our automated test
> suite of 25 single- and multi-advertising usage scenarios.
>
> A separate patch series will add support in bluetoothd. Thanks in
> advance for your feedback!
>
> Daniel Winkler
>
>
> Changes in v4:
> - Add remaining data and scan response length to MGMT params response
> - Moving optional params into 'flags' field of MGMT command
> - Combine LE tx range into a single EIR field for MGMT capabilities cmd
>
> Changes in v3:
> - Adding selected tx power to adv params mgmt response, removing event
> - Re-using security info MGMT command to carry controller capabilities
>
> Changes in v2:
> - Fixed sparse error in Capabilities MGMT command
>
> Daniel Winkler (5):
> Bluetooth: Add helper to set adv data
> Bluetooth: Break add adv into two mgmt commands
> Bluetooth: Use intervals and tx power from mgmt cmds
> Bluetooth: Query LE tx power on startup
> Bluetooth: Change MGMT security info CMD to be more generic
>
> include/net/bluetooth/hci.h | 7 +
> include/net/bluetooth/hci_core.h | 12 +-
> include/net/bluetooth/mgmt.h | 49 +++-
> net/bluetooth/hci_core.c | 47 +++-
> net/bluetooth/hci_event.c | 19 ++
> net/bluetooth/hci_request.c | 29 ++-
> net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++--
> 7 files changed, 542 insertions(+), 45 deletions(-)
>
> --
> 2.28.0.709.gb0816b6eb0-goog
>
Hi Daniel,
On Thu, Oct 29, 2020 at 2:35 PM Daniel Winkler <[email protected]> wrote:
>
> Hello Maintainers,
>
> Just a friendly reminder to review this kernel patch series. I may
> have accidentally named this series the same as the userspace series,
> so I apologize if it has caused the set to be hidden in anybody's
> inbox. I'll be sure not to do this in the future.
I will review them coming next, one of the things that seems to be
missing these days is to update mgmt-tester when a new command is
introduced, this should actually be added along side the kernel
changes since we do plan to have the CI verify the kernel patches as
well, also there is a way to test the kernel changes directly in the
host with use of tools/test-runner you just need insure the options
mentioned in doc/test-runner are set so you can run the kernel with
the changes directly.
> Thanks in advance for your time!
>
> Best regards,
> Daniel Winkler
>
> On Thu, Oct 1, 2020 at 4:04 PM Daniel Winkler <[email protected]> wrote:
> >
> > Hi Maintainers,
> >
> > This patch series defines the new two-call MGMT interface for adding
> > new advertising instances. Similarly to the hci advertising commands, a
> > mgmt call to set parameters is expected to be first, followed by a mgmt
> > call to set advertising data/scan response. The members of the
> > parameters request are optional; the caller defines a "params" bitfield
> > in the structure that indicates which parameters were intentionally set,
> > and others are set to defaults.
> >
> > The main feature here is the introduction of min/max parameters and tx
> > power that can be requested by the client. Min/max parameters will be
> > used both with and without extended advertising support, and tx power
> > will be used with extended advertising support. After a call for hci
> > advertising parameters, a new TX_POWER_SELECTED event will be emitted to
> > alert userspace to the actual chosen tx power.
> >
> > Additionally, to inform userspace of the controller LE Tx power
> > capabilities for the client's benefit, this series also changes the
> > security info MGMT command to more flexibly contain other capabilities,
> > such as LE min and max tx power.
> >
> > All changes have been tested on hatch (extended advertising) and kukui
> > (no extended advertising) chromebooks with manual testing verifying
> > correctness of parameters/data in btmon traces, and our automated test
> > suite of 25 single- and multi-advertising usage scenarios.
> >
> > A separate patch series will add support in bluetoothd. Thanks in
> > advance for your feedback!
> >
> > Daniel Winkler
> >
> >
> > Changes in v4:
> > - Add remaining data and scan response length to MGMT params response
> > - Moving optional params into 'flags' field of MGMT command
> > - Combine LE tx range into a single EIR field for MGMT capabilities cmd
> >
> > Changes in v3:
> > - Adding selected tx power to adv params mgmt response, removing event
> > - Re-using security info MGMT command to carry controller capabilities
> >
> > Changes in v2:
> > - Fixed sparse error in Capabilities MGMT command
> >
> > Daniel Winkler (5):
> > Bluetooth: Add helper to set adv data
> > Bluetooth: Break add adv into two mgmt commands
> > Bluetooth: Use intervals and tx power from mgmt cmds
> > Bluetooth: Query LE tx power on startup
> > Bluetooth: Change MGMT security info CMD to be more generic
> >
> > include/net/bluetooth/hci.h | 7 +
> > include/net/bluetooth/hci_core.h | 12 +-
> > include/net/bluetooth/mgmt.h | 49 +++-
> > net/bluetooth/hci_core.c | 47 +++-
> > net/bluetooth/hci_event.c | 19 ++
> > net/bluetooth/hci_request.c | 29 ++-
> > net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++--
> > 7 files changed, 542 insertions(+), 45 deletions(-)
> >
> > --
> > 2.28.0.709.gb0816b6eb0-goog
> >
--
Luiz Augusto von Dentz
Hi Luiz,
Thank you for the feedback regarding mgmt-tester. I intended to use
the tool, but found that it had a very high rate of test failure even
before I started adding new tests. If you have a strong preference for
its use, I can look into it again but it may take some time. These
changes were tested with manual and automated functional testing on
our end.
Please let me know your thoughts.
Thanks,
Daniel
On Thu, Oct 29, 2020 at 2:45 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Daniel,
>
> On Thu, Oct 29, 2020 at 2:35 PM Daniel Winkler <[email protected]> wrote:
> >
> > Hello Maintainers,
> >
> > Just a friendly reminder to review this kernel patch series. I may
> > have accidentally named this series the same as the userspace series,
> > so I apologize if it has caused the set to be hidden in anybody's
> > inbox. I'll be sure not to do this in the future.
>
> I will review them coming next, one of the things that seems to be
> missing these days is to update mgmt-tester when a new command is
> introduced, this should actually be added along side the kernel
> changes since we do plan to have the CI verify the kernel patches as
> well, also there is a way to test the kernel changes directly in the
> host with use of tools/test-runner you just need insure the options
> mentioned in doc/test-runner are set so you can run the kernel with
> the changes directly.
>
> > Thanks in advance for your time!
> >
> > Best regards,
> > Daniel Winkler
> >
> > On Thu, Oct 1, 2020 at 4:04 PM Daniel Winkler <[email protected]> wrote:
> > >
> > > Hi Maintainers,
> > >
> > > This patch series defines the new two-call MGMT interface for adding
> > > new advertising instances. Similarly to the hci advertising commands, a
> > > mgmt call to set parameters is expected to be first, followed by a mgmt
> > > call to set advertising data/scan response. The members of the
> > > parameters request are optional; the caller defines a "params" bitfield
> > > in the structure that indicates which parameters were intentionally set,
> > > and others are set to defaults.
> > >
> > > The main feature here is the introduction of min/max parameters and tx
> > > power that can be requested by the client. Min/max parameters will be
> > > used both with and without extended advertising support, and tx power
> > > will be used with extended advertising support. After a call for hci
> > > advertising parameters, a new TX_POWER_SELECTED event will be emitted to
> > > alert userspace to the actual chosen tx power.
> > >
> > > Additionally, to inform userspace of the controller LE Tx power
> > > capabilities for the client's benefit, this series also changes the
> > > security info MGMT command to more flexibly contain other capabilities,
> > > such as LE min and max tx power.
> > >
> > > All changes have been tested on hatch (extended advertising) and kukui
> > > (no extended advertising) chromebooks with manual testing verifying
> > > correctness of parameters/data in btmon traces, and our automated test
> > > suite of 25 single- and multi-advertising usage scenarios.
> > >
> > > A separate patch series will add support in bluetoothd. Thanks in
> > > advance for your feedback!
> > >
> > > Daniel Winkler
> > >
> > >
> > > Changes in v4:
> > > - Add remaining data and scan response length to MGMT params response
> > > - Moving optional params into 'flags' field of MGMT command
> > > - Combine LE tx range into a single EIR field for MGMT capabilities cmd
> > >
> > > Changes in v3:
> > > - Adding selected tx power to adv params mgmt response, removing event
> > > - Re-using security info MGMT command to carry controller capabilities
> > >
> > > Changes in v2:
> > > - Fixed sparse error in Capabilities MGMT command
> > >
> > > Daniel Winkler (5):
> > > Bluetooth: Add helper to set adv data
> > > Bluetooth: Break add adv into two mgmt commands
> > > Bluetooth: Use intervals and tx power from mgmt cmds
> > > Bluetooth: Query LE tx power on startup
> > > Bluetooth: Change MGMT security info CMD to be more generic
> > >
> > > include/net/bluetooth/hci.h | 7 +
> > > include/net/bluetooth/hci_core.h | 12 +-
> > > include/net/bluetooth/mgmt.h | 49 +++-
> > > net/bluetooth/hci_core.c | 47 +++-
> > > net/bluetooth/hci_event.c | 19 ++
> > > net/bluetooth/hci_request.c | 29 ++-
> > > net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++--
> > > 7 files changed, 542 insertions(+), 45 deletions(-)
> > >
> > > --
> > > 2.28.0.709.gb0816b6eb0-goog
> > >
>
>
>
> --
> Luiz Augusto von Dentz
Hi Daniel,
On Thu, Oct 29, 2020 at 3:25 PM Daniel Winkler <[email protected]> wrote:
>
> Hi Luiz,
>
> Thank you for the feedback regarding mgmt-tester. I intended to use
> the tool, but found that it had a very high rate of test failure even
> before I started adding new tests. If you have a strong preference for
> its use, I can look into it again but it may take some time. These
> changes were tested with manual and automated functional testing on
> our end.
>
> Please let me know your thoughts.
Total: 406, Passed: 358 (88.2%), Failed: 43, Not Run: 5
Looks like there are some 43 tests failing, we will need to fix these
but it should prevent us to add new ones as well, you can use -p to
filter what tests to run if you want to avoid these for now.
Hi Daniel,
On Tue, Nov 3, 2020 at 9:42 AM Daniel Winkler <[email protected]> wrote:
>
> Hello Luiz,
>
> Thank you for the information. It is good to know that this tool is
> actively used and that there is a way to skip existing flaky tests.
> Just for clarification, is this a requirement to land the kernel
> changes, i.e. should I prioritize adding these tests immediately to
> move the process forward? Or can we land the changes based on the
> testing I have already done and I'll work on these tests in parallel?
We used to require updates to mgmt-tester but it seems some of recent
command did not have a test yet, but if we intend to have the CI to
tests the kernel changes properly I think we should start to requiring
it some basic testing, obviously it will be hard to cover everything
that is affected by a new command but the basic formatting, etc, we
should be able to test, also tester supports the concept of 'not run'
which we can probably use for experimental commands.
> Thanks,
> Daniel
>
> On Thu, Oct 29, 2020 at 5:04 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Daniel,
> >
> > On Thu, Oct 29, 2020 at 3:25 PM Daniel Winkler <[email protected]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > Thank you for the feedback regarding mgmt-tester. I intended to use
> > > the tool, but found that it had a very high rate of test failure even
> > > before I started adding new tests. If you have a strong preference for
> > > its use, I can look into it again but it may take some time. These
> > > changes were tested with manual and automated functional testing on
> > > our end.
> > >
> > > Please let me know your thoughts.
> >
> > Total: 406, Passed: 358 (88.2%), Failed: 43, Not Run: 5
> >
> > Looks like there are some 43 tests failing, we will need to fix these
> > but it should prevent us to add new ones as well, you can use -p to
> > filter what tests to run if you want to avoid these for now.
--
Luiz Augusto von Dentz