Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230110155905.18203-1-kiran.k@intel.com>
In-Reply-To: <20230110155905.18203-1-kiran.k@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 10 Jan 2023 12:10:37 -0800
Message-ID: <CABBYNZLr6=OT93Znf7F92Ds9DEOBM9NLynyk4TadZQ1ueiRK7A@mail.gmail.com>
Subject: Re: [PATCH v1] btintel: Add recovery when secure verification of
 firmware fails
To:     Kiran K <kiran.k@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, ravishankar.srivatsa@intel.com,
        Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Kiran,

On Tue, Jan 10, 2023 at 7:52 AM Kiran K <kiran.k@intel.com> wrote:
>
> On warm reboot stress test case, firmware download failure
> has been observed with failure in secure verification of firmware
> and BT becomes completely inaccessible. This can happen due to a race
> condition in TOP registers when Wifi driver is also getting loaded
> at the same time. This patch adds a work around to load the BT firmware
> again when secure verify failure is observed.

In other words we can't trust the controller will be able to verify
the firmware?

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>
> ---
>  drivers/bluetooth/btintel.c | 20 ++++++++++++++++----
>  drivers/bluetooth/btintel.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index d4e2cb9a4eb4..3f2976fb056a 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1700,6 +1700,11 @@ static int btintel_download_wait(struct hci_dev *hdev, ktime_t calltime, int mse
>                 return -ETIMEDOUT;
>         }
>
> +       if (btintel_test_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED)) {
> +               bt_dev_err(hdev, "Firmware secure verification failed");
> +               return -EAGAIN;
> +       }
> +
>         if (btintel_test_flag(hdev, INTEL_FIRMWARE_FAILED)) {
>                 bt_dev_err(hdev, "Firmware loading failed");
>                 return -ENOEXEC;
> @@ -1961,7 +1966,7 @@ static int btintel_download_fw(struct hci_dev *hdev,
>          * of this device.
>          */
>         err = btintel_download_wait(hdev, calltime, 5000);
> -       if (err == -ETIMEDOUT)
> +       if (err == -ETIMEDOUT || err == -EAGAIN)
>                 btintel_reset_to_bootloader(hdev);
>
>  done:
> @@ -2153,7 +2158,7 @@ static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
>          * of this device.
>          */
>         err = btintel_download_wait(hdev, calltime, 5000);
> -       if (err == -ETIMEDOUT)
> +       if (err == -ETIMEDOUT || err == -EAGAIN)
>                 btintel_reset_to_bootloader(hdev);
>
>  done:
> @@ -2644,8 +2649,15 @@ void btintel_secure_send_result(struct hci_dev *hdev,
>         if (len != sizeof(*evt))
>                 return;
>
> -       if (evt->result)
> -               btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +       if (evt->result) {
> +               bt_dev_err(hdev, "Intel Secure Send Results event result: %u status: %u",
> +                          evt->result, evt->status);
> +
> +               if (evt->result == 3)
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED);

The result 3 is what exactly? If it returns the same value for both
the race condition and when the firmware is actually invalid that
means the later will cause a look since we don't have any counter of
how many times we would be attempting to reload the firmware, so we
better limit the number of times we attempt to reload e.g. 1-2 times
at most, or we have the firmware provide a different result when it
busy loading the wifi side.

> +               else
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +       }
>
>         if (btintel_test_and_clear_flag(hdev, INTEL_DOWNLOADING) &&
>             btintel_test_flag(hdev, INTEL_FIRMWARE_LOADED))
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index e0060e58573c..10e5be7e451a 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -147,6 +147,7 @@ enum {
>         INTEL_BOOTLOADER,
>         INTEL_DOWNLOADING,
>         INTEL_FIRMWARE_LOADED,
> +       INTEL_FIRMWARE_VERIFY_FAILED,
>         INTEL_FIRMWARE_FAILED,
>         INTEL_BOOTING,
>         INTEL_BROKEN_INITIAL_NCMD,
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz