2023-02-17 19:32:56

by Mateusz Jończyk

[permalink] [raw]
Subject: Re: [PATCH 1/1] Bluetooth: Convert MSFT filter HCI cmd to hci_sync

W dniu 21.11.2022 o 19:48, Gix, Brian pisze:
> Hi Mateusz,
>
> On Sat, 2022-11-19 at 19:26 +0100, Mateusz Jończyk wrote:
>> W dniu 2.11.2022 o 18:59, Brian Gix pisze:
>>> The msft_set_filter_enable() command was using the deprecated
>>> hci_request mechanism rather than hci_sync. This caused the warning
>>> error:
>>> hci0: HCI_REQ-0xfcf0
>>>
>>> Signed-off-by: Brian Gix <[email protected]>
>>> ---
>>> net/bluetooth/msft.c | 36 +++++++++++-------------------------
>>> 1 file changed, 11 insertions(+), 25 deletions(-)
>>
>> Hello,
>>
>> On the first time I was testing this patch, I was heavily testing
>> various Bluetooth functionality. Eventually, something stopped
>> working
>> and I was no longer able to establish a Bluetooth connection between
>> my mobile phone and my laptop. To try to resolve this issue, I tried
>> suspending
>> my laptop, but after resume, I got a kernel panic (dmesg attached
>> below, I may
>> provide journal extracts on request).

Hello,

I have to apologize for the delay.

This kernel panic happened to me again, without this patch so now I can
say that it is unrelated. I have again tested the patch you provided and
Bluetooth with it appeared to be working correctly. I have even tested
with a mobile app called "Beacon Simulator" [1] and the beacon was
successfully found on my laptop.

I have tested this on top of 6.2.0-rc8.

Additionally, to see how it worked, I tested it with some debugging
printks and additional calls to
msft_set_filter_enable(hdev, X);
(patch attached below) and the code appears to do as it is intended, but
when I combined two
msft_set_filter_enable(hdev, true);
calls one after the other, I got something like this:

Bluetooth: hci0: BEGIN msft_set_filter_enable(hdev, 1)
Bluetooth: hci0: BEGIN msft_le_set_advertisement_filter_enable_cb(hdev, user_data, 0)
Bluetooth: hci0: END msft_set_filter_enable(hdev, 1), err = 0
Bluetooth: hci0: BEGIN msft_set_filter_enable(hdev, 1)
Bluetooth: hci0: Opcode 0xfcf0 failed: -16
Bluetooth: hci0: BEGIN msft_le_set_advertisement_filter_enable_cb(hdev, user_data, f0)
Bluetooth: hci0: END msft_set_filter_enable(hdev, 1), err = -16

instead of the error code 0x0C as is described in a comment in
msft_le_set_advertisement_filter_enable_cb() and the Microsoft
specification. Is this expected?

In msft_set_filter_enable() your patch also casts err from int to u8
while calling msft_le_set_advertisement_filter_enable_cb() without
checking its sign.

Greetings,

Mateusz

[1] https://play.google.com/store/apps/details?id=net.alea.beaconsimulator

Cc: Luiz Von Dentz <[email protected]>

---
net/bluetooth/msft.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index bf5cee48916c..a6f594bd0722 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -6,6 +6,7 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
#include <net/bluetooth/mgmt.h>
+#include <linux/delay.h>

#include "hci_request.h"
#include "mgmt_util.h"
@@ -527,6 +528,12 @@ void msft_do_open(struct hci_dev *hdev)
if (msft_monitor_supported(hdev)) {
msft->resuming = true;
msft_set_filter_enable(hdev, true);
+ mdelay(1);
+ msft_set_filter_enable(hdev, false);
+ mdelay(1);
+ msft_set_filter_enable(hdev, true);
+ mdelay(1);
+ msft_set_filter_enable(hdev, true);
/* Monitors get removed on power off, so we need to explicitly
* tell the controller to re-monitor.
*/
@@ -749,6 +756,8 @@ static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
struct msft_cp_le_set_advertisement_filter_enable *cp = user_data;
struct msft_data *msft = hdev->msft_data;

+ bt_dev_warn(hdev, "BEGIN msft_le_set_advertisement_filter_enable_cb(hdev, user_data, %x)", (unsigned) status);
+
/* Error 0x0C would be returned if the filter enabled status is
* already set to whatever we were trying to set.
* Although the default state should be disabled, some controller set
@@ -804,6 +813,8 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable)
struct msft_data *msft = hdev->msft_data;
int err;

+ bt_dev_warn(hdev, "BEGIN msft_set_filter_enable(hdev, %d)", (int) enable);
+
if (!msft)
return -EOPNOTSUPP;

@@ -814,6 +825,8 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable)

msft_le_set_advertisement_filter_enable_cb(hdev, &cp, err);

+ bt_dev_warn(hdev, "END msft_set_filter_enable(hdev, %d), err = %d", (int) enable, err);
+
return 0;
}

--
2.25.1



2023-02-23 22:15:35

by Gix, Brian

[permalink] [raw]
Subject: Re: [PATCH 1/1] Bluetooth: Convert MSFT filter HCI cmd to hci_sync

Hi Luiz,

Do you need the original patch re-generated, or can you find it in this
thread?

On Fri, 2023-02-17 at 20:31 +0100, Mateusz Jończyk wrote:
> W dniu 21.11.2022 o 19:48, Gix, Brian pisze:
> > Hi Mateusz,
> >
> > On Sat, 2022-11-19 at 19:26 +0100, Mateusz Jończyk wrote:
> > > W dniu 2.11.2022 o 18:59, Brian Gix pisze:
> > > > The msft_set_filter_enable() command was using the deprecated
> > > > hci_request mechanism rather than hci_sync. This caused the
> > > > warning
> > > > error:
> > > > hci0: HCI_REQ-0xfcf0
> > > >
> > > > Signed-off-by: Brian Gix <[email protected]>
> > > > ---
> > > >  net/bluetooth/msft.c | 36 +++++++++++-------------------------
> > > >  1 file changed, 11 insertions(+), 25 deletions(-)
> > >
> > > Hello,
> > >
> > > On the first time I was testing this patch, I was heavily testing
> > > various Bluetooth functionality. Eventually, something stopped
> > > working
> > > and I was no longer able to establish a Bluetooth connection
> > > between
> > > my mobile phone and my laptop. To try to resolve this issue, I
> > > tried
> > > suspending
> > > my laptop, but after resume, I got a kernel panic (dmesg attached
> > > below, I may
> > > provide journal extracts on request).
>
> Hello,
>
> I have to apologize for the delay.
>
> This kernel panic happened to me again, without this patch so now I
> can
> say that it is unrelated. I have again tested the patch you provided
> and
> Bluetooth with it appeared to be working correctly. I have even
> tested
> with a mobile app called "Beacon Simulator" [1] and the beacon was
> successfully found on my laptop.
>
> I have tested this on top of 6.2.0-rc8.
>
> Additionally, to see how it worked, I tested it with some debugging
> printks and additional calls to
>         msft_set_filter_enable(hdev, X);
> (patch attached below) and the code appears to do as it is intended,
> but
> when I combined two
>         msft_set_filter_enable(hdev, true);
> calls one after the other, I got something like this:
>
>         Bluetooth: hci0: BEGIN msft_set_filter_enable(hdev, 1)
>         Bluetooth: hci0: BEGIN
> msft_le_set_advertisement_filter_enable_cb(hdev, user_data, 0)
>         Bluetooth: hci0: END msft_set_filter_enable(hdev, 1), err = 0
>         Bluetooth: hci0: BEGIN msft_set_filter_enable(hdev, 1)
>         Bluetooth: hci0: Opcode 0xfcf0 failed: -16
>         Bluetooth: hci0: BEGIN
> msft_le_set_advertisement_filter_enable_cb(hdev, user_data, f0)
>         Bluetooth: hci0: END msft_set_filter_enable(hdev, 1), err = -
> 16
>
> instead of the error code 0x0C as is described in a comment in
> msft_le_set_advertisement_filter_enable_cb() and the Microsoft
> specification. Is this expected?
>
> In msft_set_filter_enable() your patch also casts err from int to u8
> while calling msft_le_set_advertisement_filter_enable_cb() without
> checking its sign.
>
> Greetings,
>
> Mateusz
>
> [1]
> https://play.google.com/store/apps/details?id=net.alea.beaconsimulator
>
> Cc: Luiz Von Dentz <[email protected]>
>
> ---
>  net/bluetooth/msft.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index bf5cee48916c..a6f594bd0722 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -6,6 +6,7 @@
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
>  #include <net/bluetooth/mgmt.h>
> +#include <linux/delay.h>
>  
>  #include "hci_request.h"
>  #include "mgmt_util.h"
> @@ -527,6 +528,12 @@ void msft_do_open(struct hci_dev *hdev)
>         if (msft_monitor_supported(hdev)) {
>                 msft->resuming = true;
>                 msft_set_filter_enable(hdev, true);
> +               mdelay(1);
> +               msft_set_filter_enable(hdev, false);
> +               mdelay(1);
> +               msft_set_filter_enable(hdev, true);
> +               mdelay(1);
> +               msft_set_filter_enable(hdev, true);
>                 /* Monitors get removed on power off, so we need to
> explicitly
>                  * tell the controller to re-monitor.
>                  */
> @@ -749,6 +756,8 @@ static void
> msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
>         struct msft_cp_le_set_advertisement_filter_enable *cp =
> user_data;
>         struct msft_data *msft = hdev->msft_data;
>  
> +       bt_dev_warn(hdev, "BEGIN
> msft_le_set_advertisement_filter_enable_cb(hdev, user_data, %x)",
> (unsigned) status);
> +
>         /* Error 0x0C would be returned if the filter enabled status
> is
>          * already set to whatever we were trying to set.
>          * Although the default state should be disabled, some
> controller set
> @@ -804,6 +813,8 @@ int msft_set_filter_enable(struct hci_dev *hdev,
> bool enable)
>         struct msft_data *msft = hdev->msft_data;
>         int err;
>  
> +       bt_dev_warn(hdev, "BEGIN msft_set_filter_enable(hdev, %d)",
> (int) enable);
> +
>         if (!msft)
>                 return -EOPNOTSUPP;
>  
> @@ -814,6 +825,8 @@ int msft_set_filter_enable(struct hci_dev *hdev,
> bool enable)
>  
>         msft_le_set_advertisement_filter_enable_cb(hdev, &cp, err);
>  
> +       bt_dev_warn(hdev, "END msft_set_filter_enable(hdev, %d), err
> = %d", (int) enable, err);
> +
>         return 0;
>  }
>  

2023-02-24 01:07:48

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 1/1] Bluetooth: Convert MSFT filter HCI cmd to hci_sync

Hi Brian,

On Thu, Feb 23, 2023 at 2:21 PM Gix, Brian <[email protected]> wrote:
>
> Hi Luiz,
>
> Do you need the original patch re-generated, or can you find it in this
> thread?

Please resend it so CI can pick it up and ensures it still builds properly.

> On Fri, 2023-02-17 at 20:31 +0100, Mateusz Jończyk wrote:
> > W dniu 21.11.2022 o 19:48, Gix, Brian pisze:
> > > Hi Mateusz,
> > >
> > > On Sat, 2022-11-19 at 19:26 +0100, Mateusz Jończyk wrote:
> > > > W dniu 2.11.2022 o 18:59, Brian Gix pisze:
> > > > > The msft_set_filter_enable() command was using the deprecated
> > > > > hci_request mechanism rather than hci_sync. This caused the
> > > > > warning
> > > > > error:
> > > > > hci0: HCI_REQ-0xfcf0
> > > > >
> > > > > Signed-off-by: Brian Gix <[email protected]>
> > > > > ---
> > > > > net/bluetooth/msft.c | 36 +++++++++++-------------------------
> > > > > 1 file changed, 11 insertions(+), 25 deletions(-)
> > > >
> > > > Hello,
> > > >
> > > > On the first time I was testing this patch, I was heavily testing
> > > > various Bluetooth functionality. Eventually, something stopped
> > > > working
> > > > and I was no longer able to establish a Bluetooth connection
> > > > between
> > > > my mobile phone and my laptop. To try to resolve this issue, I
> > > > tried
> > > > suspending
> > > > my laptop, but after resume, I got a kernel panic (dmesg attached
> > > > below, I may
> > > > provide journal extracts on request).
> >
> > Hello,
> >
> > I have to apologize for the delay.
> >
> > This kernel panic happened to me again, without this patch so now I
> > can
> > say that it is unrelated. I have again tested the patch you provided
> > and
> > Bluetooth with it appeared to be working correctly. I have even
> > tested
> > with a mobile app called "Beacon Simulator" [1] and the beacon was
> > successfully found on my laptop.
> >
> > I have tested this on top of 6.2.0-rc8.
> >
> > Additionally, to see how it worked, I tested it with some debugging
> > printks and additional calls to
> > msft_set_filter_enable(hdev, X);
> > (patch attached below) and the code appears to do as it is intended,
> > but
> > when I combined two
> > msft_set_filter_enable(hdev, true);
> > calls one after the other, I got something like this:
> >
> > Bluetooth: hci0: BEGIN msft_set_filter_enable(hdev, 1)
> > Bluetooth: hci0: BEGIN
> > msft_le_set_advertisement_filter_enable_cb(hdev, user_data, 0)
> > Bluetooth: hci0: END msft_set_filter_enable(hdev, 1), err = 0
> > Bluetooth: hci0: BEGIN msft_set_filter_enable(hdev, 1)
> > Bluetooth: hci0: Opcode 0xfcf0 failed: -16
> > Bluetooth: hci0: BEGIN
> > msft_le_set_advertisement_filter_enable_cb(hdev, user_data, f0)
> > Bluetooth: hci0: END msft_set_filter_enable(hdev, 1), err = -
> > 16
> >
> > instead of the error code 0x0C as is described in a comment in
> > msft_le_set_advertisement_filter_enable_cb() and the Microsoft
> > specification. Is this expected?
> >
> > In msft_set_filter_enable() your patch also casts err from int to u8
> > while calling msft_le_set_advertisement_filter_enable_cb() without
> > checking its sign.
> >
> > Greetings,
> >
> > Mateusz
> >
> > [1]
> > https://play.google.com/store/apps/details?id=net.alea.beaconsimulator
> >
> > Cc: Luiz Von Dentz <[email protected]>
> >
> > ---
> > net/bluetooth/msft.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index bf5cee48916c..a6f594bd0722 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -6,6 +6,7 @@
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
> > #include <net/bluetooth/mgmt.h>
> > +#include <linux/delay.h>
> >
> > #include "hci_request.h"
> > #include "mgmt_util.h"
> > @@ -527,6 +528,12 @@ void msft_do_open(struct hci_dev *hdev)
> > if (msft_monitor_supported(hdev)) {
> > msft->resuming = true;
> > msft_set_filter_enable(hdev, true);
> > + mdelay(1);
> > + msft_set_filter_enable(hdev, false);
> > + mdelay(1);
> > + msft_set_filter_enable(hdev, true);
> > + mdelay(1);
> > + msft_set_filter_enable(hdev, true);
> > /* Monitors get removed on power off, so we need to
> > explicitly
> > * tell the controller to re-monitor.
> > */
> > @@ -749,6 +756,8 @@ static void
> > msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
> > struct msft_cp_le_set_advertisement_filter_enable *cp =
> > user_data;
> > struct msft_data *msft = hdev->msft_data;
> >
> > + bt_dev_warn(hdev, "BEGIN
> > msft_le_set_advertisement_filter_enable_cb(hdev, user_data, %x)",
> > (unsigned) status);
> > +
> > /* Error 0x0C would be returned if the filter enabled status
> > is
> > * already set to whatever we were trying to set.
> > * Although the default state should be disabled, some
> > controller set
> > @@ -804,6 +813,8 @@ int msft_set_filter_enable(struct hci_dev *hdev,
> > bool enable)
> > struct msft_data *msft = hdev->msft_data;
> > int err;
> >
> > + bt_dev_warn(hdev, "BEGIN msft_set_filter_enable(hdev, %d)",
> > (int) enable);
> > +
> > if (!msft)
> > return -EOPNOTSUPP;
> >
> > @@ -814,6 +825,8 @@ int msft_set_filter_enable(struct hci_dev *hdev,
> > bool enable)
> >
> > msft_le_set_advertisement_filter_enable_cb(hdev, &cp, err);
> >
> > + bt_dev_warn(hdev, "END msft_set_filter_enable(hdev, %d), err
> > = %d", (int) enable, err);
> > +
> > return 0;
> > }
> >
>


--
Luiz Augusto von Dentz