Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210121163801.v3.1.Id9bc5434114de07512661f002cdc0ada8b3d6d02@changeid>
 <070E7413-A3E3-4FEB-80BC-D3DD922DA19B@holtmann.org>
In-Reply-To: <070E7413-A3E3-4FEB-80BC-D3DD922DA19B@holtmann.org>
From:   Miao-chen Chou <mcchou@chromium.org>
Date:   Mon, 25 Jan 2021 14:18:22 -0800
Message-ID: <CABmPvSEtpmz5oK2ZshRm0+HhoLL6rig92v9d63ocpy6m7MvUdw@mail.gmail.com>
Subject: Re: [PATCH v3] Bluetooth: Keep MSFT ext info throughout ahci_dev's
 life cycle
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Alain Michaud <alainm@chromium.org>,
        Archie Pusaka <apusaka@chromium.org>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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