| Message ID | 20220509140403.1.I28d2ec514ad3b612015b28b8de861b8955033a19@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync() | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
| tedd_an/subjectprefix | success | PASS |
| tedd_an/buildkernel | success | Build Kernel PASS |
| tedd_an/buildkernel32 | success | Build Kernel32 PASS |
| tedd_an/incremental_build | success | Pass |
| tedd_an/testrunnersetup | success | Test Runner Setup PASS |
| tedd_an/testrunnerl2cap-tester | success | Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerbnep-tester | success | Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnermgmt-tester | success | Total: 493, Passed: 493 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerrfcomm-tester | success | Total: 10, Passed: 10 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersco-tester | success | Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersmp-tester | success | Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunneruserchan-tester | success | Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0 |
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=639914 ---Test result--- Test Summary: CheckPatch PASS 1.54 seconds GitLint PASS 0.96 seconds SubjectPrefix PASS 0.87 seconds BuildKernel PASS 31.03 seconds BuildKernel32 PASS 27.59 seconds Incremental Build with patchesPASS 37.79 seconds TestRunner: Setup PASS 469.43 seconds TestRunner: l2cap-tester PASS 17.48 seconds TestRunner: bnep-tester PASS 6.09 seconds TestRunner: mgmt-tester PASS 100.65 seconds TestRunner: rfcomm-tester PASS 9.66 seconds TestRunner: sco-tester PASS 9.45 seconds TestRunner: smp-tester PASS 9.36 seconds TestRunner: userchan-tester PASS 6.37 seconds --- Regards, Linux Bluetooth
Dear Manish, Thank you for your patch. Am 09.05.22 um 23:05 schrieb Manish Mandlik: > Do not call skb_pull() in msft_add_monitor_sync() as > msft_le_monitor_advertisement_cb() expects 'status' to be > part of the skb. Please reflow for 75 characters per line. > Same applies for msft_remove_monitor_sync(). Was this found by code review, or were there noticeable problems? If the later, please add a note, how to reproduce it. Also, maybe also add a Fixes tag, referencing the commit introducing the problem. Kind regards, Paul > Signed-off-by: Manish Mandlik <mmandlik@google.com> > --- > > net/bluetooth/msft.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c > index f43994523b1f..9990924719aa 100644 > --- a/net/bluetooth/msft.c > +++ b/net/bluetooth/msft.c > @@ -387,7 +387,6 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev, > return PTR_ERR(skb); > > status = skb->data[0]; > - skb_pull(skb, 1); > > msft_le_cancel_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, > skb); > @@ -506,7 +505,6 @@ static int msft_add_monitor_sync(struct hci_dev *hdev, > return PTR_ERR(skb); > > status = skb->data[0]; > - skb_pull(skb, 1); > > msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb); >
Hi Manish, On Mon, May 9, 2022 at 2:05 PM Manish Mandlik <mmandlik@google.com> wrote: > > Do not call skb_pull() in msft_add_monitor_sync() as > msft_le_monitor_advertisement_cb() expects 'status' to be > part of the skb. > > Same applies for msft_remove_monitor_sync(). > > Signed-off-by: Manish Mandlik <mmandlik@google.com> > --- > > net/bluetooth/msft.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c > index f43994523b1f..9990924719aa 100644 > --- a/net/bluetooth/msft.c > +++ b/net/bluetooth/msft.c > @@ -387,7 +387,6 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev, > return PTR_ERR(skb); > > status = skb->data[0]; > - skb_pull(skb, 1); > > msft_le_cancel_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, > skb); > @@ -506,7 +505,6 @@ static int msft_add_monitor_sync(struct hci_dev *hdev, > return PTR_ERR(skb); > > status = skb->data[0]; > - skb_pull(skb, 1); Well if it expects it to be part of the skb then there is no reason to pass it as argument in addition to the skb itself. > msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb); > > -- > 2.36.0.512.ge40c2bad7a-goog >
Hi Manish, On Wed, May 11, 2022 at 5:56 PM Manish Mandlik <mmandlik@google.com> wrote: > > Hi Luiz, > > On Wed, May 11, 2022 at 2:23 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> >> Hi Manish, >> >> On Mon, May 9, 2022 at 2:05 PM Manish Mandlik <mmandlik@google.com> wrote: >> > >> > Do not call skb_pull() in msft_add_monitor_sync() as >> > msft_le_monitor_advertisement_cb() expects 'status' to be >> > part of the skb. >> > >> > Same applies for msft_remove_monitor_sync(). >> > >> > Signed-off-by: Manish Mandlik <mmandlik@google.com> >> > --- >> > >> > net/bluetooth/msft.c | 2 -- >> > 1 file changed, 2 deletions(-) >> > >> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c >> > index f43994523b1f..9990924719aa 100644 >> > --- a/net/bluetooth/msft.c >> > +++ b/net/bluetooth/msft.c >> > @@ -387,7 +387,6 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev, >> > return PTR_ERR(skb); >> > >> > status = skb->data[0]; >> > - skb_pull(skb, 1); >> > >> > msft_le_cancel_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, >> > skb); >> > @@ -506,7 +505,6 @@ static int msft_add_monitor_sync(struct hci_dev *hdev, >> > return PTR_ERR(skb); >> > >> > status = skb->data[0]; >> > - skb_pull(skb, 1); >> >> Well if it expects it to be part of the skb then there is no reason to >> pass it as argument in addition to the skb itself. > > The problem is msft_le_monitor_advertisement_cb() is invoked directly via msft_add_monitor_sync() and also from __msft_add_monitor_pattern() as a callback from hci_req_run_skb(). So, when it is invoked from hci_req_run_skb() it sends status separately as an argument along with the skb and that's why that argument is required. > > Looks like some parts of msft.c still use the old way i.e. hci_req_run_skb() instead of __hci_cmd_sync() after hci_sync related refactoring. I am wondering if it was left like this intentionally? If not, then we probably need to refactor msft.c to use __hci_cmd_sync() for all hci requests. In that case, I can work on refactoring and we can discard this patch altogether. Please let me know. Yes, if you have time please convert it to use hci_sync.c since we would like to completely deprecate/remove hci_request.c eventually, if you think that will take some time we can perhaps merge this changes first though. >> >> > msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb); >> > >> > -- >> > 2.36.0.512.ge40c2bad7a-goog >> > >> >> >> -- >> Luiz Augusto von Dentz > > Regards, > Manish.
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c index f43994523b1f..9990924719aa 100644 --- a/net/bluetooth/msft.c +++ b/net/bluetooth/msft.c @@ -387,7 +387,6 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev, return PTR_ERR(skb); status = skb->data[0]; - skb_pull(skb, 1); msft_le_cancel_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb); @@ -506,7 +505,6 @@ static int msft_add_monitor_sync(struct hci_dev *hdev, return PTR_ERR(skb); status = skb->data[0]; - skb_pull(skb, 1); msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
Do not call skb_pull() in msft_add_monitor_sync() as msft_le_monitor_advertisement_cb() expects 'status' to be part of the skb. Same applies for msft_remove_monitor_sync(). Signed-off-by: Manish Mandlik <mmandlik@google.com> --- net/bluetooth/msft.c | 2 -- 1 file changed, 2 deletions(-)