[RFC] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work

[RFC] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The could be multiple hci_cmd_sync_work_entry enqueued when
hci_cmd_sync_work is executed so this makes sure they are all
dequeued properly.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_sync.c | 65 ++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index d146d4efae43..724d34bdd62a 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -273,43 +273,56 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
 }
 EXPORT_SYMBOL(__hci_cmd_sync_status);
 
-static void hci_cmd_sync_work(struct work_struct *work)
+
+static void hci_cmd_sync_work_entry_run(struct hci_dev *hdev,
+					struct hci_cmd_sync_work_entry *entry)
 {
-	struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
-	struct hci_cmd_sync_work_entry *entry;
 	hci_cmd_sync_work_func_t func;
 	hci_cmd_sync_work_destroy_t destroy;
 	void *data;
+	int err;
 
-	bt_dev_dbg(hdev, "");
+	bt_dev_dbg(hdev, "entry %p", entry);
 
-	mutex_lock(&hdev->cmd_sync_work_lock);
-	entry = list_first_entry(&hdev->cmd_sync_work_list,
-				 struct hci_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);
+	func = entry->func;
+	data = entry->data;
+	destroy = entry->destroy;
+	kfree(entry);
 
-	if (func) {
-		int err;
+	if (!func)
+		return;
+
+	hci_req_sync_lock(hdev);
+
+	err = func(hdev, data);
 
-		hci_req_sync_lock(hdev);
+	if (destroy)
+		destroy(hdev, data, err);
 
-		err = func(hdev, data);
+	hci_req_sync_unlock(hdev);
+}
 
-		if (destroy)
-			destroy(hdev, data, err);
+static void hci_cmd_sync_work(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
+	struct hci_cmd_sync_work_entry *entry;
+
+	bt_dev_dbg(hdev, "");
+
+	while (1) {
+		mutex_lock(&hdev->cmd_sync_work_lock);
+		entry = list_first_entry_or_null(&hdev->cmd_sync_work_list,
+						 struct hci_cmd_sync_work_entry,
+						 list);
+		if (!entry) {
+			mutex_unlock(&hdev->cmd_sync_work_lock);
+			break;
+		}
+
+		list_del(&entry->list);
+		mutex_unlock(&hdev->cmd_sync_work_lock);
 
-		hci_req_sync_unlock(hdev);
+		hci_cmd_sync_work_entry_run(hdev, entry);
 	}
 }
 
-- 
2.35.1

RE: [RFC] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

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=619444

---Test result---

Test Summary:
CheckPatch                    FAIL      1.66 seconds
GitLint                       PASS      0.99 seconds
SubjectPrefix                 PASS      0.89 seconds
BuildKernel                   PASS      30.15 seconds
BuildKernel32                 PASS      26.78 seconds
Incremental Build with patchesPASS      37.21 seconds
TestRunner: Setup             PASS      477.73 seconds
TestRunner: l2cap-tester      PASS      13.67 seconds
TestRunner: bnep-tester       PASS      6.09 seconds
TestRunner: mgmt-tester       PASS      105.01 seconds
TestRunner: rfcomm-tester     FAIL      7.68 seconds
TestRunner: sco-tester        PASS      7.80 seconds
TestRunner: smp-tester        PASS      7.68 seconds
TestRunner: userchan-tester   PASS      6.52 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.66 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[RFC] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work\CHECK:LINE_SPACING: Please don't use multiple blank lines
#99: FILE: net/bluetooth/hci_sync.c:276:
 
+

total: 0 errors, 0 warnings, 1 checks, 82 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.

/github/workspace/src/12765538.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

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


##############################
Test: TestRunner: rfcomm-tester - FAIL - 7.68 seconds
Run test-runner with rfcomm-tester
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0

Failed Test Cases
Basic RFCOMM Socket Client - Write 32k Success       Failed       0.155 seconds



---
Regards,
Linux Bluetooth

Re: [RFC] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work

[Marcel Holtmann]

Hi Luiz,

> The could be multiple hci_cmd_sync_work_entry enqueued when
> hci_cmd_sync_work is executed so this makes sure they are all
> dequeued properly.

might want to be a bit more verbose in the issue description.

> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_sync.c | 65 ++++++++++++++++++++++++----------------
> 1 file changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index d146d4efae43..724d34bdd62a 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -273,43 +273,56 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
> }
> EXPORT_SYMBOL(__hci_cmd_sync_status);
> 
> -static void hci_cmd_sync_work(struct work_struct *work)
> +
> +static void hci_cmd_sync_work_entry_run(struct hci_dev *hdev,
> +					struct hci_cmd_sync_work_entry *entry)
> {
> -	struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
> -	struct hci_cmd_sync_work_entry *entry;
> 	hci_cmd_sync_work_func_t func;
> 	hci_cmd_sync_work_destroy_t destroy;
> 	void *data;
> +	int err;
> 
> -	bt_dev_dbg(hdev, "");
> +	bt_dev_dbg(hdev, "entry %p", entry);
> 
> -	mutex_lock(&hdev->cmd_sync_work_lock);
> -	entry = list_first_entry(&hdev->cmd_sync_work_list,
> -				 struct hci_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);
> +	func = entry->func;
> +	data = entry->data;
> +	destroy = entry->destroy;
> +	kfree(entry);
> 
> -	if (func) {
> -		int err;
> +	if (!func)
> +		return;
> +
> +	hci_req_sync_lock(hdev);
> +
> +	err = func(hdev, data);
> 
> -		hci_req_sync_lock(hdev);
> +	if (destroy)
> +		destroy(hdev, data, err);
> 
> -		err = func(hdev, data);
> +	hci_req_sync_unlock(hdev);
> +}
> 
> -		if (destroy)
> -			destroy(hdev, data, err);
> +static void hci_cmd_sync_work(struct work_struct *work)
> +{
> +	struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
> +	struct hci_cmd_sync_work_entry *entry;
> +
> +	bt_dev_dbg(hdev, "");
> +
> +	while (1) {
> +		mutex_lock(&hdev->cmd_sync_work_lock);
> +		entry = list_first_entry_or_null(&hdev->cmd_sync_work_list,
> +						 struct hci_cmd_sync_work_entry,
> +						 list);
> +		if (!entry) {
> +			mutex_unlock(&hdev->cmd_sync_work_lock);
> +			break;
> +		}
> +
> +		list_del(&entry->list);
> +		mutex_unlock(&hdev->cmd_sync_work_lock);
> 
> -		hci_req_sync_unlock(hdev);
> +		hci_cmd_sync_work_entry_run(hdev, entry);
> 	}
> }

In general I don’t like having two unlock vs one lock in the code. I know it is correct and the tools will also understand it is correct, but I just dislike it. I think it makes it hard for human eyes to verify locking balance.

I would prefer something like this:

	while (1) {
		struct hci_cmd_sync_work_entry *entry;
		int err;

		mutex_lock(&hdev->cmd_sync_work_lock);
		entry = list_first_entry_or_null(&hdev->cmd_..
		if (entry)
			list_del(&entry->list);
		mutex_unlock(&hdev->cmd_sync_work_lock);

		if (!entry)
			break;

		bt_dev_dbg(hdev, "entry %p", entry);

		if (entry->func) {
			hci_req_sync_lock(hdev);
			err = func(hdev, entry->data);
			if (entry->destroy)
				entry->destroy(hdev, entry->data, err);
			hci_req_sync_unlock(hdev);
		}

		kfree(entry);
	}

This looks a lot easy to read and verify to me.

Regards

Marcel