[PATCH v3] Bluetooth: Keep MSFT ext info throughout ahci_dev's life cycle

[PATCH v3] Bluetooth: Keep MSFT ext info throughout ahci_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>
---
Hi Maintainers,

This patch fixes the life cycle of MSFT HCI extension. 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().

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

Regards,
Chrome OS Bluetooth team

Changes in v3:
- Remove the accepted commits from the series

 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 eeafed2efc0da..8056f0d4ae172 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1764,8 +1764,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);
 
@@ -3844,6 +3842,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 8579bfeb28364..34769898858ef 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -75,7 +75,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.30.0.280.ga3ce27912f-goog

Re: [PATCH v3] Bluetooth: Keep MSFT ext info throughout ahci_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.
> 
> 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>
> ---
> Hi Maintainers,
> 
> This patch fixes the life cycle of MSFT HCI extension. 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().

then just keep the values around and not wipe them. However I prefer still to keep the symmetry and re-read the value every time we init. We can make sure to release the msft_data on unregister.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Keep MSFT ext info throughout ahci_dev's life cycle

[Miao-chen Chou]

Hi Marcel,

On Mon, Jan 25, 2021 at 7:13 AM 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.
> >
> > 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>
> > ---
> > Hi Maintainers,
> >
> > This patch fixes the life cycle of MSFT HCI extension. 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().
>
> then just keep the values around and not wipe them. However I prefer still to keep the symmetry and re-read the value every time we init. We can make sure to release the msft_data on unregister.
This patch does exactly what you described - keep the values around
and not wipe them until unregistration of hdev. Since the only thing
that msft_do_close() does is to release msft_data and reset
hdev->msft_data it to NULL, and that's why I move msft_do_close() from
 hci_dev_do_close() to hci_unregister_dev() to release the msft_data.
If this is about naming, I am happy to change msft_do_close() to
perhaps msft_reset() or something similar. As for msft_do_open(), I
will change it to re-read the msft_data instead of skipping.

Regards,
Miao