Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220405215207.1415731-1-luiz.dentz@gmail.com>
In-Reply-To: <20220405215207.1415731-1-luiz.dentz@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 21 Apr 2022 14:51:37 -0700
Message-ID: <CABBYNZKAjfPY_g+OirbPFaxCRf4VDjxyxjG+gFEPFxRJ7P==Cw@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: hci_sync: Split hci_dev_open_sync
To:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Cc:     Marcel Holtmann <marcel@holtmann.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel,

On Tue, Apr 5, 2022 at 2:52 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This splits hci_dev_open_sync so each stage is handle by its own
> function so it is easier to identify each stage.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/hci_sync.c | 255 ++++++++++++++++++++++-----------------
>  1 file changed, 141 insertions(+), 114 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 5610ec1242d6..2d3b9adbd215 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3849,9 +3849,148 @@ static const struct {
>                          "advertised, but not supported.")
>  };
>
> -int hci_dev_open_sync(struct hci_dev *hdev)
> +/* This function handles hdev setup stage:
> + *
> + * Calls hdev->setup
> + * Setup address if HCI_QUIRK_USE_BDADDR_PROPERTY is set.
> + */
> +static int hci_dev_setup_sync(struct hci_dev *hdev)
>  {
>         int ret = 0;
> +       bool invalid_bdaddr;
> +       size_t i;
> +
> +       if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
> +           !test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks))
> +               return 0;
> +
> +       bt_dev_dbg(hdev, "");
> +
> +       hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> +
> +       if (hdev->setup)
> +               ret = hdev->setup(hdev);
> +
> +       for (i = 0; i < ARRAY_SIZE(hci_broken_table); i++) {
> +               if (test_bit(hci_broken_table[i].quirk, &hdev->quirks))
> +                       bt_dev_warn(hdev, "%s", hci_broken_table[i].desc);
> +       }
> +
> +       /* The transport driver can set the quirk to mark the
> +        * BD_ADDR invalid before creating the HCI device or in
> +        * its setup callback.
> +        */
> +       invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
> +
> +       if (!ret) {
> +               if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
> +                       if (!bacmp(&hdev->public_addr, BDADDR_ANY))
> +                               hci_dev_get_bd_addr_from_property(hdev);
> +
> +                       if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
> +                           hdev->set_bdaddr) {
> +                               ret = hdev->set_bdaddr(hdev,
> +                                                      &hdev->public_addr);
> +
> +                               /* If setting of the BD_ADDR from the device
> +                                * property succeeds, then treat the address
> +                                * as valid even if the invalid BD_ADDR
> +                                * quirk indicates otherwise.
> +                                */
> +                               if (!ret)
> +                                       invalid_bdaddr = false;
> +                       }
> +               }
> +       }
> +
> +       /* The transport driver can set these quirks before
> +        * creating the HCI device or in its setup callback.
> +        *
> +        * For the invalid BD_ADDR quirk it is possible that
> +        * it becomes a valid address if the bootloader does
> +        * provide it (see above).
> +        *
> +        * In case any of them is set, the controller has to
> +        * start up as unconfigured.
> +        */
> +       if (test_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks) ||
> +           invalid_bdaddr)
> +               hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
> +
> +       /* For an unconfigured controller it is required to
> +        * read at least the version information provided by
> +        * the Read Local Version Information command.
> +        *
> +        * If the set_bdaddr driver callback is provided, then
> +        * also the original Bluetooth public device address
> +        * will be read using the Read BD Address command.
> +        */
> +       if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
> +               return hci_unconf_init_sync(hdev);
> +
> +       return ret;
> +}
> +
> +/* This function handles hdev init stage:
> + *
> + * Calls hci_dev_setup_sync to perform setup stage
> + * Calls hci_init_sync to perform HCI command init sequence
> + */
> +static int hci_dev_init_sync(struct hci_dev *hdev)
> +{
> +       int ret;
> +
> +       bt_dev_dbg(hdev, "");
> +
> +       atomic_set(&hdev->cmd_cnt, 1);
> +       set_bit(HCI_INIT, &hdev->flags);
> +
> +       ret = hci_dev_setup_sync(hdev);
> +
> +       if (hci_dev_test_flag(hdev, HCI_CONFIG)) {
> +               /* If public address change is configured, ensure that
> +                * the address gets programmed. If the driver does not
> +                * support changing the public address, fail the power
> +                * on procedure.
> +                */
> +               if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
> +                   hdev->set_bdaddr)
> +                       ret = hdev->set_bdaddr(hdev, &hdev->public_addr);
> +               else
> +                       ret = -EADDRNOTAVAIL;
> +       }
> +
> +       if (!ret) {
> +               if (!hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&
> +                   !hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> +                       ret = hci_init_sync(hdev);
> +                       if (!ret && hdev->post_init)
> +                               ret = hdev->post_init(hdev);
> +               }
> +       }
> +
> +       /* If the HCI Reset command is clearing all diagnostic settings,
> +        * then they need to be reprogrammed after the init procedure
> +        * completed.
> +        */
> +       if (test_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks) &&
> +           !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> +           hci_dev_test_flag(hdev, HCI_VENDOR_DIAG) && hdev->set_diag)
> +               ret = hdev->set_diag(hdev, true);
> +
> +       if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> +               msft_do_open(hdev);
> +               aosp_do_open(hdev);
> +       }
> +
> +       clear_bit(HCI_INIT, &hdev->flags);
> +
> +       return ret;
> +}
> +
> +int hci_dev_open_sync(struct hci_dev *hdev)
> +{
> +       int ret;
>
>         bt_dev_dbg(hdev, "");
>
> @@ -3904,119 +4043,7 @@ int hci_dev_open_sync(struct hci_dev *hdev)
>         set_bit(HCI_RUNNING, &hdev->flags);
>         hci_sock_dev_event(hdev, HCI_DEV_OPEN);
>
> -       atomic_set(&hdev->cmd_cnt, 1);
> -       set_bit(HCI_INIT, &hdev->flags);
> -
> -       if (hci_dev_test_flag(hdev, HCI_SETUP) ||
> -           test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> -               bool invalid_bdaddr;
> -               size_t i;
> -
> -               hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> -
> -               if (hdev->setup)
> -                       ret = hdev->setup(hdev);
> -
> -               for (i = 0; i < ARRAY_SIZE(hci_broken_table); i++) {
> -                       if (test_bit(hci_broken_table[i].quirk, &hdev->quirks))
> -                               bt_dev_warn(hdev, "%s",
> -                                           hci_broken_table[i].desc);
> -               }
> -
> -               /* The transport driver can set the quirk to mark the
> -                * BD_ADDR invalid before creating the HCI device or in
> -                * its setup callback.
> -                */
> -               invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR,
> -                                         &hdev->quirks);
> -
> -               if (ret)
> -                       goto setup_failed;
> -
> -               if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
> -                       if (!bacmp(&hdev->public_addr, BDADDR_ANY))
> -                               hci_dev_get_bd_addr_from_property(hdev);
> -
> -                       if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
> -                           hdev->set_bdaddr) {
> -                               ret = hdev->set_bdaddr(hdev,
> -                                                      &hdev->public_addr);
> -
> -                               /* If setting of the BD_ADDR from the device
> -                                * property succeeds, then treat the address
> -                                * as valid even if the invalid BD_ADDR
> -                                * quirk indicates otherwise.
> -                                */
> -                               if (!ret)
> -                                       invalid_bdaddr = false;
> -                       }
> -               }
> -
> -setup_failed:
> -               /* The transport driver can set these quirks before
> -                * creating the HCI device or in its setup callback.
> -                *
> -                * For the invalid BD_ADDR quirk it is possible that
> -                * it becomes a valid address if the bootloader does
> -                * provide it (see above).
> -                *
> -                * In case any of them is set, the controller has to
> -                * start up as unconfigured.
> -                */
> -               if (test_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks) ||
> -                   invalid_bdaddr)
> -                       hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
> -
> -               /* For an unconfigured controller it is required to
> -                * read at least the version information provided by
> -                * the Read Local Version Information command.
> -                *
> -                * If the set_bdaddr driver callback is provided, then
> -                * also the original Bluetooth public device address
> -                * will be read using the Read BD Address command.
> -                */
> -               if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
> -                       ret = hci_unconf_init_sync(hdev);
> -       }
> -
> -       if (hci_dev_test_flag(hdev, HCI_CONFIG)) {
> -               /* If public address change is configured, ensure that
> -                * the address gets programmed. If the driver does not
> -                * support changing the public address, fail the power
> -                * on procedure.
> -                */
> -               if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
> -                   hdev->set_bdaddr)
> -                       ret = hdev->set_bdaddr(hdev, &hdev->public_addr);
> -               else
> -                       ret = -EADDRNOTAVAIL;
> -       }
> -
> -       if (!ret) {
> -               if (!hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&
> -                   !hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> -                       ret = hci_init_sync(hdev);
> -                       if (!ret && hdev->post_init)
> -                               ret = hdev->post_init(hdev);
> -               }
> -       }
> -
> -       /* If the HCI Reset command is clearing all diagnostic settings,
> -        * then they need to be reprogrammed after the init procedure
> -        * completed.
> -        */
> -       if (test_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks) &&
> -           !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> -           hci_dev_test_flag(hdev, HCI_VENDOR_DIAG) && hdev->set_diag)
> -               ret = hdev->set_diag(hdev, true);
> -
> -       if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> -               msft_do_open(hdev);
> -               aosp_do_open(hdev);
> -       }
> -
> -       clear_bit(HCI_INIT, &hdev->flags);
> -
> +       ret = hci_dev_init_sync(hdev);
>         if (!ret) {
>                 hci_dev_hold(hdev);
>                 hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
> --
> 2.35.1

Any feedback on these changes?

-- 
Luiz Augusto von Dentz