* [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync()
@ 2022-05-09 21:05 Manish Mandlik
2022-05-09 22:03 ` bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Manish Mandlik @ 2022-05-09 21:05 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth, Manish Mandlik,
David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg,
Paolo Abeni, linux-kernel, netdev
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);
msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
--
2.36.0.512.ge40c2bad7a-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync()
2022-05-09 21:05 [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync() Manish Mandlik
@ 2022-05-09 22:03 ` bluez.test.bot
2022-05-10 5:55 ` [PATCH] " Paul Menzel
2022-05-11 21:23 ` Luiz Augusto von Dentz
2 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2022-05-09 22:03 UTC (permalink / raw)
To: linux-bluetooth, mmandlik
[-- Attachment #1: Type: text/plain, Size: 1097 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=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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync()
2022-05-09 21:05 [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync() Manish Mandlik
2022-05-09 22:03 ` bluez.test.bot
@ 2022-05-10 5:55 ` Paul Menzel
2022-05-11 21:23 ` Luiz Augusto von Dentz
2 siblings, 0 replies; 5+ messages in thread
From: Paul Menzel @ 2022-05-10 5:55 UTC (permalink / raw)
To: Manish Mandlik
Cc: marcel, luiz.dentz, chromeos-bluetooth-upstreaming,
linux-bluetooth, David S. Miller, Eric Dumazet, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, linux-kernel, netdev
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync()
2022-05-09 21:05 [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync() Manish Mandlik
2022-05-09 22:03 ` bluez.test.bot
2022-05-10 5:55 ` [PATCH] " Paul Menzel
@ 2022-05-11 21:23 ` Luiz Augusto von Dentz
[not found] ` <CAGPPCLBTec4SuL+UiFPkvq9=Bz8UY_3nQa5JjjHZ_415yt7KjA@mail.gmail.com>
2 siblings, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-11 21:23 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg,
Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
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
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync()
[not found] ` <CAGPPCLBTec4SuL+UiFPkvq9=Bz8UY_3nQa5JjjHZ_415yt7KjA@mail.gmail.com>
@ 2022-05-12 4:35 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-12 4:35 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg,
Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
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.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-12 4:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-09 21:05 [PATCH] Bluetooth: Fix Adv Monitor msft_add/remove_monitor_sync() Manish Mandlik
2022-05-09 22:03 ` bluez.test.bot
2022-05-10 5:55 ` [PATCH] " Paul Menzel
2022-05-11 21:23 ` Luiz Augusto von Dentz
[not found] ` <CAGPPCLBTec4SuL+UiFPkvq9=Bz8UY_3nQa5JjjHZ_415yt7KjA@mail.gmail.com>
2022-05-12 4:35 ` Luiz Augusto von Dentz
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.