[PATCH v1] Bluetooth: btintel_pcie: serialize reset_type with RECOVERY_IN_PROGRESS

[PATCH v1] Bluetooth: btintel_pcie: serialize reset_type with RECOVERY_IN_PROGRESS

[Kiran K]

The reset path had two concurrency holes. Both are reachable in
practice when btintel_pcie_hw_error() is invoked from the HCI rx
path while another reset is being requested or is already in
flight.

  1. data->reset_type was a plain shared field. The hw_error path
     wrote it BEFORE the test_and_set_bit(RECOVERY_IN_PROGRESS)
     guard inside btintel_pcie_reset(), so a second hw_error could
     clobber the type chosen by an earlier in-flight request:

       CPU0 (reset_work)             CPU1 (hw_error #2)
                                     dev_data->reset_type = PLDR
       T2: read reset_type
                                     dev_data->reset_type = FLR
                                     reset() test_and_set sees 1
                                     -> drops, but type already
                                        clobbered

     The HCI reset path (.reset = btintel_pcie_reset, used by
     HCIDEVRESET / `hciconfig hciX reset`) compounded this by not
     writing reset_type at all -- it inherited whatever value a
     previous hw_error / resume() had left, which could be PLDR.

  2. btintel_pcie_dump_debug_registers() was called unconditionally
     at the top of hw_error(). When reset_work was already running
     pci_try_reset_function(), the BT MMIO window can read all-1s
     or trigger AER for the duration of the FLR, polluting the
     debug dump with no useful information.

Refactor the reset path to make RECOVERY_IN_PROGRESS the sole
serializer for both the type write and the work scheduling:

  - Replace btintel_pcie_reset(hdev) with
    btintel_pcie_request_reset(data, type). The helper takes the
    desired reset variant as a parameter and writes
    data->reset_type only after winning test_and_set_bit(); losers
    return without touching the field, so concurrent triggers can
    no longer clobber an in-flight reset's type. reset_work()'s
    read of reset_type is now ordered after the bit transition via
    schedule_work()'s memory barrier.

  - Add a thin btintel_pcie_hci_reset() wrapper for the
    hdev->reset callback that always requests FLR explicitly. The
    HCIDEVRESET path no longer inherits stale state from prior
    error events.

  - Add an early test_bit(RECOVERY_IN_PROGRESS) gate at the top of
    hw_error() so dump_debug_registers() and the recovery-counter
    bookkeeping are skipped when a reset is already in flight; the
    authoritative test_and_set lives in request_reset() and races
    cleanly against any caller that passes the optimistic check.

  - Convert the two resume() reset sites (FREEZE/HIBERNATE and the
    D0-error path) to request_reset(data, FLR), removing the
    redundant manual reset_type writes.

No new locks are introduced; the fix relies on the existing
RECOVERY_IN_PROGRESS bit transition for ordering.

Assisted-by: GitHub-Copilot:claude-4.7-opus
Signed-off-by: Kiran K <kiran.k@intel.com>
---
 drivers/bluetooth/btintel_pcie.c | 88 +++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 24 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 013568197a39..2a10476bae4b 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -2550,8 +2550,6 @@ static void btintel_pcie_inc_recovery_count(struct pci_dev *pdev,
 	}
 }
 
-static void btintel_pcie_reset(struct hci_dev *hdev);
-
 static int btintel_pcie_acpi_reset_method(struct btintel_pcie_data *data)
 {
 	union acpi_object *obj, argv4;
@@ -2749,56 +2747,99 @@ static void btintel_pcie_reset_work(struct work_struct *wk)
 	pci_unlock_rescan_remove();
 }
 
-static void btintel_pcie_reset(struct hci_dev *hdev)
+/* Schedule a device reset of the requested type.
+ *
+ * BTINTEL_PCIE_RECOVERY_IN_PROGRESS serializes all reset requesters
+ * (HCI ioctl, hw_error, resume error path, etc.) so that:
+ *
+ *   - dev_data->reset_type is written by exactly one caller (the
+ *     thread that wins test_and_set_bit), eliminating the race where
+ *     a second hw_error could clobber an already-scheduled reset's
+ *     type;
+ *   - the write happens AFTER the bit is set, so reset_work observes
+ *     it through schedule_work()'s memory ordering;
+ *   - losers return without touching reset_type or scheduling the
+ *     work, so concurrent triggers are silently coalesced into the
+ *     in-flight one (whose recovery will reinitialize the device
+ *     regardless of the dropped trigger's variant).
+ *
+ * The bit is cleared only by .remove() / re-probe via fresh devm
+ * allocation, which is the intended one-shot semantics: a reset
+ * tears down and re-probes 'data', so there is no "in-flight"
+ * reset to follow up after device_reprobe() succeeds.
+ */
+static void btintel_pcie_request_reset(struct btintel_pcie_data *data,
+				       enum btintel_pcie_reset_type type)
 {
-	struct btintel_pcie_data *data;
-
-	data = hci_get_drvdata(hdev);
-
 	if (!test_bit(BTINTEL_PCIE_SETUP_DONE, &data->flags))
 		return;
 
 	if (test_and_set_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &data->flags))
 		return;
 
+	data->reset_type = type;
+
 	pci_dev_get(data->pdev);
 	schedule_work(&data->reset_work);
 }
 
+/* hdev->reset callback (HCIDEVRESET ioctl, e.g. `hciconfig hciX reset`).
+ *
+ * User-triggered resets always use FLR; PLDR is reserved for the
+ * hw_error path (controller-reported fatal exceptions, code 0x13).
+ */
+static void btintel_pcie_hci_reset(struct hci_dev *hdev)
+{
+	struct btintel_pcie_data *data = hci_get_drvdata(hdev);
+
+	btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);
+}
+
 static void btintel_pcie_hw_error(struct hci_dev *hdev, u8 code)
 {
-	struct btintel_pcie_dev_recovery *data;
+	struct btintel_pcie_dev_recovery *rec;
 	struct btintel_pcie_data *dev_data = hci_get_drvdata(hdev);
 	struct pci_dev *pdev = dev_data->pdev;
+	enum btintel_pcie_reset_type type;
 	time64_t retry_window;
 
+	/* Cheap pre-check: if a reset is already in flight, skip MMIO
+	 * reads in dump_debug_registers() and the recovery-counter
+	 * bookkeeping entirely. The PCI core may be mid-FLR (BME / config
+	 * window torn down by pci_try_reset_function()), in which case
+	 * those reads return all-1s or trigger AER. The authoritative
+	 * guard is the test_and_set_bit() inside
+	 * btintel_pcie_request_reset(), which races cleanly against
+	 * concurrent triggers that pass this check.
+	 */
+	if (test_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &dev_data->flags))
+		return;
+
 	btintel_pcie_dump_debug_registers(hdev);
 
-	data = btintel_pcie_get_recovery(pdev, &hdev->dev);
-	if (!data)
+	rec = btintel_pcie_get_recovery(pdev, &hdev->dev);
+	if (!rec)
 		return;
 
-	if (code == 0x13)
-		dev_data->reset_type = BTINTEL_PCIE_IOSF_PRR_PLDR;
-	else
-		dev_data->reset_type = BTINTEL_PCIE_IOSF_PRR_FLR;
+	type = (code == 0x13) ? BTINTEL_PCIE_IOSF_PRR_PLDR
+			      : BTINTEL_PCIE_IOSF_PRR_FLR;
 
 	bt_dev_err(hdev, "Encountered exception err:0x%x triggering: %s", code,
-		   dev_data->reset_type == BTINTEL_PCIE_IOSF_PRR_PLDR ? "PLDR" : "FLR");
-	retry_window = ktime_get_boottime_seconds() - data->last_error;
+		   type == BTINTEL_PCIE_IOSF_PRR_PLDR ? "PLDR" : "FLR");
+	retry_window = ktime_get_boottime_seconds() - rec->last_error;
 
 	if (retry_window < BTINTEL_PCIE_RESET_WINDOW_SECS &&
-	    data->count >= BTINTEL_PCIE_FLR_MAX_RETRY) {
+	    rec->count >= BTINTEL_PCIE_FLR_MAX_RETRY) {
 		bt_dev_err(hdev, "Exhausted maximum: %d recovery attempts: %d",
-			   BTINTEL_PCIE_FLR_MAX_RETRY, data->count);
+			   BTINTEL_PCIE_FLR_MAX_RETRY, rec->count);
 		bt_dev_dbg(hdev, "Boot time: %lld seconds",
 			   ktime_get_boottime_seconds());
 		bt_dev_dbg(hdev, "last error at: %lld seconds",
-			   data->last_error);
+			   rec->last_error);
 		return;
 	}
 	btintel_pcie_inc_recovery_count(pdev, &hdev->dev);
-	btintel_pcie_reset(hdev);
+	btintel_pcie_request_reset(dev_data, type);
 }
 
 static bool btintel_pcie_wakeup(struct hci_dev *hdev)
@@ -2888,7 +2929,7 @@ static int btintel_pcie_setup_hdev(struct btintel_pcie_data *data)
 	hdev->hw_error = btintel_pcie_hw_error;
 	hdev->set_diag = btintel_set_diag;
 	hdev->set_bdaddr = btintel_set_bdaddr;
-	hdev->reset = btintel_pcie_reset;
+	hdev->reset = btintel_pcie_hci_reset;
 	hdev->wakeup = btintel_pcie_wakeup;
 	hdev->hci_drv = &btintel_pcie_hci_drv;
 
@@ -3176,8 +3217,7 @@ static int btintel_pcie_resume(struct device *dev)
 	if (data->pm_sx_event == PM_EVENT_FREEZE ||
 	    data->pm_sx_event == PM_EVENT_HIBERNATE) {
 		set_bit(BTINTEL_PCIE_CORE_HALTED, &data->flags);
-		data->reset_type = BTINTEL_PCIE_IOSF_PRR_FLR;
-		btintel_pcie_reset(data->hdev);
+		btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);
 		return 0;
 	}
 
@@ -3204,7 +3244,7 @@ static int btintel_pcie_resume(struct device *dev)
 		btintel_pcie_queue_coredump(data,
 					    BTINTEL_PCIE_TRIGGER_REASON_FW_ASSERT);
 		set_bit(BTINTEL_PCIE_CORE_HALTED, &data->flags);
-		btintel_pcie_reset(data->hdev);
+		btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);
 	}
 	return err;
 }
-- 
2.54.0

RE: [v1] Bluetooth: btintel_pcie: serialize reset_type with RECOVERY_IN_PROGRESS

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.72 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       PASS      0.28 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      26.11 seconds
CheckAllWarning               PASS      29.03 seconds
CheckSparse                   PASS      27.02 seconds
BuildKernel32                 PASS      24.83 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      464.89 seconds
IncrementalBuild              PASS      24.91 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


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

---
Regards,
Linux Bluetooth