| Message ID | 20210121163801.v3.1.Id9bc5434114de07512661f002cdc0ada8b3d6d02@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Bluetooth: Keep MSFT ext info throughout ahci_dev's life cycle | expand |
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=419567
---Test result---
##############################
Test: CheckPatch - PASS
##############################
Test: CheckGitLint - FAIL
workflow: Add workflow files for ci
1: T1 Title exceeds max length (92>72): "Merge cddac0be8fc86f5424b98bd35a2e56a44f2d7618 into 435ccd2045513af89a4eb03a6b7399a021fc0258"
3: B6 Body message is missing
Bluetooth: Keep MSFT ext info throughout ahci_dev's life cycle
1: T1 Title exceeds max length (92>72): "Merge cddac0be8fc86f5424b98bd35a2e56a44f2d7618 into 435ccd2045513af89a4eb03a6b7399a021fc0258"
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 - PASS
Total: 416, Passed: 402 (96.6%), Failed: 0, 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
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
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
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");