| Message ID | 20220302191100.364431-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work | expand |
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 | 64 ++++++++++++++++++++++++---------------- > 1 file changed, 38 insertions(+), 26 deletions(-) > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index d146d4efae43..121df2dcf2f1 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -273,43 +273,55 @@ 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); > + 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); > +} > + > +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, ""); > + > + /* 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; > > - hci_req_sync_unlock(hdev); > + hci_cmd_sync_work_entry_run(hdev, entry); > } > } > I was dead serious with my example on how to do this. Scrap the hci_cmd_sync_work_entry_run and do this all in one while loop. Regards Marcel
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=619678 ---Test result--- Test Summary: CheckPatch PASS 1.79 seconds GitLint FAIL 1.13 seconds SubjectPrefix PASS 0.98 seconds BuildKernel PASS 36.27 seconds BuildKernel32 PASS 32.69 seconds Incremental Build with patchesPASS 44.44 seconds TestRunner: Setup PASS 574.35 seconds TestRunner: l2cap-tester PASS 15.73 seconds TestRunner: bnep-tester PASS 7.38 seconds TestRunner: mgmt-tester PASS 119.53 seconds TestRunner: rfcomm-tester FAIL 9.16 seconds TestRunner: sco-tester PASS 9.29 seconds TestRunner: smp-tester PASS 9.34 seconds TestRunner: userchan-tester PASS 7.72 seconds Details ############################## Test: GitLint - FAIL - 1.13 seconds Run gitlint with rule in .gitlint 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 - 9.16 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.186 seconds --- Regards, Linux Bluetooth
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index d146d4efae43..121df2dcf2f1 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -273,43 +273,55 @@ 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); + 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); +} + +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, ""); + + /* 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; - hci_req_sync_unlock(hdev); + hci_cmd_sync_work_entry_run(hdev, entry); } }