Hi Brian, On Thu, Feb 23, 2023 at 2:21 PM Gix, Brian <brian.gix@xxxxxxxxx> 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 <brian.gix@xxxxxxxxx> > > > > > --- > > > > > 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 <luiz.von.dentz@xxxxxxxxx> > > > > --- > > 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
