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 <[email protected]>
Signed-off-by: Chethan Tumkur Narayan <[email protected]>
---
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);
+ 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
Adding An, Tedd in CC list.
> -----Original Message-----
> From: K, Kiran <[email protected]>
> Sent: Tuesday, January 10, 2023 9:29 PM
> To: [email protected]
> Cc: Srivatsa, Ravishankar <[email protected]>; K, Kiran
> <[email protected]>; Tumkur Narayan, Chethan
> <[email protected]>
> Subject: [PATCH v1] btintel: Add recovery when secure verification of
> firmware fails
>
> 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 <[email protected]>
> Signed-off-by: Chethan Tumkur Narayan
> <[email protected]>
> ---
> 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);
> + 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
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=710575
---Test result---
Test Summary:
CheckPatch PASS 0.90 seconds
GitLint PASS 0.35 seconds
SubjectPrefix FAIL 0.39 seconds
BuildKernel PASS 32.29 seconds
CheckAllWarning PASS 35.00 seconds
CheckSparse PASS 39.88 seconds
CheckSmatch PASS 107.37 seconds
BuildKernel32 PASS 30.46 seconds
TestRunnerSetup PASS 441.80 seconds
TestRunner_l2cap-tester PASS 16.27 seconds
TestRunner_iso-tester PASS 17.18 seconds
TestRunner_bnep-tester PASS 5.80 seconds
TestRunner_mgmt-tester PASS 110.21 seconds
TestRunner_rfcomm-tester PASS 9.07 seconds
TestRunner_sco-tester PASS 8.33 seconds
TestRunner_ioctl-tester PASS 9.87 seconds
TestRunner_mesh-tester PASS 7.24 seconds
TestRunner_smp-tester PASS 8.26 seconds
TestRunner_userchan-tester PASS 6.08 seconds
IncrementalBuild PASS 28.91 seconds
Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
---
Regards,
Linux Bluetooth
Hi Kiran,
On Tue, Jan 10, 2023 at 7:52 AM Kiran K <[email protected]> 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 <[email protected]>
> Signed-off-by: Chethan Tumkur Narayan <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Chethan Tumkur Narayan <[email protected]>
> ---
> 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