Re: [PATCH v1] btintel: Add recovery when secure verification of firmware fails

[Tedd Ho-Jeong An <tedd.an@linux.intel.com>]

Hi Kiran

On Tue, 2023-01-10 at 21:29 +0530, Kiran K 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.
> 
> 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)
Please avoid any magic number here. You can define it in btintel.h.
As Luiz suggested, let's try to add the reloading counter in btintel_data to limit the reloading try.
 
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED);
> +               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,

Regards,
Tedd