| Message ID | 20220302064351.225212-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work | expand |
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
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
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); } }