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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > Reviewed-by: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > ---
> > 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