diff mbox series

[v1,1/5] Bluetooth: Add helper for serialized HCI command execution

Message ID 20210601220349.375619-1-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v1,1/5] Bluetooth: Add helper for serialized HCI command execution | expand

Commit Message

Luiz Augusto von Dentz June 1, 2021, 10:03 p.m. UTC 
From: Marcel Holtmann <marcel@holtmann.org>

The usage of __hci_cmd_sync() within the hdev->setup() callback allows for
a nice and simple serialized execution of HCI commands. More importantly
it allows for result processing before issueing the next command.

With the current usage of hci_req_run() it is possible to batch up
commands and execute them, but it is impossible to react to their
results or errors.

This is an attempt to generalize the hdev->setup() handling and provide
a simple way of running multiple HCI commands from a single function
context.

There are multiple struct work that are decdicated to certain tasks
already used right now. It is add a lot of bloat to hci_dev struct and
extra handling code. So it might be possible to put all of these behind
a common HCI command infrastructure and just execute the HCI commands
from the same work context in a serialized fashion.

For example updating the white list and resolving list can be done now
without having to know the list size ahead of time. Also preparing for
suspend or resume shouldn't require a state machine anymore. There are
other tasks that should be simplified as well.

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h | 17 +++++++
 net/bluetooth/hci_core.c         | 82 ++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)

Comments

bluez.test.bot@gmail.com June 1, 2021, 11:48 p.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=492079

---Test result---

Test Summary:
CheckPatch                    FAIL      6.94 seconds
GitLint                       PASS      0.51 seconds
BuildKernel                   PASS      506.87 seconds
TestRunner: Setup             PASS      330.53 seconds
TestRunner: l2cap-tester      PASS      2.51 seconds
TestRunner: bnep-tester       PASS      1.86 seconds
TestRunner: mgmt-tester       FAIL      31.96 seconds
TestRunner: rfcomm-tester     PASS      2.02 seconds
TestRunner: sco-tester        PASS      1.97 seconds
TestRunner: smp-tester        PASS      2.05 seconds
TestRunner: userchan-tester   PASS      1.89 seconds

Details
##############################
Test: CheckPatch - FAIL - 6.94 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: eir: Move EIR/Adv Data functions to its own file
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#82: 
new file mode 100644

CHECK: braces {} should be used on all arms of this statement
#261: FILE: net/bluetooth/eir.c:175:
+		if (name_len > 48) {
[...]
+		} else
[...]

CHECK: Unbalanced braces around else statement
#264: FILE: net/bluetooth/eir.c:178:
+		} else

CHECK: No space is necessary after a cast
#278: FILE: net/bluetooth/eir.c:192:
+		ptr[2] = (u8) hdev->inq_tx_power;

total: 0 errors, 1 warnings, 3 checks, 1068 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: eir: Move EIR/Adv Data functions to its own file" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 1
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17: 
Set Device Class - Success 1                         Passed       0.024 seconds

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#215: 
new file mode 100644

CHECK: Alignment should match open parenthesis
#254: FILE: net/bluetooth/hci_sync.c:35:
+	err = wait_event_interruptible_timeout(hdev->req_wait_q,
+			hdev->req_status != HCI_REQ_PEND, timeout);

CHECK: multiple assignments should be avoided
#273: FILE: net/bluetooth/hci_sync.c:54:
+	hdev->req_status = hdev->req_result = 0;

CHECK: Prefer kernel type 'u8' over 'uint8_t'
#302: FILE: net/bluetooth/hci_sync.c:83:
+	uint8_t status;

total: 0 errors, 2 warnings, 3 checks, 652 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 1" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 2
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17: 
Add Advertising - Failure: LE off                    Passed       0.175 seconds

CHECK: Alignment should match open parenthesis
#138: FILE: net/bluetooth/hci_sync.c:82:
+u8 __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
 			  const void *param, u32 timeout)

CHECK: Alignment should match open parenthesis
#440: FILE: net/bluetooth/hci_sync.c:454:
+	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS,
+				       sizeof(cp), &cp, HCI_CMD_TIMEOUT);

CHECK: No space is necessary after a cast
#534: FILE: net/bluetooth/hci_sync.c:548:
+	cp = (void *) data;

CHECK: No space is necessary after a cast
#535: FILE: net/bluetooth/hci_sync.c:549:
+	set = (void *) cp->data;

CHECK: Alignment should match open parenthesis
#808: FILE: net/bluetooth/hci_sync.c:822:
+		queue_delayed_work(hdev->req_workqueue,
+			   &hdev->adv_instance_expire,

total: 0 errors, 1 warnings, 5 checks, 1404 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 2" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 3
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17: 
Add Device - Invalid Params 1                        Passed       0.016 seconds

CHECK: Alignment should match open parenthesis
#233: FILE: net/bluetooth/hci_sync.c:1079:
+	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_DEL_FROM_WHITE_LIST,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);

CHECK: Alignment should match open parenthesis
#307: FILE: net/bluetooth/hci_sync.c:1153:
+	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_ADD_TO_WHITE_LIST,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);

CHECK: Alignment should match open parenthesis
#475: FILE: net/bluetooth/hci_sync.c:1321:
+static int hci_le_set_scan_param_sync(struct hci_dev *hdev, u8 type,
+				       u16 interval, u16 window,

total: 0 errors, 1 warnings, 3 checks, 671 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 3" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS - 0.51 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 506.87 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 330.53 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.51 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.86 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - FAIL - 31.96 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 431 (96.6%), Failed: 1, Not Run: 14

Failed Test Cases
Add Ext Advertising - Success 21 (Timeout expires)   Timed out    3.348 seconds

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.02 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 1.97 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - PASS - 2.05 seconds
Run test-runner with smp-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: userchan-tester - PASS - 1.89 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 43b08bebae74..de95c47aaf77 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -302,6 +302,17 @@  struct amp_assoc {
 
 #define HCI_MAX_PAGES	3
 
+typedef int (*cmd_sync_work_func_t)(struct hci_dev *hdev, void *data);
+typedef void (*cmd_sync_work_destroy_t)(struct hci_dev *hdev, void *data,
+					int err);
+
+struct cmd_sync_work_entry {
+	struct list_head list;
+	cmd_sync_work_func_t func;
+	void *data;
+	cmd_sync_work_destroy_t destroy;
+};
+
 struct hci_dev {
 	struct list_head list;
 	struct mutex	lock;
@@ -463,6 +474,9 @@  struct hci_dev {
 	struct work_struct	power_on;
 	struct delayed_work	power_off;
 	struct work_struct	error_reset;
+	struct work_struct	cmd_sync_work;
+	struct list_head	cmd_sync_work_list;
+	struct mutex		cmd_sync_work_lock;
 
 	__u16			discov_timeout;
 	struct delayed_work	discov_off;
@@ -1701,6 +1715,9 @@  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
 struct sk_buff *hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen,
 			     const void *param, u32 timeout);
 
+int hci_cmd_sync_queue(struct hci_dev *hdev, cmd_sync_work_func_t func,
+		       void *data, cmd_sync_work_destroy_t destroy);
+
 u32 hci_conn_get_phy(struct hci_conn *conn);
 
 /* ----- HCI Sockets ----- */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 6eedf334f943..ba407976066b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2329,6 +2329,81 @@  static void hci_error_reset(struct work_struct *work)
 	hci_dev_do_open(hdev);
 }
 
+static void hci_cmd_sync_work(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
+	struct cmd_sync_work_entry *entry;
+	cmd_sync_work_func_t func;
+	cmd_sync_work_destroy_t destroy;
+	void *data;
+
+	bt_dev_dbg(hdev, "");
+
+	mutex_lock(&hdev->cmd_sync_work_lock);
+	entry = list_first_entry(&hdev->cmd_sync_work_list,
+				 struct cmd_sync_work_entry, list);
+	if (entry) {
+		list_del(&entry->list);
+		func = entry->func;
+		data = entry->data;
+		destroy = entry->destroy;
+		kfree(entry);
+	} else {
+		func = NULL;
+		data = NULL;
+		destroy = NULL;
+	}
+	mutex_unlock(&hdev->cmd_sync_work_lock);
+
+	if (func) {
+		int err;
+
+		hci_req_sync_lock(hdev);
+
+		err = func(hdev, data);
+
+		if (destroy)
+			destroy(hdev, data, err);
+
+		hci_req_sync_unlock(hdev);
+	}
+}
+
+int hci_cmd_sync_queue(struct hci_dev *hdev, cmd_sync_work_func_t func,
+		       void *data, cmd_sync_work_destroy_t destroy)
+{
+	struct cmd_sync_work_entry *entry;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	entry->func = func;
+	entry->data = data;
+	entry->destroy = destroy;
+
+	mutex_lock(&hdev->cmd_sync_work_lock);
+	list_add_tail(&entry->list, &hdev->cmd_sync_work_list);
+	mutex_unlock(&hdev->cmd_sync_work_lock);
+
+	queue_work(hdev->req_workqueue, &hdev->cmd_sync_work);
+
+	return 0;
+}
+
+static void hci_cmd_sync_clear(struct hci_dev *hdev)
+{
+	struct cmd_sync_work_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) {
+		if (entry->destroy)
+			entry->destroy(hdev, entry->data, -ECANCELED);
+
+		list_del(&entry->list);
+		kfree(entry);
+	}
+}
+
 void hci_uuids_clear(struct hci_dev *hdev)
 {
 	struct bt_uuid *uuid, *tmp;
@@ -3845,6 +3920,10 @@  struct hci_dev *hci_alloc_dev(void)
 	INIT_WORK(&hdev->error_reset, hci_error_reset);
 	INIT_WORK(&hdev->suspend_prepare, hci_prepare_suspend);
 
+	INIT_WORK(&hdev->cmd_sync_work, hci_cmd_sync_work);
+	INIT_LIST_HEAD(&hdev->cmd_sync_work_list);
+	mutex_init(&hdev->cmd_sync_work_lock);
+
 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
 
 	skb_queue_head_init(&hdev->rx_q);
@@ -4005,6 +4084,9 @@  void hci_unregister_dev(struct hci_dev *hdev)
 
 	cancel_work_sync(&hdev->power_on);
 
+	cancel_work_sync(&hdev->cmd_sync_work);
+	hci_cmd_sync_clear(hdev);
+
 	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks)) {
 		hci_suspend_clear_tasks(hdev);
 		unregister_pm_notifier(&hdev->suspend_notifier);