* [PATCH v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work
@ 2022-03-02 21:02 Luiz Augusto von Dentz
2022-03-02 22:16 ` [v2] " bluez.test.bot
2022-03-03 12:36 ` [PATCH v2] " Marcel Holtmann
0 siblings, 2 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2022-03-02 21:02 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
hci_cmd_sync_queue can be called multiple times, each adding a
hci_cmd_sync_work_entry, before hci_cmd_sync_work is run so this makes
sure they are all dequeued properly otherwise it creates a backlog of
entries that are never run.
Link: https://lore.kernel.org/all/CAJCQCtSeUtHCgsHXLGrSTWKmyjaQDbDNpP4rb0i+RE+L2FTXSA@mail.gmail.com/T/
Fixes: 6a98e3836fa20 ("Bluetooth: Add helper for serialized HCI command execution")
Tested-by: Chris Clayton <chris2553@googlemail.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/hci_sync.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index d146d4efae43..06c6e954dcbd 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -283,33 +283,36 @@ static void hci_cmd_sync_work(struct work_struct *work)
bt_dev_dbg(hdev, "");
- 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);
+ /* Dequeue all entries and run them */
+ 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)
+ list_del(&entry->list);
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ if (!entry)
+ break;
+
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;
+ if (func) {
+ int err;
- hci_req_sync_lock(hdev);
+ hci_req_sync_lock(hdev);
- err = func(hdev, data);
+ err = func(hdev, data);
- if (destroy)
- destroy(hdev, data, err);
+ if (destroy)
+ destroy(hdev, data, err);
- hci_req_sync_unlock(hdev);
+ hci_req_sync_unlock(hdev);
+ }
}
}
--
2.35.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work
2022-03-02 21:02 [PATCH v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work Luiz Augusto von Dentz
@ 2022-03-02 22:16 ` bluez.test.bot
2022-03-03 12:36 ` [PATCH v2] " Marcel Holtmann
1 sibling, 0 replies; 3+ messages in thread
From: bluez.test.bot @ 2022-03-02 22:16 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 1701 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=619699
---Test result---
Test Summary:
CheckPatch PASS 0.87 seconds
GitLint FAIL 0.46 seconds
SubjectPrefix PASS 0.32 seconds
BuildKernel PASS 34.50 seconds
BuildKernel32 PASS 30.26 seconds
Incremental Build with patchesPASS 42.30 seconds
TestRunner: Setup PASS 545.20 seconds
TestRunner: l2cap-tester PASS 14.76 seconds
TestRunner: bnep-tester PASS 6.57 seconds
TestRunner: mgmt-tester PASS 113.78 seconds
TestRunner: rfcomm-tester FAIL 8.42 seconds
TestRunner: sco-tester PASS 8.55 seconds
TestRunner: smp-tester PASS 8.35 seconds
TestRunner: userchan-tester PASS 6.92 seconds
Details
##############################
Test: GitLint - FAIL - 0.46 seconds
Run gitlint with rule in .gitlint
[v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work
10: B1 Line exceeds max length (103>80): "Link: https://lore.kernel.org/all/CAJCQCtSeUtHCgsHXLGrSTWKmyjaQDbDNpP4rb0i+RE+L2FTXSA@mail.gmail.com/T/"
##############################
Test: TestRunner: rfcomm-tester - FAIL - 8.42 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.178 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work
2022-03-02 21:02 [PATCH v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work Luiz Augusto von Dentz
2022-03-02 22:16 ` [v2] " bluez.test.bot
@ 2022-03-03 12:36 ` Marcel Holtmann
1 sibling, 0 replies; 3+ messages in thread
From: Marcel Holtmann @ 2022-03-03 12:36 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> hci_cmd_sync_queue can be called multiple times, each adding a
> hci_cmd_sync_work_entry, before hci_cmd_sync_work is run so this makes
> sure they are all dequeued properly otherwise it creates a backlog of
> entries that are never run.
>
> Link: https://lore.kernel.org/all/CAJCQCtSeUtHCgsHXLGrSTWKmyjaQDbDNpP4rb0i+RE+L2FTXSA@mail.gmail.com/T/
> Fixes: 6a98e3836fa20 ("Bluetooth: Add helper for serialized HCI command execution")
> Tested-by: Chris Clayton <chris2553@googlemail.com>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_sync.c | 39 +++++++++++++++++++++------------------
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index d146d4efae43..06c6e954dcbd 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -283,33 +283,36 @@ static void hci_cmd_sync_work(struct work_struct *work)
>
> bt_dev_dbg(hdev, "");
>
> - 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);
> + /* Dequeue all entries and run them */
> + 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)
> + list_del(&entry->list);
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> + if (!entry)
> + break;
> +
> 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;
> + if (func) {
> + int err;
>
> - hci_req_sync_lock(hdev);
> + hci_req_sync_lock(hdev);
>
> - err = func(hdev, data);
> + err = func(hdev, data);
>
> - if (destroy)
> - destroy(hdev, data, err);
> + if (destroy)
> + destroy(hdev, data, err);
>
> - hci_req_sync_unlock(hdev);
> + hci_req_sync_unlock(hdev);
> + }
> }
> }
I really don’t get this. I already gave you how I think this should look like and you still keep the massive amount of variables for no apparent reason. If I am stupid and made a mistake, then please enlighten me, otherwise this should look like this:
@@ -276,40 +276,37 @@ EXPORT_SYMBOL(__hci_cmd_sync_status);
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;
- hci_cmd_sync_work_func_t func;
- hci_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 hci_cmd_sync_work_entry, list);
- if (entry) {
- list_del(&entry->list);
- func = entry->func;
- data = entry->data;
- destroy = entry->destroy;
+ /* Dequeue all entries and run them */
+ while (1) {
+ struct hci_cmd_sync_work_entry *entry;
+
+ 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)
+ list_del(&entry->list);
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ if (!entry)
+ break;
+
+ bt_dev_dbg(hdev, "entry %p", entry);
+
+ if (entry->func) {
+ int err;
+
+ hci_req_sync_lock(hdev);
+ err = entry->func(hdev, entry->data);
+ if (entry->destroy)
+ entry->destroy(hdev, entry->data, err);
+ hci_req_sync_unlock(hdev);
+ }
+
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);
}
}
I amended the patch now and applied it to bluetooth-stable tree. I am not doing any more iterations to get this right.
Regards
Marcel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-03 12:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-02 21:02 [PATCH v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work Luiz Augusto von Dentz
2022-03-02 22:16 ` [v2] " bluez.test.bot
2022-03-03 12:36 ` [PATCH v2] " Marcel Holtmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.