* [PATCH v1] Bluetooth: btintel_pcie: serialize reset_type with RECOVERY_IN_PROGRESS
@ 2026-07-14 14:24 Kiran K
2026-07-14 14:48 ` [v1] " bluez.test.bot
0 siblings, 1 reply; 2+ messages in thread
From: Kiran K @ 2026-07-14 14:24 UTC (permalink / raw)
To: linux-bluetooth
Cc: ravishankar.srivatsa, chethan.tumkur.narayan,
chandrashekar.devegowda, 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
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-07-14 14:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-14 14:24 [PATCH v1] Bluetooth: btintel_pcie: serialize reset_type with RECOVERY_IN_PROGRESS Kiran K
2026-07-14 14:48 ` [v1] " bluez.test.bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox