| Message ID | 20210121163801.v3.1.Id9bc5434114de07512661f002cdc0ada8b3d6d02@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Bluetooth: Keep MSFT ext info throughout ahci_dev's life cycle | expand |
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");