Skip to contentSkip to conversation

Bluetooth: btintel: Fix spurious error on FW download error recovery

PATCH Superseded Ameen Al-Asady 3 participants 5 messages Jul 13 – Jul 14

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/2 Bluetooth: btintel: Fix spurious error on FW download error recovery

When firmware download fails because the controller is running an operational image whose version does not match the firmware file on disk (the normal case for a firmware upgrade across a warm reboot, since the operational image survives in controller RAM), btintel_reset_to_bootloader() sends the Intel Reset vendor command to force the controller back into bootloader mode for the download retry.

The controller acts on Intel Reset by resetting and re-enumerating on the bus, which can happen before the command complete event is delivered. In that case __hci_cmd_sync() fails with -ENODEV and the driver logs a misleading error even though recovery is proceeding exactly as intended:

Bluetooth: hci0: Found device firmware: intel/ibt-0041-0041.sfi
Bluetooth: hci0: Boot Address: 0x100800
Bluetooth: hci0: Firmware Version: 202-5.26
usb 1-6: USB disconnect, device number 4
Bluetooth: hci0: FW download error recovery failed (-19) usb 1-6: new full-speed USB device number 5 using xhci_hcd
...
Bluetooth: hci0: Firmware loaded in 2827613 usecs

Treat -ENODEV as the expected outcome of the reset rather than a recovery failure, and keep the error message for genuine failures.

---
 drivers/bluetooth/btintel.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/bluetooth/btintel.c +14 -4
index 5e9cac090..472542bff 100644
@@ -1363,12 +1363,22 @@ static void btintel_reset_to_bootloader(struct hci_dev *hdev)
 	skb = __hci_cmd_sync(hdev, BTINTEL_HCI_OP_RESET, sizeof(params),
 			     &params, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
-		bt_dev_err(hdev, "FW download error recovery failed (%ld)",
-			   PTR_ERR(skb));
-		return;
+		/* The controller acts on Intel Reset by resetting and
+		 * re-enumerating on the bus, which can happen before the
+		 * command complete event is delivered. The resulting
+		 * -ENODEV means the reset took effect, not that recovery
+		 * failed.
+		 */
+		if (PTR_ERR(skb) != -ENODEV) {
+			bt_dev_err(hdev,
+				   "FW download error recovery failed (%ld)",
+				   PTR_ERR(skb));
+			return;
+		}
+	} else {
+		kfree_skb(skb);
 	}
 	bt_dev_info(hdev, "Intel reset sent to retry FW download");
-	kfree_skb(skb);
 
 	/* Current Intel BT controllers(ThP/JfP) hold the USB reset
 	 * lines for 2ms when it receives Intel Reset in bootloader mode.

base-commit: eab19cb7618e5c50fd1f8c1efdc4e66e16575975

Signature · 1 line
2.55.0
#1 lore raw

PATCH 2/2 Bluetooth: hci_sync: Skip MSFT/AOSP init when controller init failed

hci_dev_init_sync() calls msft_do_open() and aosp_do_open() unconditionally, even when setup or the HCI init sequence has already failed. Both send vendor commands with __hci_cmd_sync(), so on a dead or uninitialized controller each of them blocks the power-on sequence for a full HCI_CMD_TIMEOUT and logs a spurious error.

This is easy to trigger with an Intel USB controller that needs a firmware upgrade: btintel resets the controller to bootloader mode, the device drops off the bus and hdev->setup() fails with -EINVAL, after which msft_do_open() still spends two more seconds waiting for a response that cannot arrive:

usb 1-6: USB disconnect, device number 4
Bluetooth: hci0: FW download error recovery failed (-19)
Bluetooth: hci0: Failed to read MSFT supported features (-110) usb 1-6: new full-speed USB device number 5 using xhci_hcd

Only initialize the MSFT and AOSP extensions when the controller initialized successfully.

---
 net/bluetooth/hci_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/hci_sync.c +1 -1
index b4ca34abe..38158835c 100644
@@ -5173,7 +5173,7 @@ static int hci_dev_init_sync(struct hci_dev *hdev)
 	    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)) {
+	if (!ret && !hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
 		msft_do_open(hdev);
 		aosp_do_open(hdev);
 	}
Signature · 1 line
2.55.0
#2 lore raw
In reply to Ameen Al-Asady

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=1126920

---Test result---

Test Summary:

CheckPatch                    PASS      1.72 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       PASS      0.57 seconds
SubjectPrefix                 PASS      0.21 seconds
BuildKernel                   PASS      25.29 seconds
CheckAllWarning               PASS      28.08 seconds
CheckSparse                   PASS      26.83 seconds
BuildKernel32                 PASS      25.24 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      456.79 seconds
TestRunner_l2cap-tester       PASS      57.69 seconds
TestRunner_iso-tester         PASS      80.99 seconds
TestRunner_bnep-tester        PASS      18.81 seconds
TestRunner_mgmt-tester        FAIL      206.36 seconds
TestRunner_rfcomm-tester      PASS      24.99 seconds
TestRunner_sco-tester         PASS      30.51 seconds
TestRunner_ioctl-tester       PASS      25.57 seconds
TestRunner_mesh-tester        FAIL      24.92 seconds
TestRunner_smp-tester         PASS      22.74 seconds
TestRunner_userchan-tester    PASS      19.71 seconds
TestRunner_6lowpan-tester     PASS      22.64 seconds
IncrementalBuild              PASS      27.18 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success Failed 0.237 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases

Mesh - Send cancel - 1                               Timed out    1.901 seconds
Mesh - Send cancel - 2                               Timed out    1.989 seconds

https://github.com/bluez/bluetooth-next/pull/432

---
Regards,
Linux Bluetooth

#3 lore raw
In reply to Ameen Al-Asady

Dear Ameen,

Thank you for your patch.

Am 13.07.26 um 23:55 schrieb Ameen Al-Asady:

Quoted 1 line

When firmware download fails because the controller is running an

“fail” is ambiguous to me. Maybe “errors out”.

Quoted 12 lines

operational image whose version does not match the firmware file on disk (the normal case for a firmware upgrade across a warm reboot, since the operational image survives in controller RAM), btintel_reset_to_bootloader() sends the Intel Reset vendor command to force the controller back into bootloader mode for the download retry.

The controller acts on Intel Reset by resetting and re-enumerating on the bus, which can happen before the command complete event is delivered. In that case __hci_cmd_sync() fails with -ENODEV and the driver logs a misleading error even though recovery is proceeding exactly as intended:

Quoted 8 lines

Bluetooth: hci0: Found device firmware: intel/ibt-0041-0041.sfi
Bluetooth: hci0: Boot Address: 0x100800
Bluetooth: hci0: Firmware Version: 202-5.26
usb 1-6: USB disconnect, device number 4
Bluetooth: hci0: FW download error recovery failed (-19) usb 1-6: new full-speed USB device number 5 using xhci_hcd
...
Bluetooth: hci0: Firmware loaded in 2827613 usecs

Any hint how to match the firmware versions, or add a sentence what firmware is on the controller and which one is downloaded?

Quoted 2 lines

Treat -ENODEV as the expected outcome of the reset rather than a recovery failure, and keep the error message for genuine failures.

Please mention the system you tested this on.

Quoted 23 lines
---
  drivers/bluetooth/btintel.c | 18 ++++++++++++++----
  1 file changed, 14 insertions(+), 4 deletions(-)
drivers/bluetooth/btintel.c +7 -3
index 5e9cac090..472542bff 100644
@@ -1363,12 +1363,22 @@ static void btintel_reset_to_bootloader(struct hci_dev *hdev)
  	skb = __hci_cmd_sync(hdev, BTINTEL_HCI_OP_RESET, sizeof(params),
  			     &params, HCI_INIT_TIMEOUT);
  	if (IS_ERR(skb)) {
-		bt_dev_err(hdev, "FW download error recovery failed (%ld)",
-			   PTR_ERR(skb));
-		return;
+		/* The controller acts on Intel Reset by resetting and
+		 * re-enumerating on the bus, which can happen before the
+		 * command complete event is delivered. The resulting
+		 * -ENODEV means the reset took effect, not that recovery
+		 * failed.
+		 */
+		if (PTR_ERR(skb) != -ENODEV) {

drivers/bluetooth/btintel.c static void btintel_reset_to_bootloader(struct hci_dev *hdev) · line 1371

Quoted 10 lines
drivers/bluetooth/btintel.c +7 -1
+			bt_dev_err(hdev,
+				   "FW download error recovery failed (%ld)",
+				   PTR_ERR(skb));
+			return;
+		}
+	} else {
+		kfree_skb(skb);
  	}
  	bt_dev_info(hdev, "Intel reset sent to retry FW download");
-	kfree_skb(skb);

Excuse my ignorance, but moving kfree_skb to a new else branch is not totally clear to me.

Quoted 5 lines
drivers/bluetooth/btintel.c +0 -0
  	/* Current Intel BT controllers(ThP/JfP) hold the USB reset
  	 * lines for 2ms when it receives Intel Reset in bootloader mode.
 

base-commit: eab19cb7618e5c50fd1f8c1efdc4e66e16575975

Kind regards,

Paul

#4 lore raw

Hi Paul,

Thanks for the review.

Quoted 1 line

"fail" is ambiguous to me. Maybe "errors out".

Reworded in v2.

Quoted 2 lines

Any hint how to match the firmware versions, or add a sentence what firmware is on the controller and which one is downloaded?

Added the firmware build lines to the log excerpt in v2. The controller was running build 81755 (2024.33) and the file on disk contained build 82122 (2026.5).

Quoted 1 line

Please mention the system you tested this on.

Done. Tested on an Intel AX210 (USB 8087:0032) on a 7.1.3 kernel by rebinding btusb with a non-matching firmware file on disk, in both the downgrade and upgrade direction. Both runs took the -ENODEV path, logged the info message instead of the error and completed the download after re-enumeration.

Quoted 2 lines

Excuse my ignorance, but moving kfree_skb to a new else branch is not totally clear to me.

When __hci_cmd_sync() fails it returns an error pointer instead of a real skb, so there is nothing to free. kfree_skb() must only run in the success case, which is now the else branch. The old code had the same behavior through the early return.

v2 sent.

Regards,
Ameen