[PATCH v2 1/4] Bluetooth: Keep MSFT ext info throughout a hci_dev's life cycle

[PATCH v2 1/4] Bluetooth: Keep MSFT ext info throughout a hci_dev's life cycle

[Miao-chen Chou]

This moves msft_do_close() from hci_dev_do_close() to
hci_unregister_dev() to avoid clearing MSFT extension info. This also
avoids retrieving MSFT info upon every msft_do_open() if MSFT extension
has been initialized.

The following test steps were performed.
(1) boot the test device and verify the MSFT support debug log in syslog
(2) restart bluetoothd and verify msft_do_close() doesn't get invoked

Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

(no changes since v1)

 net/bluetooth/hci_core.c | 4 ++--
 net/bluetooth/msft.c     | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9d2c9a1c552fd..8471be105a2ac 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1780,8 +1780,6 @@ int hci_dev_do_close(struct hci_dev *hdev)
 
 	hci_sock_dev_event(hdev, HCI_DEV_DOWN);
 
-	msft_do_close(hdev);
-
 	if (hdev->flush)
 		hdev->flush(hdev);
 
@@ -3869,6 +3867,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	unregister_pm_notifier(&hdev->suspend_notifier);
 	cancel_work_sync(&hdev->suspend_prepare);
 
+	msft_do_close(hdev);
+
 	hci_dev_do_close(hdev);
 
 	if (!test_bit(HCI_INIT, &hdev->flags) &&
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index 4b39534a14a18..d9d2269bc93ef 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -76,7 +76,8 @@ void msft_do_open(struct hci_dev *hdev)
 {
 	struct msft_data *msft;
 
-	if (hdev->msft_opcode == HCI_OP_NOP)
+	/* Skip if opcode is not supported or MSFT has been initiatlized */
+	if (hdev->msft_opcode == HCI_OP_NOP || hdev->msft_data)
 		return;
 
 	bt_dev_dbg(hdev, "Initialize MSFT extension");
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v2 2/4] Bluetooth: btqca: Enable MSFT extension for Qualcomm WCN399x

[Miao-chen Chou]

The following Qualcomm WCN399x Bluetooth controllers support the
Microsoft vendor extension and they are using 0xFD70 for VsMsftOpCode.
-WCN3990
-WCN3991
-WCN3998

< HCI Command: ogf 0x3f, ocf 0x0170, plen 1
  00
> HCI Event: 0x0e plen 18
  01 70 FD 00 00 1F 00 00 00 00 00 00 00 04 4D 53 46 54

The following test step was performed.
- Boot the device with WCN3991 and verify INFO print in dmesg.

Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

(no changes since v1)

 drivers/bluetooth/btqca.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index f85a55add9be5..ab19963c83616 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -517,6 +517,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 			return err;
 	}
 
+	/* WCN399x supports the Microsoft vendor extension with 0xFD70 as the
+	 * VsMsftOpCode.
+	 */
+	switch (soc_type) {
+	case QCA_WCN3990:
+	case QCA_WCN3991:
+	case QCA_WCN3998:
+		hci_set_msft_opcode(hdev, 0xFD70);
+		break;
+	default:
+		break;
+	}
+
 	/* Perform HCI reset */
 	err = qca_send_reset(hdev);
 	if (err < 0) {
-- 
2.29.2.684.gfbc64c5ab5-goog

Re: [PATCH v2 2/4] Bluetooth: btqca: Enable MSFT extension for Qualcomm WCN399x

[Marcel Holtmann]

Hi Miao-chen,

> The following Qualcomm WCN399x Bluetooth controllers support the
> Microsoft vendor extension and they are using 0xFD70 for VsMsftOpCode.
> -WCN3990
> -WCN3991
> -WCN3998
> 
> < HCI Command: ogf 0x3f, ocf 0x0170, plen 1
>  00
>> HCI Event: 0x0e plen 18
>  01 70 FD 00 00 1F 00 00 00 00 00 00 00 04 4D 53 46 54
> 
> The following test step was performed.
> - Boot the device with WCN3991 and verify INFO print in dmesg.
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> (no changes since v1)
> 
> drivers/bluetooth/btqca.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH v2 3/4] Bluetooth: btusb: Enable MSFT extension for Intel controllers

[Miao-chen Chou]

The Intel JeffersonPeak, HarrisonPeak and CyclonePeak Bluetooth
controllers support the Microsoft vendor extension and they are using
0xFC1E for VsMsftOpCode.

< HCI Command: Vendor (0x3f|0x001e) plen 1
        00
> HCI Event: Command Complete (0x0e) plen 15
      Vendor (0x3f|0x001e) ncmd 1
        Status: Success (0x00)
        00 3f 00 00 00 00 00 00 00 01 50

The following test step was performed.
- Boot the test devices with HarrisonPeak and verify INFO print in
dmesg.

Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

(no changes since v1)

 drivers/bluetooth/btusb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 03b83aa912779..25cfa47995a8a 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2924,7 +2924,10 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	 * extension are using 0xFC1E for VsMsftOpCode.
 	 */
 	switch (ver.hw_variant) {
+	case 0x11:	/* JfP */
 	case 0x12:	/* ThP */
+	case 0x13:	/* HrP */
+	case 0x14:	/* CcP */
 		hci_set_msft_opcode(hdev, 0xFC1E);
 		break;
 	}
-- 
2.29.2.684.gfbc64c5ab5-goog

Re: [PATCH v2 3/4] Bluetooth: btusb: Enable MSFT extension for Intel controllers

[Marcel Holtmann]

Hi Miao-chen,

> The Intel JeffersonPeak, HarrisonPeak and CyclonePeak Bluetooth
> controllers support the Microsoft vendor extension and they are using
> 0xFC1E for VsMsftOpCode.
> 
> < HCI Command: Vendor (0x3f|0x001e) plen 1
>        00
>> HCI Event: Command Complete (0x0e) plen 15
>      Vendor (0x3f|0x001e) ncmd 1
>        Status: Success (0x00)
>        00 3f 00 00 00 00 00 00 00 01 50
> 
> The following test step was performed.
> - Boot the test devices with HarrisonPeak and verify INFO print in
> dmesg.
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> (no changes since v1)
> 
> drivers/bluetooth/btusb.c | 3 +++
> 1 file changed, 3 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH v2 4/4] Bluetooth: btrtl: Enable MSFT extension for RTL8822CE controller

[Miao-chen Chou]

The Realtek RTL8822CE Bluetooth controller support Microsoft vendor
extension and it uses 0xFCF0 for VsMsftOpCode.

The following test step was performed.
- Boot the test device with RTL8822CE and verify the INFO print in
dmesg.

Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

(no changes since v1)

 drivers/bluetooth/btrtl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index a4f7cace66b06..94df4e94999d5 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -658,6 +658,12 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 		}
 	}
 
+	/* RTL8822CE supports the Microsoft vendor extension and uses 0xFCF0
+	 * for VsMsftOpCode.
+	 */
+	if (lmp_subver == RTL_ROM_LMP_8822B)
+		hci_set_msft_opcode(hdev, 0xFCF0);
+
 	return btrtl_dev;
 
 err_free:
-- 
2.29.2.684.gfbc64c5ab5-goog

Re: [PATCH v2 4/4] Bluetooth: btrtl: Enable MSFT extension for RTL8822CE controller

[Marcel Holtmann]

Hi Miao-chen,

> The Realtek RTL8822CE Bluetooth controller support Microsoft vendor
> extension and it uses 0xFCF0 for VsMsftOpCode.
> 
> The following test step was performed.
> - Boot the test device with RTL8822CE and verify the INFO print in
> dmesg.
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> (no changes since v1)
> 
> drivers/bluetooth/btrtl.c | 6 ++++++
> 1 file changed, 6 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

RE: [v2,1/4] Bluetooth: Keep MSFT ext info throughout a hci_dev's life cycle

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2716 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=403863

---Test result---

##############################
    Test: CheckPatch - PASS
    

    ##############################
    Test: CheckGitLint - FAIL
    workflow: Add workflow files for ci
1: T1 Title exceeds max length (92>72): "Merge f3ee8bc94ee111d1a4fecb87ca9bd8ffc11af840 into 6b1b2c912f1060a5a090407357f24d97490100ef"
3: B6 Body message is missing

Bluetooth: Keep MSFT ext info throughout a hci_dev's life cycle
1: T1 Title exceeds max length (92>72): "Merge f3ee8bc94ee111d1a4fecb87ca9bd8ffc11af840 into 6b1b2c912f1060a5a090407357f24d97490100ef"
3: B6 Body message is missing

Bluetooth: btqca: Enable MSFT extension for Qualcomm WCN399x
1: T1 Title exceeds max length (92>72): "Merge f3ee8bc94ee111d1a4fecb87ca9bd8ffc11af840 into 6b1b2c912f1060a5a090407357f24d97490100ef"
3: B6 Body message is missing

Bluetooth: btusb: Enable MSFT extension for Intel controllers
1: T1 Title exceeds max length (92>72): "Merge f3ee8bc94ee111d1a4fecb87ca9bd8ffc11af840 into 6b1b2c912f1060a5a090407357f24d97490100ef"
3: B6 Body message is missing

Bluetooth: btrtl: Enable MSFT extension for RTL8822CE controller
1: T1 Title exceeds max length (92>72): "Merge f3ee8bc94ee111d1a4fecb87ca9bd8ffc11af840 into 6b1b2c912f1060a5a090407357f24d97490100ef"
3: B6 Body message is missing


    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - FAIL
    Total: 416, Passed: 394 (94.7%), Failed: 8, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 43340 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3531 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 548205 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11651 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9886 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11797 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5427 bytes --]

Re: [PATCH v2 1/4] Bluetooth: Keep MSFT ext info throughout a hci_dev's life cycle

[Marcel Holtmann]

Hi Miao-chen,

> This moves msft_do_close() from hci_dev_do_close() to
> hci_unregister_dev() to avoid clearing MSFT extension info. This also
> avoids retrieving MSFT info upon every msft_do_open() if MSFT extension
> has been initialized.

what is the actual benefit of this?

It is fundamentally one extra HCI command and that one does no harm. You are trying to outsmart the hdev->setup vs the !hdev->setup case. I don’t think this is a good idea.

So unless I see a real argument why we want to do this, I am leaving this patch out. And on a side note, I named these function exactly this way so they are symmetric with hci_dev_do_{open,close}.

Regards

Marcel

Re: [PATCH v2 1/4] Bluetooth: Keep MSFT ext info throughout a hci_dev's life cycle

[Miao-chen Chou]

Hi Marcel,

On Fri, Dec 18, 2020 at 1:39 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Miao-chen,
>
> > This moves msft_do_close() from hci_dev_do_close() to
> > hci_unregister_dev() to avoid clearing MSFT extension info. This also
> > avoids retrieving MSFT info upon every msft_do_open() if MSFT extension
> > has been initialized.
>
> what is the actual benefit of this?
>
> It is fundamentally one extra HCI command and that one does no harm. You are trying to outsmart the hdev->setup vs the !hdev->setup case. I don’t think this is a good idea.
>
> So unless I see a real argument why we want to do this, I am leaving this patch out. And on a side note, I named these function exactly this way so they are symmetric with hci_dev_do_{open,close}.
>
> Regards
>
> Marcel
>
Thanks for pointing that out. I totally agree that it's not a wise
thing to outsmart the symmetric hci_dev_do_{open,close}. However, the
following two cases justify why we need this change.
(1) The current symmetric calls to msft_do{open,close} in
hci_dev_do_{open,close} cause incorrect MSFT features during
bluetoothd start-up. After the kernel powers on the controller to
register the hci_dev, it performs hci_dev_do_close() which call
msft_do_close() and MSFT data gets wiped out. And then during the
startup of bluetoothd, Adv Monitor Manager relies on reading the MSFT
features from the kernel to present the feature set of the controller
to D-Bus clients. However, the power state of the controller is off
during the init of D-Bus interfaces. As a result, invalid MSFT
features are returned by the kernel, since it was previously wiped out
due to hci_dev_do_close().
(2) Assuming bluetoothd has started, and users can be toggling the
power state of the adapter. Powering off the adapter invokes
hci_power_off()->hci_dev_do_close()->msft_do_close(), and MSFT
features get wiped out. During powered-off period, D-Bus client can
still add/remove monitor from the kernel, and the kernel needs to
issue corresponding MSFT HCI commands to the controller. However, the
MSFT opcode has been reset and invalid.

And here is the trace (for case 1 above) that I captured without this change.

2021-01-15T01:34:43.800155Z INFO kernel: [    2.754911] Bluetooth:
hci_power_on() @@ call hci_dev_do_open
2021-01-15T01:34:45.145025Z INFO kernel: [    4.272376] Bluetooth:
hci_dev_do_open() @@ call msft_do_open
2021-01-15T01:34:45.145050Z INFO kernel: [    4.272382] Bluetooth:
msft_do_open() @@
2021-01-15T01:34:45.146020Z INFO kernel: [    4.273139] Bluetooth:
read_supported_features() @@ features 000000000000003f
2021-01-15T01:34:47.176410Z INFO kernel: [    6.303439] Bluetooth:
hci_power_off() @@ call hci_dev_do_close
2021-01-15T01:34:47.189020Z INFO kernel: [    6.316152] Bluetooth:
hci_dev_do_close() @@ call msft_do_close
2021-01-15T01:34:47.189032Z INFO kernel: [    6.316158] Bluetooth:
msft_do_close() @@
2021-01-15T01:34:47.957401Z INFO bluetoothd[2591]: Bluetooth daemon 5.54
// skip some logs here
2021-01-15T01:34:48.004066Z INFO bluetoothd[2591]: Bluetooth
management interface 1.14 initialized
2021-01-15T01:34:48.167703Z INFO bluetoothd[2591]: @@ call
btd_adv_monitor_manager_create
2021-01-15T01:34:48.167832Z INFO bluetoothd[2591]: @@ call
MGMT_OP_READ_ADV_MONITOR_FEATURES
2021-01-15T01:34:48.167886Z INFO bluetoothd[2591]: Battery Provider
Manager created
2021-01-15T01:34:48.171924Z INFO bluetoothd[2591]: @@ features
supported_features 00000000 enabled_features 00000000
2021-01-15T01:34:48.172088Z INFO kernel: [    7.299305] Bluetooth:
hci_power_on() @@ call hci_dev_do_open
2021-01-15T01:34:48.172083Z INFO bluetoothd[2591]: Adv Monitor Manager
created with supported features:0x00000000, enabled
features:0x00000000, max number of supported monitors:32, max number
of supported patterns:16
2021-01-15T01:34:48.207800Z INFO bluetoothd[2591]: Endpoint
registered: sender=:1.52 path=/org/chromium/Cras/Bluetooth/A2DPSource
2021-01-15T01:34:48.212522Z INFO bluetoothd[2591]: Player registered:
sender=:1.52 path=/org/chromium/Cras/Bluetooth/DefaultPlayer
2021-01-15T01:34:48.214813Z INFO bluetoothd[2591]: BlueZ log level is set to 1
2021-01-15T01:34:48.230035Z INFO kernel: [    7.357118] Bluetooth:
hci_dev_do_open() @@ call msft_do_open
2021-01-15T01:34:48.230063Z INFO kernel: [    7.357124] Bluetooth:
msft_do_open() @@
2021-01-15T01:34:48.231027Z INFO kernel: [    7.358131] Bluetooth:
read_supported_features() @@ features 000000000000003f
2021-01-15T01:34:48.248967Z INFO bluetoothd[2591]: adapter
/org/bluez/hci0 has been enabled
2021-01-15T01:34:49.176198Z INFO bluetoothd[2591]: adapter
/org/bluez/hci0 set power to 1

Thanks,
Miao