* [PATCH v1 0/3] Support reading and setting default system parameters
@ 2020-06-09 14:03 Alain Michaud
2020-06-09 14:03 ` [PATCH v1 1/3] mgmt: read/set system parameter definitions Alain Michaud
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Alain Michaud @ 2020-06-09 14:03 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Alain Michaud
This series adds support for reading and setting default system
parameters from userspace. In particular, combined with the userspace
changes, allows platforms to override default system parameters from a
main.conf file.
The series has been reviewed and tested on chromeos.
Alain Michaud (3):
mgmt: read/set system parameter definitions
bluetooth:centralize default value initialization.
bluetooth: implement read/set default system parameters mgmt
include/net/bluetooth/hci_core.h | 18 ++
include/net/bluetooth/mgmt.h | 18 ++
net/bluetooth/hci_conn.c | 14 +-
net/bluetooth/hci_core.c | 14 +-
net/bluetooth/hci_request.c | 15 +-
net/bluetooth/mgmt.c | 429 +++++++++++++++++++++++++++++++
6 files changed, 487 insertions(+), 21 deletions(-)
--
2.27.0.278.ge193c7cf3a9-goog
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 1/3] mgmt: read/set system parameter definitions
2020-06-09 14:03 [PATCH v1 0/3] Support reading and setting default system parameters Alain Michaud
@ 2020-06-09 14:03 ` Alain Michaud
2020-06-10 14:16 ` Marcel Holtmann
2020-06-09 14:03 ` [PATCH v1 2/3] bluetooth:centralize default value initialization Alain Michaud
2020-06-09 14:03 ` [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt Alain Michaud
2 siblings, 1 reply; 11+ messages in thread
From: Alain Michaud @ 2020-06-09 14:03 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Alain Michaud, Abhishek Pandit-Subedi, Yu Liu
This patch submits the corresponding kernel definitions to mgmt.h.
This is submitted before the implementation to avoid any conflicts in
values allocations.
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Yu Liu <yudiliu@google.com>
Signed-off-by: Alain Michaud <alainm@chromium.org>
---
include/net/bluetooth/mgmt.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 16e0d87bd8fa..1081e371f03d 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -702,6 +702,24 @@ struct mgmt_rp_set_exp_feature {
__le32 flags;
} __packed;
+#define MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS 0x004b
+
+struct mgmt_system_parameter_tlv {
+ __u16 type;
+ __u8 length;
+ __u8 value[];
+} __packed;
+
+struct mgmt_rp_read_default_system_parameters {
+ __u8 parameters[0]; /* mgmt_system_parameter_tlv */
+} __packed;
+
+#define MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS 0x004c
+
+struct mgmt_cp_set_default_system_parameters {
+ __u8 parameters[0]; /* mgmt_system_parameter_tlv */
+} __packed;
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
--
2.27.0.278.ge193c7cf3a9-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v1 2/3] bluetooth:centralize default value initialization.
2020-06-09 14:03 [PATCH v1 0/3] Support reading and setting default system parameters Alain Michaud
2020-06-09 14:03 ` [PATCH v1 1/3] mgmt: read/set system parameter definitions Alain Michaud
@ 2020-06-09 14:03 ` Alain Michaud
2020-06-09 14:03 ` [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt Alain Michaud
2 siblings, 0 replies; 11+ messages in thread
From: Alain Michaud @ 2020-06-09 14:03 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Alain Michaud, Abhishek Pandit-Subedi
This patch centralized the initialization of default parameters. This
is required to allow clients to more easily customize the default
system parameters.
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Signed-off-by: Alain Michaud <alainm@chromium.org>
---
include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++
net/bluetooth/hci_conn.c | 14 ++++----------
net/bluetooth/hci_core.c | 14 +++++++++++++-
net/bluetooth/hci_request.c | 15 +++++----------
4 files changed, 40 insertions(+), 21 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cdd4f1db8670..0d5dbb6cb5a0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -295,6 +295,14 @@ struct hci_dev {
__u8 le_scan_type;
__u16 le_scan_interval;
__u16 le_scan_window;
+ __u16 le_scan_int_suspend;
+ __u16 le_scan_window_suspend;
+ __u16 le_scan_int_discovery;
+ __u16 le_scan_window_discovery;
+ __u16 le_scan_int_adv_monitor;
+ __u16 le_scan_window_adv_monitor;
+ __u16 le_scan_int_connect;
+ __u16 le_scan_window_connect;
__u16 le_conn_min_interval;
__u16 le_conn_max_interval;
__u16 le_conn_latency;
@@ -323,6 +331,16 @@ struct hci_dev {
__u16 devid_product;
__u16 devid_version;
+ __u8 def_page_scan_type;
+ __u16 def_page_scan_int;
+ __u16 def_page_scan_window;
+ __u8 def_inq_scan_type;
+ __u16 def_inq_scan_int;
+ __u16 def_inq_scan_window;
+ __u16 def_br_lsto;
+ __u16 def_page_timeout;
+ __u16 def_multi_adv_rotation_duration;
+
__u16 pkt_type;
__u16 esco_type;
__u16 link_policy;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 307800fd18e6..9bdffc4e79b0 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -789,11 +789,8 @@ static void set_ext_conn_params(struct hci_conn *conn,
memset(p, 0, sizeof(*p));
- /* Set window to be the same value as the interval to
- * enable continuous scanning.
- */
- p->scan_interval = cpu_to_le16(hdev->le_scan_interval);
- p->scan_window = p->scan_interval;
+ p->scan_interval = cpu_to_le16(hdev->le_scan_int_connect);
+ p->scan_window = cpu_to_le16(hdev->le_scan_window_connect);
p->conn_interval_min = cpu_to_le16(conn->le_conn_min_interval);
p->conn_interval_max = cpu_to_le16(conn->le_conn_max_interval);
p->conn_latency = cpu_to_le16(conn->le_conn_latency);
@@ -875,11 +872,8 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
memset(&cp, 0, sizeof(cp));
- /* Set window to be the same value as the interval to enable
- * continuous scanning.
- */
- cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
- cp.scan_window = cp.scan_interval;
+ cp.scan_interval = cpu_to_le16(hdev->le_scan_int_connect);
+ cp.scan_window = cpu_to_le16(hdev->le_scan_window_connect);
bacpy(&cp.peer_addr, &conn->dst);
cp.peer_addr_type = conn->dst_type;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbe2d79f233f..122e44a82995 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2982,7 +2982,7 @@ int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
adv_instance->remaining_time = timeout;
if (duration == 0)
- adv_instance->duration = HCI_DEFAULT_ADV_DURATION;
+ adv_instance->duration = hdev->def_multi_adv_rotation_duration;
else
adv_instance->duration = duration;
@@ -3397,6 +3397,12 @@ struct hci_dev *hci_alloc_dev(void)
hdev->le_adv_max_interval = 0x0800;
hdev->le_scan_interval = 0x0060;
hdev->le_scan_window = 0x0030;
+ hdev->le_scan_int_suspend = 0x0400;
+ hdev->le_scan_window_suspend = 0x0012;
+ hdev->le_scan_int_discovery = DISCOV_LE_SCAN_INT;
+ hdev->le_scan_window_discovery = DISCOV_LE_SCAN_WIN;
+ hdev->le_scan_int_connect = 0x0060;
+ hdev->le_scan_window_connect = 0x0060;
hdev->le_conn_min_interval = 0x0018;
hdev->le_conn_max_interval = 0x0028;
hdev->le_conn_latency = 0x0000;
@@ -3412,6 +3418,7 @@ struct hci_dev *hci_alloc_dev(void)
hdev->le_tx_def_phys = HCI_LE_SET_PHY_1M;
hdev->le_rx_def_phys = HCI_LE_SET_PHY_1M;
hdev->le_num_of_adv_sets = HCI_MAX_ADV_INSTANCES;
+ hdev->def_multi_adv_rotation_duration = HCI_DEFAULT_ADV_DURATION;
hdev->rpa_timeout = HCI_DEFAULT_RPA_TIMEOUT;
hdev->discov_interleaved_timeout = DISCOV_INTERLEAVED_TIMEOUT;
@@ -3420,6 +3427,11 @@ struct hci_dev *hci_alloc_dev(void)
hdev->auth_payload_timeout = DEFAULT_AUTH_PAYLOAD_TIMEOUT;
hdev->min_enc_key_size = HCI_MIN_ENC_KEY_SIZE;
+ /* default 1.28 sec page scan */
+ hdev->def_page_scan_type = PAGE_SCAN_TYPE_STANDARD;
+ hdev->def_page_scan_int = 0x0800;
+ hdev->def_page_scan_window = 0x0012;
+
mutex_init(&hdev->lock);
mutex_init(&hdev->req_lock);
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 1acf5b8e0910..a7f572ad38ef 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -34,9 +34,6 @@
#define HCI_REQ_PEND 1
#define HCI_REQ_CANCELED 2
-#define LE_SUSPEND_SCAN_WINDOW 0x0012
-#define LE_SUSPEND_SCAN_INTERVAL 0x0400
-
void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
{
skb_queue_head_init(&req->cmd_q);
@@ -366,13 +363,11 @@ void __hci_req_write_fast_connectable(struct hci_request *req, bool enable)
/* 160 msec page scan interval */
acp.interval = cpu_to_le16(0x0100);
} else {
- type = PAGE_SCAN_TYPE_STANDARD; /* default */
-
- /* default 1.28 sec page scan */
- acp.interval = cpu_to_le16(0x0800);
+ type = hdev->def_page_scan_type;
+ acp.interval = cpu_to_le16(hdev->def_page_scan_int);
}
- acp.window = cpu_to_le16(0x0012);
+ acp.window = cpu_to_le16(hdev->def_page_scan_window);
if (__cpu_to_le16(hdev->page_scan_interval) != acp.interval ||
__cpu_to_le16(hdev->page_scan_window) != acp.window)
@@ -927,8 +922,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
filter_policy |= 0x02;
if (hdev->suspended) {
- window = LE_SUSPEND_SCAN_WINDOW;
- interval = LE_SUSPEND_SCAN_INTERVAL;
+ window = hdev->le_scan_window_suspend;
+ interval = hdev->le_scan_int_suspend;
} else {
window = hdev->le_scan_window;
interval = hdev->le_scan_interval;
--
2.27.0.278.ge193c7cf3a9-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt
2020-06-09 14:03 [PATCH v1 0/3] Support reading and setting default system parameters Alain Michaud
2020-06-09 14:03 ` [PATCH v1 1/3] mgmt: read/set system parameter definitions Alain Michaud
2020-06-09 14:03 ` [PATCH v1 2/3] bluetooth:centralize default value initialization Alain Michaud
@ 2020-06-09 14:03 ` Alain Michaud
2020-06-10 14:36 ` Marcel Holtmann
2 siblings, 1 reply; 11+ messages in thread
From: Alain Michaud @ 2020-06-09 14:03 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Alain Michaud, Abhishek Pandit-Subedi
This patch implements the read default system parameters and the set
default system parameters mgmt commands.
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Signed-off-by: Alain Michaud <alainm@chromium.org>
---
net/bluetooth/mgmt.c | 429 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 429 insertions(+)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 9e8a3cccc6ca..98a4193c8e66 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -111,6 +111,8 @@ static const u16 mgmt_commands[] = {
MGMT_OP_READ_SECURITY_INFO,
MGMT_OP_READ_EXP_FEATURES_INFO,
MGMT_OP_SET_EXP_FEATURE,
+ MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
+ MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
};
static const u16 mgmt_events[] = {
@@ -3849,6 +3851,431 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
MGMT_STATUS_NOT_SUPPORTED);
}
+static int read_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 data_len)
+{
+ struct {
+ struct mgmt_system_parameter_tlv entry;
+ union {
+ /* This is a simplification for now since all values
+ * are 16 bits. In the future, this code may need
+ * refactoring to account for variable length values
+ * and properly calculate the required buffer size.
+ */
+ u16 value;
+ };
+ } __packed params[] = {
+ /* Please see mgmt-api.txt for documentation of these values */
+ {
+ { 0x0000, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_page_scan_type) }
+ },
+ {
+ { 0x0001, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_page_scan_int) }
+ },
+ {
+ { 0x0002, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_page_scan_window) }
+ },
+ {
+ { 0x0003, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_inq_scan_type) }
+ },
+ {
+ { 0x0004, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_inq_scan_int) }
+ },
+ {
+ { 0x0005, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_inq_scan_window) }
+ },
+ {
+ { 0x0006, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_br_lsto) }
+ },
+ {
+ { 0x0007, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_page_timeout) }
+ },
+ {
+ { 0x0008, sizeof(__u16) },
+ { cpu_to_le16(hdev->sniff_min_interval) }
+ },
+ {
+ { 0x0009, sizeof(__u16) },
+ { cpu_to_le16(hdev->sniff_max_interval) }
+ },
+ {
+ { 0x000a, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_adv_min_interval) }
+ },
+ {
+ { 0x000b, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_adv_max_interval) }
+ },
+ {
+ { 0x000c, sizeof(__u16) },
+ { cpu_to_le16(hdev->def_multi_adv_rotation_duration) }
+ },
+ {
+ { 0x000d, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_interval) }
+ },
+ {
+ { 0x000e, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_window) }
+ },
+ {
+ { 0x000f, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_int_suspend) }
+ },
+ {
+ { 0x0010, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_window_suspend) }
+ },
+ {
+ { 0x0011, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_int_discovery) }
+ },
+ {
+ { 0x0012, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_window_discovery) }
+ },
+ {
+ { 0x0013, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_int_adv_monitor) }
+ },
+ {
+ { 0x0014, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_window_adv_monitor) }
+ },
+ {
+ { 0x0015, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_int_connect) }
+ },
+ {
+ { 0x0016, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_scan_window_connect) }
+ },
+ {
+ { 0x0017, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_conn_min_interval) }
+ },
+ {
+ { 0x0018, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_conn_max_interval) }
+ },
+ {
+ { 0x0019, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_conn_latency) }
+ },
+ {
+ { 0x001a, sizeof(__u16) },
+ { cpu_to_le16(hdev->le_supv_timeout) }
+ },
+ };
+ struct mgmt_rp_read_default_system_parameters *rp = (void *)params;
+
+ bt_dev_dbg(hdev, "sock %p", sk);
+
+ return mgmt_cmd_complete(sk, hdev->id,
+ MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
+ 0, rp, sizeof(params));
+}
+
+#define TO_TLV(x) ((struct mgmt_system_parameter_tlv *)(x))
+#define TLV_VALUE_CAST(tlv, type) (*((type *)(TO_TLV(tlv)->value)))
+
+static int set_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 data_len)
+{
+ u16 buffer_left = data_len;
+ u8 *buffer = data;
+
+ if (buffer_left < sizeof(struct mgmt_system_parameter_tlv)) {
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
+ MGMT_STATUS_INVALID_PARAMS);
+ }
+
+ /* First pass to validate the tlv */
+ while (buffer_left >= sizeof(struct mgmt_system_parameter_tlv)) {
+ const u8 len = TO_TLV(buffer)->length;
+ const u16 exp_len = sizeof(struct mgmt_system_parameter_tlv) +
+ len;
+ const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
+
+ if (buffer_left < exp_len) {
+ bt_dev_warn(hdev, "invalid len left %d, exp >= %d",
+ buffer_left, exp_len);
+
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
+ MGMT_STATUS_INVALID_PARAMS);
+ }
+
+ /* Please see mgmt-api.txt for documentation of these values */
+ switch (type) {
+ case 0x0000:
+ case 0x0001:
+ case 0x0002:
+ case 0x0003:
+ case 0x0004:
+ case 0x0005:
+ case 0x0006:
+ case 0x0007:
+ case 0x0008:
+ case 0x0009:
+ case 0x000a:
+ case 0x000b:
+ case 0x000c:
+ case 0x000d:
+ case 0x000e:
+ case 0x000f:
+ case 0x0010:
+ case 0x0011:
+ case 0x0012:
+ case 0x0013:
+ case 0x0014:
+ case 0x0015:
+ case 0x0016:
+ case 0x0017:
+ case 0x0018:
+ case 0x0019:
+ case 0x001a:
+ if (len != sizeof(u16)) {
+ bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
+ len, sizeof(u16), type);
+
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
+ MGMT_STATUS_INVALID_PARAMS);
+ }
+ break;
+ default:
+ bt_dev_warn(hdev, "unsupported parameter %u", type);
+ break;
+ }
+
+ buffer_left -= exp_len;
+ buffer += exp_len;
+ }
+
+ buffer_left = data_len;
+ buffer = data;
+ while (buffer_left >= sizeof(struct mgmt_system_parameter_tlv)) {
+ const u8 len = TO_TLV(buffer)->length;
+ const u16 exp_len = sizeof(struct mgmt_system_parameter_tlv) +
+ len;
+ const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
+
+ switch (type) {
+ case 0x0000:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_page_scan_type = le16_to_cpu(val);
+ break;
+ }
+ case 0x0001:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_page_scan_int = le16_to_cpu(val);
+ break;
+ }
+ case 0x0002:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_page_scan_window = le16_to_cpu(val);
+ break;
+ }
+ case 0x0003:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_inq_scan_type = le16_to_cpu(val);
+ break;
+ }
+ case 0x0004:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_inq_scan_int = le16_to_cpu(val);
+ break;
+ }
+ case 0x0005:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_inq_scan_window = le16_to_cpu(val);
+ break;
+ }
+ case 0x0006:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_br_lsto = le16_to_cpu(val);
+ break;
+ }
+ case 0x0007:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_page_timeout = le16_to_cpu(val);
+ break;
+ }
+ case 0x0008:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->sniff_min_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x0009:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->sniff_max_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x000a:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_adv_min_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x000b:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_adv_max_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x000c:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->def_multi_adv_rotation_duration =
+ le16_to_cpu(val);
+ break;
+ }
+ case 0x000d:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x000e:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_window = le16_to_cpu(val);
+ break;
+ }
+ case 0x000f:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_int_suspend = le16_to_cpu(val);
+ break;
+ }
+ case 0x0010:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_window_suspend = le16_to_cpu(val);
+ break;
+ }
+ case 0x0011:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_int_discovery = le16_to_cpu(val);
+ break;
+ }
+ case 0x00012:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_window_discovery = le16_to_cpu(val);
+ break;
+ }
+ case 0x00013:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_int_adv_monitor = le16_to_cpu(val);
+ break;
+ }
+ case 0x00014:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_window_adv_monitor = le16_to_cpu(val);
+ break;
+ }
+ case 0x00015:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_int_connect = le16_to_cpu(val);
+ break;
+ }
+ case 0x00016:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_scan_window_connect = le16_to_cpu(val);
+ break;
+ }
+ case 0x00017:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_conn_min_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x00018:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_conn_max_interval = le16_to_cpu(val);
+ break;
+ }
+ case 0x00019:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_conn_latency = le16_to_cpu(val);
+ break;
+ }
+ case 0x0001a:
+ {
+ const u16 val = TLV_VALUE_CAST(buffer, u16);
+
+ hdev->le_supv_timeout = le16_to_cpu(val);
+ break;
+ }
+ default:
+ bt_dev_warn(hdev, "unsupported parameter %u",
+ buffer[0]);
+ break;
+ }
+
+ buffer_left -= exp_len;
+ buffer += exp_len;
+ }
+
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
+ MGMT_STATUS_SUCCESS);
+}
+
static void read_local_oob_data_complete(struct hci_dev *hdev, u8 status,
u16 opcode, struct sk_buff *skb)
{
@@ -7297,6 +7724,8 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
{ set_exp_feature, MGMT_SET_EXP_FEATURE_SIZE,
HCI_MGMT_VAR_LEN |
HCI_MGMT_HDEV_OPTIONAL },
+ { read_default_system_parameters, 0, HCI_MGMT_UNTRUSTED },
+ { set_default_system_parameters, 0, HCI_MGMT_VAR_LEN },
};
void mgmt_index_added(struct hci_dev *hdev)
--
2.27.0.278.ge193c7cf3a9-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions
2020-06-09 14:03 ` [PATCH v1 1/3] mgmt: read/set system parameter definitions Alain Michaud
@ 2020-06-10 14:16 ` Marcel Holtmann
2020-06-10 15:54 ` Alain Michaud
0 siblings, 1 reply; 11+ messages in thread
From: Marcel Holtmann @ 2020-06-10 14:16 UTC (permalink / raw)
To: Alain Michaud; +Cc: linux-bluetooth, Abhishek Pandit-Subedi, Yu Liu
Hi Alain,
> This patch submits the corresponding kernel definitions to mgmt.h.
> This is submitted before the implementation to avoid any conflicts in
> values allocations.
>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Yu Liu <yudiliu@google.com>
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
>
> include/net/bluetooth/mgmt.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 16e0d87bd8fa..1081e371f03d 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -702,6 +702,24 @@ struct mgmt_rp_set_exp_feature {
> __le32 flags;
> } __packed;
>
> +#define MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS 0x004b
> +
I would go for MGMT_OP_READ_DEF_SYSTEM_CONFIG or MGMT_OP_READ_DEFAULT_SYSTEM_CONFIG to match the name in the mgmt-api.txt more closely.
> +struct mgmt_system_parameter_tlv {
> + __u16 type;
> + __u8 length;
> + __u8 value[];
> +} __packed;
> +
Can we just introduce a generic mgmt_tlv {} struct. I think we could use it more broadly. However I wonder if we need it actually since have the EIR parsing support. Maybe just extend that one.
> +struct mgmt_rp_read_default_system_parameters {
> + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> +} __packed;
> +
> +#define MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS 0x004c
Similar to the comment above.
> +
> +struct mgmt_cp_set_default_system_parameters {
> + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> +} __packed;
> +
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;
If you have a chance, please also add MGMT_OP_{READ,SET}_DEF_RUNTIME_CONFIG as well. If not, then I am going to send out a patch for that by myself.
Regards
Marcel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt
2020-06-09 14:03 ` [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt Alain Michaud
@ 2020-06-10 14:36 ` Marcel Holtmann
2020-06-10 16:06 ` Alain Michaud
0 siblings, 1 reply; 11+ messages in thread
From: Marcel Holtmann @ 2020-06-10 14:36 UTC (permalink / raw)
To: Alain Michaud; +Cc: linux-bluetooth, Abhishek Pandit-Subedi
Hi Alain,
> This patch implements the read default system parameters and the set
> default system parameters mgmt commands.
>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
>
> net/bluetooth/mgmt.c | 429 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 429 insertions(+)
I have the feeling that we want to create a net/bluetooth/mgmt_config.c to move all the configuration parameters parsing and handling into a separate. I have the feeling that otherwise mgmt.c will grow quickly.
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 9e8a3cccc6ca..98a4193c8e66 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -111,6 +111,8 @@ static const u16 mgmt_commands[] = {
> MGMT_OP_READ_SECURITY_INFO,
> MGMT_OP_READ_EXP_FEATURES_INFO,
> MGMT_OP_SET_EXP_FEATURE,
> + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> };
Please also add the OP_READ_ command to mgmt_untrusted_commands array.
> static const u16 mgmt_events[] = {
> @@ -3849,6 +3851,431 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> MGMT_STATUS_NOT_SUPPORTED);
> }
>
> +static int read_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 data_len)
> +{
> + struct {
> + struct mgmt_system_parameter_tlv entry;
> + union {
> + /* This is a simplification for now since all values
> + * are 16 bits. In the future, this code may need
> + * refactoring to account for variable length values
> + * and properly calculate the required buffer size.
> + */
> + u16 value;
> + };
> + } __packed params[] = {
> + /* Please see mgmt-api.txt for documentation of these values */
> + {
> + { 0x0000, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_page_scan_type) }
> + },
> + {
> + { 0x0001, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_page_scan_int) }
> + },
> + {
> + { 0x0002, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_page_scan_window) }
> + },
> + {
> + { 0x0003, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_inq_scan_type) }
> + },
> + {
> + { 0x0004, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_inq_scan_int) }
> + },
> + {
> + { 0x0005, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_inq_scan_window) }
> + },
> + {
> + { 0x0006, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_br_lsto) }
> + },
> + {
> + { 0x0007, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_page_timeout) }
> + },
> + {
> + { 0x0008, sizeof(__u16) },
> + { cpu_to_le16(hdev->sniff_min_interval) }
> + },
> + {
> + { 0x0009, sizeof(__u16) },
> + { cpu_to_le16(hdev->sniff_max_interval) }
> + },
> + {
> + { 0x000a, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_adv_min_interval) }
> + },
> + {
> + { 0x000b, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_adv_max_interval) }
> + },
> + {
> + { 0x000c, sizeof(__u16) },
> + { cpu_to_le16(hdev->def_multi_adv_rotation_duration) }
> + },
> + {
> + { 0x000d, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_interval) }
> + },
> + {
> + { 0x000e, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_window) }
> + },
> + {
> + { 0x000f, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_int_suspend) }
> + },
> + {
> + { 0x0010, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_window_suspend) }
> + },
> + {
> + { 0x0011, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_int_discovery) }
> + },
> + {
> + { 0x0012, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_window_discovery) }
> + },
> + {
> + { 0x0013, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_int_adv_monitor) }
> + },
> + {
> + { 0x0014, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_window_adv_monitor) }
> + },
> + {
> + { 0x0015, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_int_connect) }
> + },
> + {
> + { 0x0016, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_scan_window_connect) }
> + },
> + {
> + { 0x0017, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_conn_min_interval) }
> + },
> + {
> + { 0x0018, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_conn_max_interval) }
> + },
> + {
> + { 0x0019, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_conn_latency) }
> + },
> + {
> + { 0x001a, sizeof(__u16) },
> + { cpu_to_le16(hdev->le_supv_timeout) }
> + },
This is creative and I like it. However I would take it one step further. Define a macro similar USB_DEVICE etc. so you get a simpler table.
{ HDEV_PARAM(0x0018, le_conn_max_interval) },
{ HDEV_PARAM(0x0019, le_conn_latency) },
{ }
> + };
> + struct mgmt_rp_read_default_system_parameters *rp = (void *)params;
> +
> + bt_dev_dbg(hdev, "sock %p", sk);
> +
> + return mgmt_cmd_complete(sk, hdev->id,
> + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
> + 0, rp, sizeof(params));
Please introduce HCI_MGMT_SYSTEM_CONFIG_EVENTS and send them out as described in mgmt-api.txt
> +}
> +
> +#define TO_TLV(x) ((struct mgmt_system_parameter_tlv *)(x))
> +#define TLV_VALUE_CAST(tlv, type) (*((type *)(TO_TLV(tlv)->value)))
> +
> +static int set_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 data_len)
> +{
> + u16 buffer_left = data_len;
> + u8 *buffer = data;
> +
> + if (buffer_left < sizeof(struct mgmt_system_parameter_tlv)) {
> + return mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> + MGMT_STATUS_INVALID_PARAMS);
> + }
> +
> + /* First pass to validate the tlv */
> + while (buffer_left >= sizeof(struct mgmt_system_parameter_tlv)) {
> + const u8 len = TO_TLV(buffer)->length;
> + const u16 exp_len = sizeof(struct mgmt_system_parameter_tlv) +
> + len;
> + const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
> +
> + if (buffer_left < exp_len) {
> + bt_dev_warn(hdev, "invalid len left %d, exp >= %d",
> + buffer_left, exp_len);
> +
> + return mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> + MGMT_STATUS_INVALID_PARAMS);
> + }
> +
> + /* Please see mgmt-api.txt for documentation of these values */
> + switch (type) {
> + case 0x0000:
> + case 0x0001:
> + case 0x0002:
> + case 0x0003:
> + case 0x0004:
> + case 0x0005:
> + case 0x0006:
> + case 0x0007:
> + case 0x0008:
> + case 0x0009:
> + case 0x000a:
> + case 0x000b:
> + case 0x000c:
> + case 0x000d:
> + case 0x000e:
> + case 0x000f:
> + case 0x0010:
> + case 0x0011:
> + case 0x0012:
> + case 0x0013:
> + case 0x0014:
> + case 0x0015:
> + case 0x0016:
> + case 0x0017:
> + case 0x0018:
> + case 0x0019:
> + case 0x001a:
> + if (len != sizeof(u16)) {
> + bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
> + len, sizeof(u16), type);
> +
> + return mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> + MGMT_STATUS_INVALID_PARAMS);
> + }
> + break;
> + default:
> + bt_dev_warn(hdev, "unsupported parameter %u", type);
> + break;
> + }
> +
> + buffer_left -= exp_len;
> + buffer += exp_len;
> + }
> +
> + buffer_left = data_len;
> + buffer = data;
> + while (buffer_left >= sizeof(struct mgmt_system_parameter_tlv)) {
> + const u8 len = TO_TLV(buffer)->length;
> + const u16 exp_len = sizeof(struct mgmt_system_parameter_tlv) +
> + len;
> + const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
> +
> + switch (type) {
> + case 0x0000:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_page_scan_type = le16_to_cpu(val);
I would really turn this into a single macro that also does the byte_swapping and more importantly also the unaligned access. Mainly because I highly doubt the TLV_VALUE_CAST will actually work on ARM platforms. So I am thinking something along the lines of this:
hdev->def_page_scan_type = TLV_GET_LE16(buffer);
> + break;
> + }
> + case 0x0001:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_page_scan_int = le16_to_cpu(val);
> + break;
> + }
> + case 0x0002:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_page_scan_window = le16_to_cpu(val);
> + break;
> + }
> + case 0x0003:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_inq_scan_type = le16_to_cpu(val);
> + break;
> + }
> + case 0x0004:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_inq_scan_int = le16_to_cpu(val);
> + break;
> + }
> + case 0x0005:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_inq_scan_window = le16_to_cpu(val);
> + break;
> + }
> + case 0x0006:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_br_lsto = le16_to_cpu(val);
> + break;
> + }
> + case 0x0007:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_page_timeout = le16_to_cpu(val);
> + break;
> + }
> + case 0x0008:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->sniff_min_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x0009:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->sniff_max_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x000a:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_adv_min_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x000b:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_adv_max_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x000c:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->def_multi_adv_rotation_duration =
> + le16_to_cpu(val);
> + break;
> + }
> + case 0x000d:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x000e:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_window = le16_to_cpu(val);
> + break;
> + }
> + case 0x000f:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_int_suspend = le16_to_cpu(val);
> + break;
> + }
> + case 0x0010:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_window_suspend = le16_to_cpu(val);
> + break;
> + }
> + case 0x0011:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_int_discovery = le16_to_cpu(val);
> + break;
> + }
> + case 0x00012:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_window_discovery = le16_to_cpu(val);
> + break;
> + }
> + case 0x00013:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_int_adv_monitor = le16_to_cpu(val);
> + break;
> + }
> + case 0x00014:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_window_adv_monitor = le16_to_cpu(val);
> + break;
> + }
> + case 0x00015:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_int_connect = le16_to_cpu(val);
> + break;
> + }
> + case 0x00016:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_scan_window_connect = le16_to_cpu(val);
> + break;
> + }
> + case 0x00017:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_conn_min_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x00018:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_conn_max_interval = le16_to_cpu(val);
> + break;
> + }
> + case 0x00019:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_conn_latency = le16_to_cpu(val);
> + break;
> + }
> + case 0x0001a:
> + {
> + const u16 val = TLV_VALUE_CAST(buffer, u16);
> +
> + hdev->le_supv_timeout = le16_to_cpu(val);
> + break;
> + }
> + default:
> + bt_dev_warn(hdev, "unsupported parameter %u",
> + buffer[0]);
> + break;
> + }
> +
> + buffer_left -= exp_len;
> + buffer += exp_len;
> + }
> +
> + return mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> + MGMT_STATUS_SUCCESS);
> +}
> +
> static void read_local_oob_data_complete(struct hci_dev *hdev, u8 status,
> u16 opcode, struct sk_buff *skb)
> {
> @@ -7297,6 +7724,8 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
> { set_exp_feature, MGMT_SET_EXP_FEATURE_SIZE,
> HCI_MGMT_VAR_LEN |
> HCI_MGMT_HDEV_OPTIONAL },
> + { read_default_system_parameters, 0, HCI_MGMT_UNTRUSTED },
> + { set_default_system_parameters, 0, HCI_MGMT_VAR_LEN },
> };
>
> void mgmt_index_added(struct hci_dev *hdev)
Regards
Marcel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions
2020-06-10 14:16 ` Marcel Holtmann
@ 2020-06-10 15:54 ` Alain Michaud
2020-06-10 16:02 ` Marcel Holtmann
0 siblings, 1 reply; 11+ messages in thread
From: Alain Michaud @ 2020-06-10 15:54 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Alain Michaud, BlueZ, Abhishek Pandit-Subedi, Yu Liu
Hi Marcel,
Since this has already been committed in user space, could we agree to
keep it as is? The alternative is that we'd need to re-patch all the
userspace implementation through a seperate patch. Up to you.
I won't have time to implement the runtime config ones in the next few
weeks, feel free to post it separately, or I can get to it in July.
Thanks,
Alain
On Wed, Jun 10, 2020 at 10:16 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > This patch submits the corresponding kernel definitions to mgmt.h.
> > This is submitted before the implementation to avoid any conflicts in
> > values allocations.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Reviewed-by: Yu Liu <yudiliu@google.com>
> >
> > Signed-off-by: Alain Michaud <alainm@chromium.org>
> > ---
> >
> > include/net/bluetooth/mgmt.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > index 16e0d87bd8fa..1081e371f03d 100644
> > --- a/include/net/bluetooth/mgmt.h
> > +++ b/include/net/bluetooth/mgmt.h
> > @@ -702,6 +702,24 @@ struct mgmt_rp_set_exp_feature {
> > __le32 flags;
> > } __packed;
> >
> > +#define MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS 0x004b
> > +
>
> I would go for MGMT_OP_READ_DEF_SYSTEM_CONFIG or MGMT_OP_READ_DEFAULT_SYSTEM_CONFIG to match the name in the mgmt-api.txt more closely.
>
> > +struct mgmt_system_parameter_tlv {
> > + __u16 type;
> > + __u8 length;
> > + __u8 value[];
> > +} __packed;
> > +
>
> Can we just introduce a generic mgmt_tlv {} struct. I think we could use it more broadly. However I wonder if we need it actually since have the EIR parsing support. Maybe just extend that one.
>
> > +struct mgmt_rp_read_default_system_parameters {
> > + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> > +} __packed;
> > +
> > +#define MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS 0x004c
>
> Similar to the comment above.
>
> > +
> > +struct mgmt_cp_set_default_system_parameters {
> > + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> > +} __packed;
> > +
> > #define MGMT_EV_CMD_COMPLETE 0x0001
> > struct mgmt_ev_cmd_complete {
> > __le16 opcode;
>
> If you have a chance, please also add MGMT_OP_{READ,SET}_DEF_RUNTIME_CONFIG as well. If not, then I am going to send out a patch for that by myself.
>
> Regards
>
> Marcel
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions
2020-06-10 15:54 ` Alain Michaud
@ 2020-06-10 16:02 ` Marcel Holtmann
2020-06-10 16:08 ` Alain Michaud
0 siblings, 1 reply; 11+ messages in thread
From: Marcel Holtmann @ 2020-06-10 16:02 UTC (permalink / raw)
To: Alain Michaud; +Cc: Alain Michaud, BlueZ, Abhishek Pandit-Subedi, Yu Liu
Hi Alain,
> Since this has already been committed in user space, could we agree to
> keep it as is? The alternative is that we'd need to re-patch all the
> userspace implementation through a seperate patch. Up to you.
we can fix this up easily. And I can do that if needed.
> I won't have time to implement the runtime config ones in the next few
> weeks, feel free to post it separately, or I can get to it in July.
That is fine, then I will add it for you. It would be just great if you can review it though.
Regards
Marcel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt
2020-06-10 14:36 ` Marcel Holtmann
@ 2020-06-10 16:06 ` Alain Michaud
2020-06-10 16:58 ` Marcel Holtmann
0 siblings, 1 reply; 11+ messages in thread
From: Alain Michaud @ 2020-06-10 16:06 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Alain Michaud, BlueZ, Abhishek Pandit-Subedi
Hi Marcel,
On Wed, Jun 10, 2020 at 10:36 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > This patch implements the read default system parameters and the set
> > default system parameters mgmt commands.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Signed-off-by: Alain Michaud <alainm@chromium.org>
> > ---
> >
> > net/bluetooth/mgmt.c | 429 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 429 insertions(+)
>
> I have the feeling that we want to create a net/bluetooth/mgmt_config.c to move all the configuration parameters parsing and handling into a separate. I have the feeling that otherwise mgmt.c will grow quickly.
ok, I'll send out a v2 that split it out in mgmt_config.c/h
>
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 9e8a3cccc6ca..98a4193c8e66 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -111,6 +111,8 @@ static const u16 mgmt_commands[] = {
> > MGMT_OP_READ_SECURITY_INFO,
> > MGMT_OP_READ_EXP_FEATURES_INFO,
> > MGMT_OP_SET_EXP_FEATURE,
> > + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
> > + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> > };
>
> Please also add the OP_READ_ command to mgmt_untrusted_commands array.
ACK.
>
> > static const u16 mgmt_events[] = {
> > @@ -3849,6 +3851,431 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > MGMT_STATUS_NOT_SUPPORTED);
> > }
> >
> > +static int read_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
> > + void *data, u16 data_len)
> > +{
> > + struct {
> > + struct mgmt_system_parameter_tlv entry;
> > + union {
> > + /* This is a simplification for now since all values
> > + * are 16 bits. In the future, this code may need
> > + * refactoring to account for variable length values
> > + * and properly calculate the required buffer size.
> > + */
> > + u16 value;
> > + };
> > + } __packed params[] = {
> > + /* Please see mgmt-api.txt for documentation of these values */
> > + {
> > + { 0x0000, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_page_scan_type) }
> > + },
> > + {
> > + { 0x0001, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_page_scan_int) }
> > + },
> > + {
> > + { 0x0002, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_page_scan_window) }
> > + },
> > + {
> > + { 0x0003, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_inq_scan_type) }
> > + },
> > + {
> > + { 0x0004, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_inq_scan_int) }
> > + },
> > + {
> > + { 0x0005, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_inq_scan_window) }
> > + },
> > + {
> > + { 0x0006, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_br_lsto) }
> > + },
> > + {
> > + { 0x0007, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_page_timeout) }
> > + },
> > + {
> > + { 0x0008, sizeof(__u16) },
> > + { cpu_to_le16(hdev->sniff_min_interval) }
> > + },
> > + {
> > + { 0x0009, sizeof(__u16) },
> > + { cpu_to_le16(hdev->sniff_max_interval) }
> > + },
> > + {
> > + { 0x000a, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_adv_min_interval) }
> > + },
> > + {
> > + { 0x000b, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_adv_max_interval) }
> > + },
> > + {
> > + { 0x000c, sizeof(__u16) },
> > + { cpu_to_le16(hdev->def_multi_adv_rotation_duration) }
> > + },
> > + {
> > + { 0x000d, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_interval) }
> > + },
> > + {
> > + { 0x000e, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_window) }
> > + },
> > + {
> > + { 0x000f, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_int_suspend) }
> > + },
> > + {
> > + { 0x0010, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_window_suspend) }
> > + },
> > + {
> > + { 0x0011, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_int_discovery) }
> > + },
> > + {
> > + { 0x0012, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_window_discovery) }
> > + },
> > + {
> > + { 0x0013, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_int_adv_monitor) }
> > + },
> > + {
> > + { 0x0014, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_window_adv_monitor) }
> > + },
> > + {
> > + { 0x0015, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_int_connect) }
> > + },
> > + {
> > + { 0x0016, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_scan_window_connect) }
> > + },
> > + {
> > + { 0x0017, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_conn_min_interval) }
> > + },
> > + {
> > + { 0x0018, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_conn_max_interval) }
> > + },
> > + {
> > + { 0x0019, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_conn_latency) }
> > + },
> > + {
> > + { 0x001a, sizeof(__u16) },
> > + { cpu_to_le16(hdev->le_supv_timeout) }
> > + },
>
> This is creative and I like it. However I would take it one step further. Define a macro similar USB_DEVICE etc. so you get a simpler table.
>
> { HDEV_PARAM(0x0018, le_conn_max_interval) },
> { HDEV_PARAM(0x0019, le_conn_latency) },
> { }
>
>
Ok, that makes sense, I'll think of something for v2.
> > + };
> > + struct mgmt_rp_read_default_system_parameters *rp = (void *)params;
> > +
> > + bt_dev_dbg(hdev, "sock %p", sk);
> > +
> > + return mgmt_cmd_complete(sk, hdev->id,
> > + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
> > + 0, rp, sizeof(params));
>
> Please introduce HCI_MGMT_SYSTEM_CONFIG_EVENTS and send them out as described in mgmt-api.txt
Looks like you added this after my original proposal to mgmt-api.txt.
I'd like to propose this is also implemented in a separate patch.
>
> > +}
> > +
> > +#define TO_TLV(x) ((struct mgmt_system_parameter_tlv *)(x))
> > +#define TLV_VALUE_CAST(tlv, type) (*((type *)(TO_TLV(tlv)->value)))
> > +
> > +static int set_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
> > + void *data, u16 data_len)
> > +{
> > + u16 buffer_left = data_len;
> > + u8 *buffer = data;
> > +
> > + if (buffer_left < sizeof(struct mgmt_system_parameter_tlv)) {
> > + return mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> > + MGMT_STATUS_INVALID_PARAMS);
> > + }
> > +
> > + /* First pass to validate the tlv */
> > + while (buffer_left >= sizeof(struct mgmt_system_parameter_tlv)) {
> > + const u8 len = TO_TLV(buffer)->length;
> > + const u16 exp_len = sizeof(struct mgmt_system_parameter_tlv) +
> > + len;
> > + const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
> > +
> > + if (buffer_left < exp_len) {
> > + bt_dev_warn(hdev, "invalid len left %d, exp >= %d",
> > + buffer_left, exp_len);
> > +
> > + return mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> > + MGMT_STATUS_INVALID_PARAMS);
> > + }
> > +
> > + /* Please see mgmt-api.txt for documentation of these values */
> > + switch (type) {
> > + case 0x0000:
> > + case 0x0001:
> > + case 0x0002:
> > + case 0x0003:
> > + case 0x0004:
> > + case 0x0005:
> > + case 0x0006:
> > + case 0x0007:
> > + case 0x0008:
> > + case 0x0009:
> > + case 0x000a:
> > + case 0x000b:
> > + case 0x000c:
> > + case 0x000d:
> > + case 0x000e:
> > + case 0x000f:
> > + case 0x0010:
> > + case 0x0011:
> > + case 0x0012:
> > + case 0x0013:
> > + case 0x0014:
> > + case 0x0015:
> > + case 0x0016:
> > + case 0x0017:
> > + case 0x0018:
> > + case 0x0019:
> > + case 0x001a:
> > + if (len != sizeof(u16)) {
> > + bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
> > + len, sizeof(u16), type);
> > +
> > + return mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> > + MGMT_STATUS_INVALID_PARAMS);
> > + }
> > + break;
> > + default:
> > + bt_dev_warn(hdev, "unsupported parameter %u", type);
> > + break;
> > + }
> > +
> > + buffer_left -= exp_len;
> > + buffer += exp_len;
> > + }
> > +
> > + buffer_left = data_len;
> > + buffer = data;
> > + while (buffer_left >= sizeof(struct mgmt_system_parameter_tlv)) {
> > + const u8 len = TO_TLV(buffer)->length;
> > + const u16 exp_len = sizeof(struct mgmt_system_parameter_tlv) +
> > + len;
> > + const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
> > +
> > + switch (type) {
> > + case 0x0000:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_page_scan_type = le16_to_cpu(val);
>
> I would really turn this into a single macro that also does the byte_swapping and more importantly also the unaligned access. Mainly because I highly doubt the TLV_VALUE_CAST will actually work on ARM platforms. So I am thinking something along the lines of this:
>
> hdev->def_page_scan_type = TLV_GET_LE16(buffer);
>
ok.
> > + break;
> > + }
> > + case 0x0001:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_page_scan_int = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0002:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_page_scan_window = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0003:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_inq_scan_type = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0004:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_inq_scan_int = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0005:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_inq_scan_window = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0006:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_br_lsto = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0007:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_page_timeout = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0008:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->sniff_min_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0009:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->sniff_max_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x000a:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_adv_min_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x000b:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_adv_max_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x000c:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->def_multi_adv_rotation_duration =
> > + le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x000d:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x000e:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_window = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x000f:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_int_suspend = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0010:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_window_suspend = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0011:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_int_discovery = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00012:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_window_discovery = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00013:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_int_adv_monitor = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00014:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_window_adv_monitor = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00015:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_int_connect = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00016:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_scan_window_connect = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00017:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_conn_min_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00018:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_conn_max_interval = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x00019:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_conn_latency = le16_to_cpu(val);
> > + break;
> > + }
> > + case 0x0001a:
> > + {
> > + const u16 val = TLV_VALUE_CAST(buffer, u16);
> > +
> > + hdev->le_supv_timeout = le16_to_cpu(val);
> > + break;
> > + }
> > + default:
> > + bt_dev_warn(hdev, "unsupported parameter %u",
> > + buffer[0]);
> > + break;
> > + }
> > +
> > + buffer_left -= exp_len;
> > + buffer += exp_len;
> > + }
> > +
> > + return mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
> > + MGMT_STATUS_SUCCESS);
> > +}
> > +
> > static void read_local_oob_data_complete(struct hci_dev *hdev, u8 status,
> > u16 opcode, struct sk_buff *skb)
> > {
> > @@ -7297,6 +7724,8 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
> > { set_exp_feature, MGMT_SET_EXP_FEATURE_SIZE,
> > HCI_MGMT_VAR_LEN |
> > HCI_MGMT_HDEV_OPTIONAL },
> > + { read_default_system_parameters, 0, HCI_MGMT_UNTRUSTED },
> > + { set_default_system_parameters, 0, HCI_MGMT_VAR_LEN },
> > };
> >
> > void mgmt_index_added(struct hci_dev *hdev)
>
> Regards
>
> Marcel
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions
2020-06-10 16:02 ` Marcel Holtmann
@ 2020-06-10 16:08 ` Alain Michaud
0 siblings, 0 replies; 11+ messages in thread
From: Alain Michaud @ 2020-06-10 16:08 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Alain Michaud, BlueZ, Abhishek Pandit-Subedi, Yu Liu
Hi Marcel,
I'll be happy to review :)
Thanks,
Alain
On Wed, Jun 10, 2020 at 12:02 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > Since this has already been committed in user space, could we agree to
> > keep it as is? The alternative is that we'd need to re-patch all the
> > userspace implementation through a seperate patch. Up to you.
>
> we can fix this up easily. And I can do that if needed.
>
> > I won't have time to implement the runtime config ones in the next few
> > weeks, feel free to post it separately, or I can get to it in July.
>
> That is fine, then I will add it for you. It would be just great if you can review it though.
>
> Regards
>
> Marcel
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt
2020-06-10 16:06 ` Alain Michaud
@ 2020-06-10 16:58 ` Marcel Holtmann
0 siblings, 0 replies; 11+ messages in thread
From: Marcel Holtmann @ 2020-06-10 16:58 UTC (permalink / raw)
To: Alain Michaud; +Cc: Alain Michaud, BlueZ, Abhishek Pandit-Subedi
Hi Alain,
>>> This patch implements the read default system parameters and the set
>>> default system parameters mgmt commands.
>>>
>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> Signed-off-by: Alain Michaud <alainm@chromium.org>
>>> ---
>>>
>>> net/bluetooth/mgmt.c | 429 +++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 429 insertions(+)
>>
>> I have the feeling that we want to create a net/bluetooth/mgmt_config.c to move all the configuration parameters parsing and handling into a separate. I have the feeling that otherwise mgmt.c will grow quickly.
> ok, I'll send out a v2 that split it out in mgmt_config.c/h
>
>>
>>>
>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> index 9e8a3cccc6ca..98a4193c8e66 100644
>>> --- a/net/bluetooth/mgmt.c
>>> +++ b/net/bluetooth/mgmt.c
>>> @@ -111,6 +111,8 @@ static const u16 mgmt_commands[] = {
>>> MGMT_OP_READ_SECURITY_INFO,
>>> MGMT_OP_READ_EXP_FEATURES_INFO,
>>> MGMT_OP_SET_EXP_FEATURE,
>>> + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
>>> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
>>> };
>>
>> Please also add the OP_READ_ command to mgmt_untrusted_commands array.
> ACK.
>>
>>> static const u16 mgmt_events[] = {
>>> @@ -3849,6 +3851,431 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
>>> MGMT_STATUS_NOT_SUPPORTED);
>>> }
>>>
>>> +static int read_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
>>> + void *data, u16 data_len)
>>> +{
>>> + struct {
>>> + struct mgmt_system_parameter_tlv entry;
>>> + union {
>>> + /* This is a simplification for now since all values
>>> + * are 16 bits. In the future, this code may need
>>> + * refactoring to account for variable length values
>>> + * and properly calculate the required buffer size.
>>> + */
>>> + u16 value;
>>> + };
>>> + } __packed params[] = {
>>> + /* Please see mgmt-api.txt for documentation of these values */
>>> + {
>>> + { 0x0000, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_scan_type) }
>>> + },
>>> + {
>>> + { 0x0001, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_scan_int) }
>>> + },
>>> + {
>>> + { 0x0002, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_scan_window) }
>>> + },
>>> + {
>>> + { 0x0003, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_inq_scan_type) }
>>> + },
>>> + {
>>> + { 0x0004, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_inq_scan_int) }
>>> + },
>>> + {
>>> + { 0x0005, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_inq_scan_window) }
>>> + },
>>> + {
>>> + { 0x0006, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_br_lsto) }
>>> + },
>>> + {
>>> + { 0x0007, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_timeout) }
>>> + },
>>> + {
>>> + { 0x0008, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->sniff_min_interval) }
>>> + },
>>> + {
>>> + { 0x0009, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->sniff_max_interval) }
>>> + },
>>> + {
>>> + { 0x000a, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_adv_min_interval) }
>>> + },
>>> + {
>>> + { 0x000b, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_adv_max_interval) }
>>> + },
>>> + {
>>> + { 0x000c, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_multi_adv_rotation_duration) }
>>> + },
>>> + {
>>> + { 0x000d, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_interval) }
>>> + },
>>> + {
>>> + { 0x000e, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window) }
>>> + },
>>> + {
>>> + { 0x000f, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_suspend) }
>>> + },
>>> + {
>>> + { 0x0010, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_suspend) }
>>> + },
>>> + {
>>> + { 0x0011, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_discovery) }
>>> + },
>>> + {
>>> + { 0x0012, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_discovery) }
>>> + },
>>> + {
>>> + { 0x0013, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_adv_monitor) }
>>> + },
>>> + {
>>> + { 0x0014, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_adv_monitor) }
>>> + },
>>> + {
>>> + { 0x0015, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_connect) }
>>> + },
>>> + {
>>> + { 0x0016, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_connect) }
>>> + },
>>> + {
>>> + { 0x0017, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_conn_min_interval) }
>>> + },
>>> + {
>>> + { 0x0018, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_conn_max_interval) }
>>> + },
>>> + {
>>> + { 0x0019, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_conn_latency) }
>>> + },
>>> + {
>>> + { 0x001a, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_supv_timeout) }
>>> + },
>>
>> This is creative and I like it. However I would take it one step further. Define a macro similar USB_DEVICE etc. so you get a simpler table.
>>
>> { HDEV_PARAM(0x0018, le_conn_max_interval) },
>> { HDEV_PARAM(0x0019, le_conn_latency) },
>> { }
>>
>>
> Ok, that makes sense, I'll think of something for v2.
>
>>> + };
>>> + struct mgmt_rp_read_default_system_parameters *rp = (void *)params;
>>> +
>>> + bt_dev_dbg(hdev, "sock %p", sk);
>>> +
>>> + return mgmt_cmd_complete(sk, hdev->id,
>>> + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
>>> + 0, rp, sizeof(params));
>>
>> Please introduce HCI_MGMT_SYSTEM_CONFIG_EVENTS and send them out as described in mgmt-api.txt
> Looks like you added this after my original proposal to mgmt-api.txt.
> I'd like to propose this is also implemented in a separate patch.
feel free to add it as a separate patch. See the Experimental Feature commands + event on how this could be done.
Regards
Marcel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-10 16:59 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09 14:03 [PATCH v1 0/3] Support reading and setting default system parameters Alain Michaud
2020-06-09 14:03 ` [PATCH v1 1/3] mgmt: read/set system parameter definitions Alain Michaud
2020-06-10 14:16 ` Marcel Holtmann
2020-06-10 15:54 ` Alain Michaud
2020-06-10 16:02 ` Marcel Holtmann
2020-06-10 16:08 ` Alain Michaud
2020-06-09 14:03 ` [PATCH v1 2/3] bluetooth:centralize default value initialization Alain Michaud
2020-06-09 14:03 ` [PATCH v1 3/3] bluetooth: implement read/set default system parameters mgmt Alain Michaud
2020-06-10 14:36 ` Marcel Holtmann
2020-06-10 16:06 ` Alain Michaud
2020-06-10 16:58 ` Marcel Holtmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).