[PATCH 0/2] Bluetooth: btintel_pcie: two PM fixes, assembled as one series

[PATCH 0/2] Bluetooth: btintel_pcie: two PM fixes, assembled as one series

[Sergey Lebedev]

Two patches already on this list fix different halves of the same fault, and
each leaves a real failure behind when applied alone. This assembles them into
one series. I am the submitter only - authorship, Fixes: tags and existing
trailers are unchanged.

  1/2  Vladimir V. Kondratyev  - re-read BOOT_STAGE_REG before the fallback
                                 check, so a missed alive interrupt is
                                 survivable
  2/2  Ravindra (Intel)        - fix the PM flow for S0ix, S3 and S4, which
                                 among other things stops .thaw running an FLR

Both carry Fixes: e57362f4911b.

How this came about, so nobody has to take my word for it. Ravindra agreed to
the assembly on 2026-09-08, on the condition that authorship and the Fixes:
tags be preserved and that his patch be rebased on top of Vladimir's; both are
done:

  https://lore.kernel.org/linux-bluetooth/IA1PR11MB786922E0DBE2CDC8FB00C5B6DAB12@IA1PR11MB7869.namprd11.prod.outlook.com/

Vladimir has not replied to the message that said this would go out unless he
objected, and I am not treating that silence as agreement. Vladimir - it is
your patch. If you would rather post the series yourself, or not at all, say
so and I will drop this:

  https://lore.kernel.org/linux-bluetooth/20260908102758.72135-1-lsa.uz@pm.me/

Why one series
==============

They were posted separately and read as alternatives. They are not. The
evidence now comes from two machines, two controller generations, and two
distinct faults:

Surface Pro 11, Lunar Lake, BE201 8086:a876, s2idle. With a fixture that
drops the alive interrupt on demand, Ravindra's change alone leaves the
missed-interrupt case failing exactly as unpatched; Vladimir's re-read fixes
it; together they do not interfere. The same failure was also caught
spontaneously with injection disabled, one run in six.

  https://lore.kernel.org/linux-bluetooth/20260902133836.11786-1-lsa.uz@pm.me/

ThinkPad X9-15p, Panther Lake 8086:e476, hibernation. Ferenc Lengyel reported
hibernation aborting with -EBUSY after the image was already written, and
tested both patches:

  stock                    3 aborts in 10 cycles
  1/2 alone                2 aborts in 5
  1/2 + 2/2                0 aborts in 10

He then instrumented 1/2's re-read and found why 1/2 alone is not enough on
his machine: the register honestly reports the controller is not in D3,
because .thaw has just run an FLR that dropped it to ROM, and firmware has
not reloaded by the time .poweroff asks for D3 some nine seconds later. 2/2
removes that FLR from .thaw and keeps it on .restore. His own caveat, which
he states himself: ten clean cycles against a roughly one-in-three prior
failure rate is Fisher p ~ 0.06 - consistent and matching the mechanism,
not a large sample.

  https://lore.kernel.org/linux-bluetooth/df180a89-b214-41b4-b8ce-c6ed6b12372f@lengyelf.eu/

What the rebase changed
=======================

Nothing but context. 2/2 needed one hand adjustment: its header hunk removes
u8 pm_sx_event from struct btintel_pcie_data, and bluetooth-next has since
gained struct btintel_pcie_mdbgc mdbgc between dbgc and dmp_hdr, so the
three-line context no longer matched. The resulting diffstat is identical to
Ravindra's posting - 64 lines in the .c, 2 in the .h, 44 insertions and 22
deletions - and no reference to pm_sx_event is left anywhere in
drivers/bluetooth.

Built against bluetooth-next at 701ca7188 with W=1: no warnings from either
file. The code is otherwise byte-for-byte what was tested on both machines.

One thing for Paul: you gave a Reviewed-by on v3 of 1/2 at 14:36 UTC on
2026-09-03 and v4 went out at 19:22 without carrying it. I have not added it
back, since it is not mine to move, but you may want to re-give it here.

Originals:
  1/2  https://lore.kernel.org/linux-bluetooth/20260903192245.135310-2-vladimirkondratyev2@gmail.com/
  2/2  https://lore.kernel.org/linux-bluetooth/20260902042840.2432862-1-ravindra@intel.com/

Ravindra (1):
  Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

Vladimir V. Kondratyev (1):
  Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

 drivers/bluetooth/btintel_pcie.c | 72 ++++++++++++++++++++++----------
 drivers/bluetooth/btintel_pcie.h |  3 +-
 2 files changed, 50 insertions(+), 25 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH 1/2] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

[Sergey Lebedev]

From: Vladimir V. Kondratyev <vladimirkondratyev2@gmail.com>

btintel_pcie returns -16 (EBUSY) during suspend, causing the entire
suspend operation to abort on Intel Lunar Lake hardware. The system
immediately resumes
after every suspend attempt:
  Bluetooth: hci0: Timeout (200 ms) on alive interrupt for D2 entry,
retry count 0
  Bluetooth: hci0: Timeout (200 ms) on alive interrupt for D2 entry,
retry count 1
  Bluetooth: hci0: Timeout (200 ms) on alive interrupt for D2 entry,
retry count 2
  btintel_pcie 0000:00:14.7: PM: pci_pm_suspend(): btintel_pcie_suspend
[btintel_pcie] returns -16
  btintel_pcie 0000:00:14.7: PM: dpm_run_callback(): pci_pm_suspend
returns -16
  btintel_pcie 0000:00:14.7: PM: failed to suspend async: error -16
  PM: Some devices failed to suspend, or early wake event detected

btintel_pcie_set_dxstate() falls back to checking the controller state via
btintel_pcie_in_d3/d0() when the alive interrupt is missed. However, these
helpers read boot_stage_cache, which is only updated by the interrupt
handler. As such, if the interrupt was missed, the cache is stale and the
fallback check always fails, exhausting all retries and returning -EBUSY,
causing suspend to abort.

The fix involves re-reading the hardware register before the fallback state
check, consistent with btintel_pcie_resume().

Fixes: e57362f4911b ("Bluetooth: btintel_pcie: Add support for _suspend() / _resume()")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=221481
Link: https://lore.kernel.org/linux-bluetooth/20260830151550.44687-1-lsa.uz@pm.me/
Signed-off-by: Vladimir V. Kondratyev <vladimirkondratyev2@gmail.com>
Tested-by: Sergey Lebedev <lsa.uz@pm.me>

Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
---
 drivers/bluetooth/btintel_pcie.c | 8 +++++---
 drivers/bluetooth/btintel_pcie.h | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 7a2139a04..361c550b5 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -4191,10 +4191,12 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
 					  BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
 					  BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
 
-		/* A hardware bug may cause the alive interrupt to be missed.
-		 * Check if the controller reached the expected state and retry
-		 * the operation only if it hasn't.
+		/* A hardware bug may cause the alive interrupt to be missed. Refresh
+		 * boot_stage_cache from hardware, since only the interrupt handler
+		 * updates it. Finally retry only if the state check still fails.
 		 */
+		data->boot_stage_cache = btintel_pcie_rd_reg32(data,
+							       BTINTEL_PCIE_CSR_BOOT_STAGE_REG);
 		if (dxstate == BTINTEL_PCIE_STATE_D0) {
 			if (btintel_pcie_in_d0(data))
 				return 0;
diff --git a/drivers/bluetooth/btintel_pcie.h b/drivers/bluetooth/btintel_pcie.h
index f35f80f80..016795fcb 100644
--- a/drivers/bluetooth/btintel_pcie.h
+++ b/drivers/bluetooth/btintel_pcie.h
@@ -51,6 +51,7 @@
 #define BTINTEL_PCIE_CSR_BOOT_STAGE_DEVICE_HALTED	(BIT(14))
 #define BTINTEL_PCIE_CSR_BOOT_STAGE_MAC_ACCESS_ON	(BIT(16))
 #define BTINTEL_PCIE_CSR_BOOT_STAGE_ALIVE		(BIT(23))
+/* Reflects live D-state. Updated by hardware on every D-state transition. */
 #define BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY	(BIT(24))
 
 #define BTINTEL_PCIE_CSR_DOORBELL_MBOX_READ_CONFIRM	(BIT(4))
-- 
2.50.1 (Apple Git-155)

[PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Sergey Lebedev]

From: Ravindra <ravindra@intel.com>

Use pm_suspend_target_state to differentiate S0ix from S3/S4. Set the
controller to D3_HOT for S0ix (PM_SUSPEND_TO_IDLE) and D3_COLD for
S3/S4, freeze and hibernate to prevent post-resume instability.

Register .freeze, .thaw, .poweroff and .restore callbacks for proper
hibernation support. The freeze and poweroff (hibernate) paths set
D3_COLD via btintel_pcie_suspend_late. The thaw path resumes with a
normal D0 transition, while .restore forces FLR-based firmware recovery
after S4 since power is lost. S3 (PM_SUSPEND_MEM) resume also triggers
FLR. S0ix resumes via a normal D0 transition.

Fixes: e57362f4911b ("Bluetooth: btintel_pcie: Add support for _suspend() / _resume()")
Assisted-by: GitHub-Copilot:GPT5
Signed-off-by: Ravindra <ravindra@intel.com>
Assisted-by: Claude:claude-opus-5
Tested-by: Ferenc Lengyel <dev@lengyelf.eu>
Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
---
 drivers/bluetooth/btintel_pcie.c | 64 ++++++++++++++++++++++----------
 drivers/bluetooth/btintel_pcie.h |  2 -
 2 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 361c550b5..e37e710b9 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -16,6 +16,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 
 #include <linux/unaligned.h>
 #include <linux/devcoredump.h>
@@ -4168,11 +4169,16 @@ static void btintel_pcie_coredump(struct device *dev)
 
 static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
 {
-	int retry = 0, status;
+	int retry = 0;
+	long status;
 	u32 dx_intr_timeout_ms = 200;
 
+	/* Not reset per retry: dxstate is unchanged, so a late interrupt from
+	 * an earlier attempt still confirms the target state.
+	 */
+	data->gp0_received = false;
+
 	do {
-		data->gp0_received = false;
 
 		btintel_pcie_wr_sleep_cntrl(data, dxstate);
 
@@ -4220,18 +4226,23 @@ static int btintel_pcie_suspend_late(struct device *dev, pm_message_t mesg)
 
 	data = pci_get_drvdata(pdev);
 
-	dxstate = (mesg.event == PM_EVENT_SUSPEND ?
-		   BTINTEL_PCIE_STATE_D3_HOT : BTINTEL_PCIE_STATE_D3_COLD);
-
-	data->pm_sx_event = mesg.event;
+	/* S0ix (s2idle) uses D3_HOT; S3, freeze and hibernate use D3_COLD. */
+	if (mesg.event == PM_EVENT_SUSPEND &&
+	    pm_suspend_target_state == PM_SUSPEND_TO_IDLE)
+		dxstate = BTINTEL_PCIE_STATE_D3_HOT;
+	else
+		dxstate = BTINTEL_PCIE_STATE_D3_COLD;
 
 	start = ktime_get();
 
 	/* Refer: 6.4.11.7 -> Platform power management */
 	err = btintel_pcie_set_dxstate(data, dxstate);
 
-	if (err)
+	if (err) {
+		bt_dev_err(data->hdev, "Failed to set dxstate:%u (%d)",
+			   dxstate, err);
 		return err;
+	}
 
 	bt_dev_dbg(data->hdev,
 		   "device entered into d3 state from d0 in %lld us",
@@ -4254,7 +4265,7 @@ static int btintel_pcie_freeze(struct device *dev)
 	return btintel_pcie_suspend_late(dev, PMSG_FREEZE);
 }
 
-static int btintel_pcie_resume(struct device *dev)
+static int btintel_pcie_resume_event(struct device *dev, pm_message_t mesg)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct btintel_pcie_data *data;
@@ -4262,19 +4273,15 @@ static int btintel_pcie_resume(struct device *dev)
 	int err;
 
 	data = pci_get_drvdata(pdev);
-	data->gp0_received = false;
 
 	start = ktime_get();
 
-	/* When the system enters S4 (hibernate) mode, bluetooth device loses
-	 * power, which results in the erasure of its loaded firmware.
-	 * Consequently, function level reset (flr) is required on system
-	 * resume to bring the controller back into an operational state by
-	 * initiating a new firmware download.
+	/* S3 and S4 may cut power, erasing the firmware. Force FLR to recover
+	 * instead of a normal D0 transition.
 	 */
-
-	if (data->pm_sx_event == PM_EVENT_FREEZE ||
-	    data->pm_sx_event == PM_EVENT_HIBERNATE) {
+	if (mesg.event == PM_EVENT_RESTORE ||
+	    (mesg.event == PM_EVENT_RESUME &&
+	     pm_suspend_target_state == PM_SUSPEND_MEM)) {
 		set_bit(BTINTEL_PCIE_CORE_HALTED, &data->flags);
 		btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);
 		return 0;
@@ -4283,7 +4290,9 @@ static int btintel_pcie_resume(struct device *dev)
 	/* Refer: 6.4.11.7 -> Platform power management */
 	err = btintel_pcie_set_dxstate(data, BTINTEL_PCIE_STATE_D0);
 
-	if (err == 0) {
+	if (err) {
+		bt_dev_err(data->hdev, "Failed to set D0 state (%d)", err);
+	} else {
 		bt_dev_dbg(data->hdev,
 			   "device entered into d0 state from d3 in %lld us",
 			   ktime_to_us(ktime_get() - start));
@@ -4308,13 +4317,28 @@ static int btintel_pcie_resume(struct device *dev)
 	return err;
 }
 
+static int btintel_pcie_resume(struct device *dev)
+{
+	return btintel_pcie_resume_event(dev, PMSG_RESUME);
+}
+
+static int btintel_pcie_restore(struct device *dev)
+{
+	return btintel_pcie_resume_event(dev, PMSG_RESTORE);
+}
+
+static int btintel_pcie_thaw(struct device *dev)
+{
+	return btintel_pcie_resume_event(dev, PMSG_THAW);
+}
+
 static const struct dev_pm_ops btintel_pcie_pm_ops = {
 	.suspend = btintel_pcie_suspend,
 	.resume = btintel_pcie_resume,
 	.freeze = btintel_pcie_freeze,
-	.thaw = btintel_pcie_resume,
+	.thaw = btintel_pcie_thaw,
 	.poweroff = btintel_pcie_hibernate,
-	.restore = btintel_pcie_resume,
+	.restore = btintel_pcie_restore,
 };
 
 static struct pci_driver btintel_pcie_driver = {
diff --git a/drivers/bluetooth/btintel_pcie.h b/drivers/bluetooth/btintel_pcie.h
index 016795fcb..3030b4e8b 100644
--- a/drivers/bluetooth/btintel_pcie.h
+++ b/drivers/bluetooth/btintel_pcie.h
@@ -713,7 +713,6 @@ struct btintel_pcie_ini_dump_info {
  * @txq: TX Queue struct
  * @rxq: RX Queue struct
  * @alive_intr_ctxt: Alive interrupt context
- * @pm_sx_event: PM event on which system got suspended
  */
 struct btintel_pcie_data {
 	struct pci_dev	*pdev;
@@ -773,7 +772,6 @@ struct btintel_pcie_data {
 	struct btintel_pcie_dbgc	dbgc;
 	struct btintel_pcie_mdbgc	mdbgc;
 	struct btintel_pcie_dump_header dmp_hdr;
-	u8	pm_sx_event;
 	u32	debug_evt_addr;
 	u32	debug_evt_size;
 	dma_addr_t	debug_table_addr;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Luiz Augusto von Dentz]

Hi Sergey,

On Wed, Sep 9, 2026 at 8:34 AM Sergey Lebedev <lsa.uz@pm.me> wrote:
>
> From: Ravindra <ravindra@intel.com>
>
> Use pm_suspend_target_state to differentiate S0ix from S3/S4. Set the
> controller to D3_HOT for S0ix (PM_SUSPEND_TO_IDLE) and D3_COLD for
> S3/S4, freeze and hibernate to prevent post-resume instability.
>
> Register .freeze, .thaw, .poweroff and .restore callbacks for proper
> hibernation support. The freeze and poweroff (hibernate) paths set
> D3_COLD via btintel_pcie_suspend_late. The thaw path resumes with a
> normal D0 transition, while .restore forces FLR-based firmware recovery
> after S4 since power is lost. S3 (PM_SUSPEND_MEM) resume also triggers
> FLR. S0ix resumes via a normal D0 transition.
>
> Fixes: e57362f4911b ("Bluetooth: btintel_pcie: Add support for _suspend() / _resume()")
> Assisted-by: GitHub-Copilot:GPT5
> Signed-off-by: Ravindra <ravindra@intel.com>
> Assisted-by: Claude:claude-opus-5
> Tested-by: Ferenc Lengyel <dev@lengyelf.eu>
> Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
> ---
>  drivers/bluetooth/btintel_pcie.c | 64 ++++++++++++++++++++++----------
>  drivers/bluetooth/btintel_pcie.h |  2 -
>  2 files changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 361c550b5..e37e710b9 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -16,6 +16,7 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/acpi.h>
> +#include <linux/suspend.h>
>
>  #include <linux/unaligned.h>
>  #include <linux/devcoredump.h>
> @@ -4168,11 +4169,16 @@ static void btintel_pcie_coredump(struct device *dev)
>
>  static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
>  {
> -       int retry = 0, status;
> +       int retry = 0;
> +       long status;
>         u32 dx_intr_timeout_ms = 200;
>
> +       /* Not reset per retry: dxstate is unchanged, so a late interrupt from
> +        * an earlier attempt still confirms the target state.
> +        */
> +       data->gp0_received = false;
> +
>         do {
> -               data->gp0_received = false;
>
>                 btintel_pcie_wr_sleep_cntrl(data, dxstate);
>
> @@ -4220,18 +4226,23 @@ static int btintel_pcie_suspend_late(struct device *dev, pm_message_t mesg)
>
>         data = pci_get_drvdata(pdev);
>
> -       dxstate = (mesg.event == PM_EVENT_SUSPEND ?
> -                  BTINTEL_PCIE_STATE_D3_HOT : BTINTEL_PCIE_STATE_D3_COLD);
> -
> -       data->pm_sx_event = mesg.event;
> +       /* S0ix (s2idle) uses D3_HOT; S3, freeze and hibernate use D3_COLD. */
> +       if (mesg.event == PM_EVENT_SUSPEND &&
> +           pm_suspend_target_state == PM_SUSPEND_TO_IDLE)
> +               dxstate = BTINTEL_PCIE_STATE_D3_HOT;
> +       else
> +               dxstate = BTINTEL_PCIE_STATE_D3_COLD;
>
>         start = ktime_get();
>
>         /* Refer: 6.4.11.7 -> Platform power management */
>         err = btintel_pcie_set_dxstate(data, dxstate);
>
> -       if (err)
> +       if (err) {
> +               bt_dev_err(data->hdev, "Failed to set dxstate:%u (%d)",
> +                          dxstate, err);
>                 return err;
> +       }
>
>         bt_dev_dbg(data->hdev,
>                    "device entered into d3 state from d0 in %lld us",
> @@ -4254,7 +4265,7 @@ static int btintel_pcie_freeze(struct device *dev)
>         return btintel_pcie_suspend_late(dev, PMSG_FREEZE);
>  }
>
> -static int btintel_pcie_resume(struct device *dev)
> +static int btintel_pcie_resume_event(struct device *dev, pm_message_t mesg)
>  {
>         struct pci_dev *pdev = to_pci_dev(dev);
>         struct btintel_pcie_data *data;
> @@ -4262,19 +4273,15 @@ static int btintel_pcie_resume(struct device *dev)
>         int err;
>
>         data = pci_get_drvdata(pdev);
> -       data->gp0_received = false;
>
>         start = ktime_get();
>
> -       /* When the system enters S4 (hibernate) mode, bluetooth device loses
> -        * power, which results in the erasure of its loaded firmware.
> -        * Consequently, function level reset (flr) is required on system
> -        * resume to bring the controller back into an operational state by
> -        * initiating a new firmware download.
> +       /* S3 and S4 may cut power, erasing the firmware. Force FLR to recover
> +        * instead of a normal D0 transition.
>          */
> -
> -       if (data->pm_sx_event == PM_EVENT_FREEZE ||
> -           data->pm_sx_event == PM_EVENT_HIBERNATE) {
> +       if (mesg.event == PM_EVENT_RESTORE ||
> +           (mesg.event == PM_EVENT_RESUME &&
> +            pm_suspend_target_state == PM_SUSPEND_MEM)) {
>                 set_bit(BTINTEL_PCIE_CORE_HALTED, &data->flags);
>                 btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);
>                 return 0;
> @@ -4283,7 +4290,9 @@ static int btintel_pcie_resume(struct device *dev)
>         /* Refer: 6.4.11.7 -> Platform power management */
>         err = btintel_pcie_set_dxstate(data, BTINTEL_PCIE_STATE_D0);
>
> -       if (err == 0) {
> +       if (err) {
> +               bt_dev_err(data->hdev, "Failed to set D0 state (%d)", err);
> +       } else {
>                 bt_dev_dbg(data->hdev,
>                            "device entered into d0 state from d3 in %lld us",
>                            ktime_to_us(ktime_get() - start));
> @@ -4308,13 +4317,28 @@ static int btintel_pcie_resume(struct device *dev)
>         return err;
>  }
>
> +static int btintel_pcie_resume(struct device *dev)
> +{
> +       return btintel_pcie_resume_event(dev, PMSG_RESUME);
> +}
> +
> +static int btintel_pcie_restore(struct device *dev)
> +{
> +       return btintel_pcie_resume_event(dev, PMSG_RESTORE);
> +}
> +
> +static int btintel_pcie_thaw(struct device *dev)
> +{
> +       return btintel_pcie_resume_event(dev, PMSG_THAW);
> +}
> +
>  static const struct dev_pm_ops btintel_pcie_pm_ops = {
>         .suspend = btintel_pcie_suspend,
>         .resume = btintel_pcie_resume,
>         .freeze = btintel_pcie_freeze,
> -       .thaw = btintel_pcie_resume,
> +       .thaw = btintel_pcie_thaw,
>         .poweroff = btintel_pcie_hibernate,
> -       .restore = btintel_pcie_resume,
> +       .restore = btintel_pcie_restore,
>  };
>
>  static struct pci_driver btintel_pcie_driver = {
> diff --git a/drivers/bluetooth/btintel_pcie.h b/drivers/bluetooth/btintel_pcie.h
> index 016795fcb..3030b4e8b 100644
> --- a/drivers/bluetooth/btintel_pcie.h
> +++ b/drivers/bluetooth/btintel_pcie.h
> @@ -713,7 +713,6 @@ struct btintel_pcie_ini_dump_info {
>   * @txq: TX Queue struct
>   * @rxq: RX Queue struct
>   * @alive_intr_ctxt: Alive interrupt context
> - * @pm_sx_event: PM event on which system got suspended
>   */
>  struct btintel_pcie_data {
>         struct pci_dev  *pdev;
> @@ -773,7 +772,6 @@ struct btintel_pcie_data {
>         struct btintel_pcie_dbgc        dbgc;
>         struct btintel_pcie_mdbgc       mdbgc;
>         struct btintel_pcie_dump_header dmp_hdr;
> -       u8      pm_sx_event;
>         u32     debug_evt_addr;
>         u32     debug_evt_size;
>         dma_addr_t      debug_table_addr;
> --
> 2.50.1 (Apple Git-155)

Sashiko flagged a few things:

https://sashiko.dev/#/patchset/20260909123416.71919-1-lsa.uz%40pm.me

-- 
Luiz Augusto von Dentz

Consent for Assembly

[yCduIhFgkD]

Hi Sergey,

Sorry for the late response.

I consent to the assembling the series.
Also thanks for the extensive testing! You're really rad!

Regards,
Vladimir

Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Sergey Lebedev]

Thank you for the pointer. That review reached no list at all: not
linux-bluetooth, not devicetree, and not sashiko's own sashiko-reviews list,
which I checked over 31 August to 9 September. It exists only on the site, so
without you going to look we would not have known it was there.

One of its three findings is real, reproducible, and worse than it claims. I
have measured it rather than argued about it, and the fix is below. The other
two are at the end, more briefly.

A missed alive interrupt now costs the controller, not just the suspend
=======================================================================

1/2 makes set_dxstate() return success when the register says the target state
was reached but the interrupt never arrived. That is correct about the
hardware and silent about data->alive_intr_ctxt, which only the interrupt
handler ever moves. So the tracker is left saying D0 after a suspend that
actually reached D3.

On resume the handler then runs with a stale D0 context while the hardware is
already heading to D0, takes neither branch, and sets neither signal_waitq nor
submit_rx. btintel_pcie_reset_ia() and btintel_pcie_start_rx() are the only
things that re-arm the RX rings, and nothing else on the resume path calls
them.

Measured with the same fixture as my 2 September matrix - one alive interrupt
dropped inside the handler, before it touches anything, which is the state a
genuinely missed one leaves:

  no injection      SP11RX: ctxt d3 -> d0, submit_rx=1     device unchanged
  interrupt dropped SP11RX: ctxt d0 -> d0, submit_rx=0     ...then:

    Bluetooth: hci0: Received hw exception interrupt
    Bluetooth: hci0: command 0x0c01 tx timeout
    Bluetooth: hci0: Opcode 0x0c1a failed: -110
    btintel_pcie 0000:00:14.7: resetting

and the controller comes back as a new hci index. So it is not only that RX
stops: the firmware throws an exception, two HCI commands time out, the driver
FLRs it, and every paired device is gone until something re-pairs.

The fix
=======

Do in the fallback what the handler's branch would have done. It mirrors the
handler's own call site, which also ignores start_rx()'s return:

@@ -4204,11 +4204,25 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
 		if (dxstate == BTINTEL_PCIE_STATE_D0) {
-			if (btintel_pcie_in_d0(data))
+			if (btintel_pcie_in_d0(data)) {
+				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
+				btintel_pcie_reset_ia(data);
+				btintel_pcie_start_rx(data);
 				return 0;
+			}
 		} else {
-			if (btintel_pcie_in_d3(data))
+			if (btintel_pcie_in_d3(data)) {
+				data->alive_intr_ctxt = BTINTEL_PCIE_D3;
 				return 0;
+			}
 		}

Both halves are needed, and I only know that because setting the context alone
looked like a complete fix until I dropped the interrupt on the way up
instead:

  build                     drop on D3 entry        drop on D0 entry
  as posted                 wedged, FLR, new hci    -
  context only              clean                   wedged, FLR, new hci
  context + RX re-arm       clean, 3 of 3           clean, 3 of 3

Three cycles of each plus three controls: no "hw exception" and no "resetting"
in any of the nine, and the hci index never moved. One caveat about my own
instrument: I also counted HCI events during a scan after each resume, and one
*control* run returned zero with the device plainly healthy, so that counter is
not trustworthy on its own. The exception and reset lines are what never
misfired.

What I propose to do
====================

Fold it into 1/2 and send the series as v2. My reasoning is that a fix for an
unmerged patch in the same series belongs inside it rather than on top, but I
hold that loosely and a separate patch is just as easy if you prefer it for
review.

It changes Vladimir's logic rather than adding to it, so: Vladimir, say if you
would rather carry it yourself and I will hold. Otherwise I will send v2 in a
day or two, unless Luiz would rather see it sooner.

The second finding, which I could not measure
=============================================

Moving data->gp0_received = false out of the retry loop means a late interrupt
from attempt N can satisfy wait_event_timeout() at the top of attempt N+1, and
"if (status) return 0;" has no hardware check behind it - the register re-read
1/2 adds sits on the timeout path only. So the function can report success
having just written wr_sleep_cntrl() and waited for nothing, right after the
previous iteration read the hardware and found it *not* in the target state.

Real by reading, but unmeasured: my fixture drops interrupts and does not delay
them, so I cannot produce a late one. Saying so rather than implying I tested
it.

The third, which is pre-existing and not this series'
=====================================================

set_dxstate() clears the GP0 cause with btintel_pcie_clr_reg_bits(), which is
a read-modify-write. BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES looks
write-1-to-clear: the interrupt handler acknowledges it by writing back exactly
what it read. If so the call does the opposite of both halves of its job - it
writes 0 to GP0, which clears nothing, and 1 to whatever else was pending in
that register, retiring HWEXP, GP1 or FWTRIG unserviced.

Neither patch touches that line, so it is not this series' business, but
someone at Intel may want it.

Thanks,
Sergey

RE: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Ravindra]

Hi Sergey,

Thanks for the detailed investigation and write-up.

I agree with the issue arising from not updating the alive interrupt
context, as well as the incorrect (read-modify-write) clear of the GP0
cause bit. I've folded both fixes into the PM-flow patch as a single
commit and tested 50 suspend/resume (S4) cycles on an NVL Linux platform -
no issues seen across all 50 runs.

I'm fine either way on how this lands: I can push the combined patch
myself, or if you'd prefer to carry it and push it upstream directly,
that works for me too.

Diff:

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 1816c81c4721..9d4f0bdf992b 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -5193,21 +5193,35 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
                "Timeout (%u ms) on alive interrupt for D%d entry, retry count %d",
                dx_intr_timeout_ms, dxstate, retry);
 
-		/* clear gp0 cause */
-		btintel_pcie_clr_reg_bits(data,
-					  BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
-					  BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
+		/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so write only
+		 * this bit to avoid acking other pending causes
+		 */
+		btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
+				      BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
 
-		/* A hardware bug may cause the alive interrupt to be missed.
-		 * Check if the controller reached the expected state and retry
-		 * the operation only if it hasn't.
+		/* A hardware bug may cause the alive interrupt to be missed. Refresh
+		 * boot_stage_cache from hardware, since only the interrupt handler
+		 * updates it. Finally retry only if the state check still fails.
          */
+		data->boot_stage_cache = btintel_pcie_rd_reg32(data,
+				BTINTEL_PCIE_CSR_BOOT_STAGE_REG);
+
         if (dxstate == BTINTEL_PCIE_STATE_D0) {
-			if (btintel_pcie_in_d0(data))
+			if (btintel_pcie_in_d0(data)) {
+				/* GP0 handler never ran to do this: keep the
+				 * state tracker in sync and resubmit RX.
+				 */
+				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
+				btintel_pcie_reset_ia(data);
+				btintel_pcie_start_rx(data);
                 return 0;
+			}
         } else {
-			if (btintel_pcie_in_d3(data))
+			if (btintel_pcie_in_d3(data)) {
+				/* GP0 handler never ran to do this. */
+				data->alive_intr_ctxt = BTINTEL_PCIE_D3;
                 return 0;
+			}
         }
 
     } while (++retry < BTINTEL_PCIE_DX_TRANSITION_MAX_RETRIES);

Per Paul's comments, I've also updated the commit message:

Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

Fix two issues in the PM suspend/resume path:

1. S3 was handled the same as S0ix, keeping the controller on the
   D3hot-style path. That caused resume instability because S3 can
   remove power from the controller, unlike s2idle/S0ix. Use
   pm_suspend_target_state to distinguish them: D3_HOT for S0ix and
   D3_COLD for S3/S4. Add .restore to force FLR-based firmware recovery
   after S4 and S3 (PM_SUSPEND_MEM), as power is lost. S0ix resumes via
   a normal D0 transition.

2. During hibernation, .freeze() puts the controller into the D3cold
   state without any loss of power, and the flow normally continues to
   .poweroff(). If hibernation instead fails, .thaw() is called to bring
   the controller back up, and the old code routed it through
   btintel_pcie_resume(), which forced FLR-based firmware recovery
   whenever data->pm_sx_event was PM_EVENT_FREEZE. That check was
   incorrect: since the controller's power was never actually removed on
   this failed-hibernation path, FLR-based recovery is unnecessary. Remove
   pm_sx_event and route .thaw through a normal D0 transition instead;
   FLR-based recovery is retained only in .restore, where genuine S4
   power loss requires it.

The Sx debug logging (debug_mask & BTINTEL_PCIE_LOG_SX) is kept and
adapted to use pm_message_t.event instead of the removed pm_sx_event
field.

Tested with:
  S0ix: sudo sh -c 'echo "+40" > /sys/class/rtc/rtc0/wakealarm' && \
        echo freeze | sudo tee /sys/power/state
  S3:   sudo rtcwake -m mem  -s 60
  S4:   sudo rtcwake -m disk -s 60

Fixes: e57362f4911b ("Bluetooth: btintel_pcie: Add support for _suspend() / _resume()")
Signed-off-by: Ravindra <ravindra@intel.com>

Thanks,
Ravindra
> Thank you for the pointer. That review reached no list at all: not linux-
> bluetooth, not devicetree, and not sashiko's own sashiko-reviews list, which I
> checked over 31 August to 9 September. It exists only on the site, so without
> you going to look we would not have known it was there.
> 
> One of its three findings is real, reproducible, and worse than it claims. I have
> measured it rather than argued about it, and the fix is below. The other two
> are at the end, more briefly.
> 
> A missed alive interrupt now costs the controller, not just the suspend
> ================================================================
> =======
> 
> 1/2 makes set_dxstate() return success when the register says the target state
> was reached but the interrupt never arrived. That is correct about the
> hardware and silent about data->alive_intr_ctxt, which only the interrupt
> handler ever moves. So the tracker is left saying D0 after a suspend that
> actually reached D3.
> 
> On resume the handler then runs with a stale D0 context while the hardware
> is already heading to D0, takes neither branch, and sets neither signal_waitq
> nor submit_rx. btintel_pcie_reset_ia() and btintel_pcie_start_rx() are the only
> things that re-arm the RX rings, and nothing else on the resume path calls
> them.
> 
> Measured with the same fixture as my 2 September matrix - one alive
> interrupt dropped inside the handler, before it touches anything, which is the
> state a genuinely missed one leaves:
> 
>   no injection      SP11RX: ctxt d3 -> d0, submit_rx=1     device unchanged
>   interrupt dropped SP11RX: ctxt d0 -> d0, submit_rx=0     ...then:
> 
>     Bluetooth: hci0: Received hw exception interrupt
>     Bluetooth: hci0: command 0x0c01 tx timeout
>     Bluetooth: hci0: Opcode 0x0c1a failed: -110
>     btintel_pcie 0000:00:14.7: resetting
> 
> and the controller comes back as a new hci index. So it is not only that RX
> stops: the firmware throws an exception, two HCI commands time out, the
> driver FLRs it, and every paired device is gone until something re-pairs.
> 
> The fix
> =======
> 
> Do in the fallback what the handler's branch would have done. It mirrors the
> handler's own call site, which also ignores start_rx()'s return:
> 
> @@ -4204,11 +4204,25 @@ static int btintel_pcie_set_dxstate(struct
> btintel_pcie_data *data, u32 dxstate)
>  		if (dxstate == BTINTEL_PCIE_STATE_D0) {
> -			if (btintel_pcie_in_d0(data))
> +			if (btintel_pcie_in_d0(data)) {
> +				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
> +				btintel_pcie_reset_ia(data);
> +				btintel_pcie_start_rx(data);
>  				return 0;
> +			}
>  		} else {
> -			if (btintel_pcie_in_d3(data))
> +			if (btintel_pcie_in_d3(data)) {
> +				data->alive_intr_ctxt = BTINTEL_PCIE_D3;
>  				return 0;
> +			}
>  		}
> 
> Both halves are needed, and I only know that because setting the context
> alone looked like a complete fix until I dropped the interrupt on the way up
> instead:
> 
>   build                     drop on D3 entry        drop on D0 entry
>   as posted                 wedged, FLR, new hci    -
>   context only              clean                   wedged, FLR, new hci
>   context + RX re-arm       clean, 3 of 3           clean, 3 of 3
> 
> Three cycles of each plus three controls: no "hw exception" and no "resetting"
> in any of the nine, and the hci index never moved. One caveat about my own
> instrument: I also counted HCI events during a scan after each resume, and
> one
> *control* run returned zero with the device plainly healthy, so that counter is
> not trustworthy on its own. The exception and reset lines are what never
> misfired.
> 
> What I propose to do
> ====================
> 
> Fold it into 1/2 and send the series as v2. My reasoning is that a fix for an
> unmerged patch in the same series belongs inside it rather than on top, but I
> hold that loosely and a separate patch is just as easy if you prefer it for review.
> 
> It changes Vladimir's logic rather than adding to it, so: Vladimir, say if you
> would rather carry it yourself and I will hold. Otherwise I will send v2 in a day
> or two, unless Luiz would rather see it sooner.
> 
> The second finding, which I could not measure
> =============================================
> 
> Moving data->gp0_received = false out of the retry loop means a late
> interrupt from attempt N can satisfy wait_event_timeout() at the top of
> attempt N+1, and "if (status) return 0;" has no hardware check behind it - the
> register re-read
> 1/2 adds sits on the timeout path only. So the function can report success
> having just written wr_sleep_cntrl() and waited for nothing, right after the
> previous iteration read the hardware and found it *not* in the target state.
> 
> Real by reading, but unmeasured: my fixture drops interrupts and does not
> delay them, so I cannot produce a late one. Saying so rather than implying I
> tested it.
> 
> The third, which is pre-existing and not this series'
> =====================================================
> 
> set_dxstate() clears the GP0 cause with btintel_pcie_clr_reg_bits(), which is a
> read-modify-write. BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES looks
> write-1-to-clear: the interrupt handler acknowledges it by writing back exactly
> what it read. If so the call does the opposite of both halves of its job - it
> writes 0 to GP0, which clears nothing, and 1 to whatever else was pending in
> that register, retiring HWEXP, GP1 or FWTRIG unserviced.
> 
> Neither patch touches that line, so it is not this series' business, but someone
> at Intel may want it.
> 
> Thanks,
> Sergey

Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Sergey Lebedev]

Thank you - and for taking the third finding, which I had written off as
not this series' business.

Where this stands, and what I need from you
===========================================

A v3 is assembled and ready: 1/2 and 2/2 unchanged, plus a third patch for
the one path the fold does not close. It applies to today's bluetooth-next
with no adjustment, checkpatch --strict gives 0 errors and 0 checks, and the
third patch is measured rather than argued - 3 of 3 injected cycles failed
without it, 3 of 3 were clean with it, plus four uninjected controls.

I am not sending it. Four things are not mine to settle, and I would rather
ask than guess:

  1. Is 1/2 still a separate commit in your tree? The diff you sent reads
     either way, and the answer decides whether v3 is three patches or two.

  2. Can btintel_pcie_set_dxstate() race the gp0 handler? The new patch
     guards on alive_intr_ctxt so that RX is not armed twice, and that guard
     is only sound if it cannot. You know that path; I have only read it.

  3. Which tree is your hunk against? It sits at 5193, while bluetooth-next
     has set_dxstate() at 4169 and nothing inside it has moved since the
     series went out.

  4. Where do you want the W1C clear? It is your fix and pre-existing, and
     neither patch touched that line, so it reads better as its own commit
     with its own Fixes: than inside a PM patch. Say the word and v3 carries
     it as a fourth patch with your Signed-off-by; say nothing and I leave
     it out for you to send.

An answer to any of them - or "hold, I will carry it myself" - and I will
act on it the same day. Everything below is the evidence behind them.

Whose commit 1/2 is
===================

The diff applies to a tree without 1/2: its minus side carries the old
comment, its plus side carries Vladimir's re-read word for word. So either
the two commits are now one, or it was taken against the pre-series base.
I cannot tell from here.

If they are one, please keep his From: and Signed-off-by, or add
Co-developed-by: and Signed-off-by: if it has to stay a single commit. His
one condition for letting me carry his patch was authorship, and I answered
him on-list that it was already safe - so this part is mine to ask about
rather than his. His two Link: tags are worth keeping either way.

Ferenc Lengyel's Tested-by is a different matter: it was given for the patch
as posted, so dropping it from a modified one is fair. He is on Cc, so I am
saying it here rather than letting it go quietly.

The flag means less than it says
================================

I argued the opposite to you on 9 September - that moving gp0_received out
of the retry loop rescues a late interrupt, and that your comment stated the
invariant well. That was wrong, and I would rather say so than switch sides
quietly.

data->gp0_received = true is set at 1968 unconditionally, before a switch
that may match no case at all. The comment at 1940 says the same thing from
the other side: gp0 is raised for three causes and it is not easy to know
which one fired. So the flag means a gp0 arrived, not that the controller
moved.

"if (status) return 0;" at 4182 has no register read behind it. Together
those two let set_dxstate() report success while alive_intr_ctxt is stale
and the RX rings were never re-armed.

That needs no late interrupt and no second attempt: it happens inside one
pass, and it is in bluetooth-next today. Moving the flag out of the loop
widens it to a later attempt as well. The fold closes neither, because it
sits on the timeout path and this returns above it.

Measured
========

Fixture: the gp0 handler returns after data->gp0_received = true and before
the switch, once, while alive_intr_ctxt is D3. That is not a delayed
interrupt - it is what a gp0 that matches no case does on its own. s2idle,
rtcwake -m freeze -s 45, SP11, BE201 8086:a876 rev 10, kernel 7.0.0-30.

  build                                     injected    result
  ---------------------------------------------------------------
  set_dxstate() byte-identical to bt-next    3 of 3     failed
  1/2 + 2/2 + your fold                      3 of 3     failed
  the same plus the diff below               3 of 3     clean
  the same, no injection                     4 of 4     clean

Failure is the same every time:

    Bluetooth: hci0: Received hw exception interrupt
    Bluetooth: hci0: Controller in error state
    Bluetooth: hci0: command 0x0c01 tx timeout
    Bluetooth: hci0: Opcode 0x0c1a failed: -110
    btintel_pcie 0000:00:14.7: resetting

and the controller returns under a new hci index, so every paired device is
gone. In one of the six the FLR did not recover it either: it came back at
00:00:00:00:00:00 and needed btintel_pcie and btintel both unloaded.

The part I did not expect is how quiet it is. "Timeout (200 ms) on alive
interrupt" appears in none of the six, because the function returns above
it. Nothing in the log says the resume went wrong - the first sign is the
firmware exception a second later.

The diff
========

This is v3's third patch, shown against your folded version so the change is
visible rather than the whole function. It moves the verification above the
success return rather than adding anything, and does the handler's work only
when the handler has not.

  @@ -1,31 +1,36 @@
   		status = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
   			msecs_to_jiffies(dx_intr_timeout_ms));

  -		if (status)
  -			return 0;
  +		if (!status) {
  +			bt_dev_warn(data->hdev,
  +				   "Timeout (%u ms) on alive interrupt for D%d entry, retry count %d",
  +				   dx_intr_timeout_ms, dxstate, retry);

  -		bt_dev_warn(data->hdev,
  -			   "Timeout (%u ms) on alive interrupt for D%d entry, retry count %d",
  -			   dx_intr_timeout_ms, dxstate, retry);
  +			/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so write only
  +			 * this bit to avoid acking other pending causes
  +			 */
  +			btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
  +					      BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
  +		}

  -		/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so write only
  -		 * this bit to avoid acking other pending causes
  +		/* gp0_received only says a gp0 arrived. It is raised for three
  +		 * causes and the handler sets the flag before a switch that may
  +		 * match none of them, so it does not mean the target state was
  +		 * reached; a hardware bug may also drop the interrupt outright.
  +		 * Either way only the register knows, and only the handler
  +		 * refreshes the cache. Refresh it here and retry only if the
  +		 * state check still fails.
   		 */
  -		btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
  -				      BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
  -
  -		/* A hardware bug may cause the alive interrupt to be missed. Refresh
  -		 * boot_stage_cache from hardware, since only the interrupt handler
  -		 * updates it. Finally retry only if the state check still fails.
  -		 */
   		data->boot_stage_cache = btintel_pcie_rd_reg32(data,
   				BTINTEL_PCIE_CSR_BOOT_STAGE_REG);

   		if (dxstate == BTINTEL_PCIE_STATE_D0) {
   			if (btintel_pcie_in_d0(data)) {
  -				/* GP0 handler never ran to do this: keep the
  -				 * state tracker in sync and resubmit RX.
  +				/* Do what the handler's D3 -> D0 branch would
  +				 * have done, unless it already did it.
   				 */
  +				if (data->alive_intr_ctxt == BTINTEL_PCIE_D0)
  +					return 0;
   				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
   				btintel_pcie_reset_ia(data);
   				btintel_pcie_start_rx(data);

That guard - "if alive_intr_ctxt is already D0, the handler did it" - is
question 2 above. The handler is threaded (2779) and the gp0 dispatch at
2736 runs after irq_lock is dropped at 2697, so it can set gp0_received on
one CPU and reach submit_rx at 2030 while set_dxstate() is reading the
context on another. If that window is real the guard is wrong, and the rest
of the patch stands without it. It bears on your fold the same way.

Fifty clean S4 cycles cannot reach any of this - the path runs only when the
alive interrupt is missing or unactionable, so a healthy controller never
enters it. That makes your run a no-regression result on the normal path,
which is worth having. Send me whatever you settle on and I will put it
through the same fixture.

Whichever way this goes, please post it to the list rather than pushing it
straight - then Vladimir, Ferenc and Luiz all see the trailers before it
lands.

Thanks,
Sergey

RE: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Ravindra]

[Removed ravishankar.srivatsa@intel.com]

Hi Sergey,

> Subject: Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and
> S4
> 
> Thank you - and for taking the third finding, which I had written off as not this
> series' business.
> 
> Where this stands, and what I need from you
> ===========================================
> 
> A v3 is assembled and ready: 1/2 and 2/2 unchanged, plus a third patch for
> the one path the fold does not close. It applies to today's bluetooth-next with
> no adjustment, checkpatch --strict gives 0 errors and 0 checks, and the third
> patch is measured rather than argued - 3 of 3 injected cycles failed without it,
> 3 of 3 were clean with it, plus four uninjected controls.
> 
> I am not sending it. Four things are not mine to settle, and I would rather ask
> than guess:
> 
>   1. Is 1/2 still a separate commit in your tree? The diff you sent reads
>      either way, and the answer decides whether v3 is three patches or two.
   One clarification on the diff you saw: for my own testing I
   temporarily squashed everything (1/2 + 2/2 + your fold) into a
   single working commit. That is almost certainly why the diff
   read ambiguously. The squash was local-only and never intended
   for posting.

>   2. Can btintel_pcie_set_dxstate() race the gp0 handler? The new patch
>      guards on alive_intr_ctxt so that RX is not armed twice, and that guard
>      is only sound if it cannot. You know that path; I have only read it.
   Not in the way the fold's guard needs to worry about. GP0 is
   raised for a specific, known set of causes, and a D0<->D3
   transition is one of them. set_dxstate() waits 200 ms for that
   interrupt. If it has not arrived within that window, a missed
   interrupt is far more likely than a merely delayed one, since a
   healthy D0<->D3 transition completes well inside that budget.

   So when the timeout fires and the boot stage register already
   shows the target state, that is read as ground truth: the
   handler almost certainly never ran for this transition, so the
   driver re-arms the RX rings itself. There is no race with
   gp0_handler() here, because the register check and the re-arm
   only happen after the wait has already timed out; nothing about
   this path invokes or waits on a second, concurrent handler
   execution.

>   3. Which tree is your hunk against? It sits at 5193, while bluetooth-next
>      has set_dxstate() at 4169 and nothing inside it has moved since the
>      series went out.
   That hunk came from an internal kernel tree carrying some
   additional, unrelated changes, which is why the line numbers
   do not line up with bluetooth-next. Sorry for the inconvenience
   caused. v4 will be rebased and diffed directly against today's
   bluetooth-next/master so the line numbers match what you see
   there.

>   4. Where do you want the W1C clear? It is your fix and pre-existing, and
>      neither patch touched that line, so it reads better as its own commit
>      with its own Fixes: than inside a PM patch. Say the word and v3 carries
>      it as a fourth patch with your Signed-off-by; say nothing and I leave
>      it out for you to send.
   I will include it as 4/4 in v4, as its own commit with its own
   Fixes: tag and my Signed-off-by.

> An answer to any of them - or "hold, I will carry it myself" - and I will act on it
> the same day. Everything below is the evidence behind them.
> 
> Whose commit 1/2 is
> ===================
> 
> The diff applies to a tree without 1/2: its minus side carries the old comment,
> its plus side carries Vladimir's re-read word for word. So either the two
> commits are now one, or it was taken against the pre-series base.
> I cannot tell from here.
> 
> If they are one, please keep his From: and Signed-off-by, or add
> Co-developed-by: and Signed-off-by: if it has to stay a single commit. His one
> condition for letting me carry his patch was authorship, and I answered him
> on-list that it was already safe - so this part is mine to ask about rather than
> his. His two Link: tags are worth keeping either way.
> 
> Ferenc Lengyel's Tested-by is a different matter: it was given for the patch as
> posted, so dropping it from a modified one is fair. He is on Cc, so I am saying it
> here rather than letting it go quietly.
> 
> The flag means less than it says
> ================================
> 
> I argued the opposite to you on 9 September - that moving gp0_received out
> of the retry loop rescues a late interrupt, and that your comment stated the
> invariant well. That was wrong, and I would rather say so than switch sides
> quietly.

On moving data->gp0_received outside the retry loop:

That change assumed a late interrupt could still arrive after a
timed-out attempt, so keeping the flag armed across retries would
let it be caught. In practice that case does not occur: the only
scenario is a missed interrupt (never sent, not late), which the
existing register readback already handles on each retry. So there
is no benefit to relocating the flag, and I will keep it exactly as
it is in bluetooth-next today, reset inside the loop before each
sleep_cntrl write.

> data->gp0_received = true is set at 1968 unconditionally, before a
> data->switch
> that may match no case at all. The comment at 1940 says the same thing from
> the other side: gp0 is raised for three causes and it is not easy to know which
> one fired. So the flag means a gp0 arrived, not that the controller moved.

That comment looks misguiding. Will fix it in the next v4 series.
alive_intr_ctxt records the reason the driver is waiting for the
alive interrupt, not an unconstrained guess. Before the switch even
runs, the top of the gp0 handler already checks the error and
lockdown states and returns early on either, so gp0 arriving for a
genuinely unexpected/error reason is filtered out before
gp0_received is ever set. By the time gp0_received = true is
reached, the interrupt has already been qualified as a normal
boot-stage transition, not an arbitrary "any cause" signal as the
comment implies.

> "if (status) return 0;" at 4182 has no register read behind it. Together those
> two let set_dxstate() report success while alive_intr_ctxt is stale and the RX
> rings were never re-armed.

You are correct. In the interrupt-miss case, both
alive_intr_ctxt and the RX rings need to be updated. Otherwise,
set_dxstate() may report success while operating on stale state.

I will include the fix you provided, along with your Signed-off-by,
in the next revision.

> 
> That needs no late interrupt and no second attempt: it happens inside one
> pass, and it is in bluetooth-next today. Moving the flag out of the loop widens
> it to a later attempt as well. The fold closes neither, because it sits on the
> timeout path and this returns above it.
>
> Measured
> ========
> 
> Fixture: the gp0 handler returns after data->gp0_received = true and before
> the switch, once, while alive_intr_ctxt is D3. That is not a delayed interrupt - it
> is what a gp0 that matches no case does on its own. s2idle, rtcwake -m freeze
> -s 45, SP11, BE201 8086:a876 rev 10, kernel 7.0.0-30.
> 
>   build                                     injected    result
>   ---------------------------------------------------------------
>   set_dxstate() byte-identical to bt-next    3 of 3     failed
>   1/2 + 2/2 + your fold                      3 of 3     failed
>   the same plus the diff below               3 of 3     clean
>   the same, no injection                     4 of 4     clean
> 
> Failure is the same every time:
> 
>     Bluetooth: hci0: Received hw exception interrupt
>     Bluetooth: hci0: Controller in error state
>     Bluetooth: hci0: command 0x0c01 tx timeout
>     Bluetooth: hci0: Opcode 0x0c1a failed: -110
>     btintel_pcie 0000:00:14.7: resetting
> 
> and the controller returns under a new hci index, so every paired device is
> gone. In one of the six the FLR did not recover it either: it came back at
> 00:00:00:00:00:00 and needed btintel_pcie and btintel both unloaded.
> 
> The part I did not expect is how quiet it is. "Timeout (200 ms) on alive
> interrupt" appears in none of the six, because the function returns above it.
> Nothing in the log says the resume went wrong - the first sign is the firmware
> exception a second later.
> 
> The diff
> ========
> 
> This is v3's third patch, shown against your folded version so the change is
> visible rather than the whole function. It moves the verification above the
> success return rather than adding anything, and does the handler's work only
> when the handler has not.
> 
>   @@ -1,31 +1,36 @@
>    		status = wait_event_timeout(data->gp0_wait_q, data-
> >gp0_received,
>    			msecs_to_jiffies(dx_intr_timeout_ms));
> 
>   -		if (status)
>   -			return 0;
>   +		if (!status) {
>   +			bt_dev_warn(data->hdev,
>   +				   "Timeout (%u ms) on alive interrupt for D%d
> entry, retry count %d",
>   +				   dx_intr_timeout_ms, dxstate, retry);
> 
>   -		bt_dev_warn(data->hdev,
>   -			   "Timeout (%u ms) on alive interrupt for D%d entry,
> retry count %d",
>   -			   dx_intr_timeout_ms, dxstate, retry);
>   +			/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so
> write only
>   +			 * this bit to avoid acking other pending causes
>   +			 */
>   +			btintel_pcie_wr_reg32(data,
> BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
>   +
> BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
>   +		}
> 
>   -		/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so write
> only
>   -		 * this bit to avoid acking other pending causes
>   +		/* gp0_received only says a gp0 arrived. It is raised for three
>   +		 * causes and the handler sets the flag before a switch that
> may
>   +		 * match none of them, so it does not mean the target state
> was
>   +		 * reached; a hardware bug may also drop the interrupt
> outright.
>   +		 * Either way only the register knows, and only the handler
>   +		 * refreshes the cache. Refresh it here and retry only if the
>   +		 * state check still fails.
>    		 */
>   -		btintel_pcie_wr_reg32(data,
> BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
>   -
> BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
>   -
>   -		/* A hardware bug may cause the alive interrupt to be missed.
> Refresh
>   -		 * boot_stage_cache from hardware, since only the interrupt
> handler
>   -		 * updates it. Finally retry only if the state check still fails.
>   -		 */
>    		data->boot_stage_cache = btintel_pcie_rd_reg32(data,
>    				BTINTEL_PCIE_CSR_BOOT_STAGE_REG);
> 
>    		if (dxstate == BTINTEL_PCIE_STATE_D0) {
>    			if (btintel_pcie_in_d0(data)) {
>   -				/* GP0 handler never ran to do this: keep the
>   -				 * state tracker in sync and resubmit RX.
>   +				/* Do what the handler's D3 -> D0 branch
> would
>   +				 * have done, unless it already did it.
>    				 */
>   +				if (data->alive_intr_ctxt == BTINTEL_PCIE_D0)
>   +					return 0;
>    				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
>    				btintel_pcie_reset_ia(data);
>    				btintel_pcie_start_rx(data);
> 
> That guard - "if alive_intr_ctxt is already D0, the handler did it" - is question 2
> above. The handler is threaded (2779) and the gp0 dispatch at
> 2736 runs after irq_lock is dropped at 2697, so it can set gp0_received on one
> CPU and reach submit_rx at 2030 while set_dxstate() is reading the context on
> another. If that window is real the guard is wrong, and the rest of the patch
> stands without it. It bears on your fold the same way.
> 
> Fifty clean S4 cycles cannot reach any of this - the path runs only when the
> alive interrupt is missing or unactionable, so a healthy controller never enters
> it. That makes your run a no-regression result on the normal path, which is
> worth having. Send me whatever you settle on and I will put it through the
> same fixture.
> 
> Whichever way this goes, please post it to the list rather than pushing it
> straight - then Vladimir, Ferenc and Luiz all see the trailers before it lands.
> 
Thanks for the suggestion. I will post the mailing list.

> Thanks,
> Sergey

Thanks,
Ravindra

Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Sergey Lebedev]

Ravindra,

Thank you - that settles it. I am not sending a v3; your v4 is the right
vehicle, and I will not put a second copy of one fix in flight.

But do not take the third patch as posted. Re-reading it against your
answer turned up four things, three of them mine. Line numbers are from
bluetooth-next master at 671d566d3c3b.

1. The comment is wrong, and your answer is what makes it wrong.

You are right that error and lockdown are filtered at 1958 and 1965,
before the flag is set. My comment says the flag is raised for three
causes and the switch may match none of them. Carried as written, v4
would fix the old misguiding comment and import the same fault in a new
one.

The patch is still needed, for a reason the comment does not give:

  1971  data->gp0_received = true;
  2014  case BTINTEL_PCIE_D3:
  2015          if (btintel_pcie_in_d0(data)) {
   ..              ctxt = D0, submit_rx, signal_waitq
  2020          }
  2021          break;          <- matched, changed nothing

A matched case can complete having done nothing, with the flag already
true and signal_waitq clear. So it is not about which cause raised the
interrupt. That also means "the only scenario is a missed interrupt" is
too narrow: here the interrupt arrives and is handled, to no effect.

And it bites without any missed interrupt. Measured on the part rather
than argued, with a debug copy of the module: the handler's D3 case is
made to read in_d0() as false so it takes its own 2021 break, a 2 ms
sleep before the wait widens a window that is already there, and the
early return is a module parameter so stock and patched are one build.
The SP11 lines below are that instrumentation; everything else is the
driver's own.

  stock (early return kept):
    SP11: handler forced down the do-nothing break
    SP11: early return, dx=0 ctxt=6 after 204902 us
    Bluetooth: hci0: Received hw exception interrupt
    btintel_pcie 0000:00:14.7: resetting
    Bluetooth: hci0: command 0x0c01 tx timeout
    Bluetooth: hci0: Opcode 0x0c01 failed: -110
    Bluetooth: hci0: Opcode 0x0c1a failed: -110

  with the patch:
    SP11: handler forced down the do-nothing break
    SP11: verified path re-armed RX
    SP11: ok dx=0 ctxt=5 waits=1 nosleep=1 2133 us

ctxt 6 is D3 and 5 is D0, so 4213 returned success with the context still
saying D3 and the rings unarmed. Note the 204902 us: signal_waitq is clear
on that path, so nothing woke the queue - the wait ran its full 200 ms,
re-checked the condition at expiry, found the flag true and returned
non-zero. The success was believed on a flag set by a handler run that
had done nothing and had not even woken anyone.

One caveat on that pair, since it is a race and I would rather you heard
it from me. An earlier attempt suppressed the handler once instead, and
the stock side came back clean - a later handler run had repaired the
state before anything used it. So the window is real and reachable but
not hit every time. The older fixture, which injected a return between
1971 and the switch rather than taking the 2021 path, failed 3 of 3 and
was clean 3 of 3 with the fix; that is where the reproducibility comes
from, and this run is what ties it to the real path.

2. Keeping gp0_received inside the loop is not optional once this patch
is in, which is a second reason for the decision you already made.

With the flag hoisted out of the loop and "if (status) return 0" gone,
once any handler run has set it - the 2021 case is one - the later
retries find it still true, wait_event_timeout returns at once, and each
retry becomes a bare register read. Three 200 ms attempts collapse into
microseconds and the function returns -EBUSY without ever having waited.
A genuinely missed interrupt leaves the flag false and still waits, so
this only shows up in exactly the case the patch is for.

Measured the same way, on the suspend direction so a failure only aborts
the suspend, with the state check forced to fail all three times:

    hoisted:      waits=3  nosleep=2  total    1465 us   (and 1143 us)
    in the loop:  waits=3  nosleep=0  total  622445 us   (and 411499 us)

with the three "Timeout (200 ms)" lines present only in the second. The
first pair ran back to back, which left the second configuration facing a
controller the first had put in D3, so it was not a control; the figures
in brackets are a rerun in the opposite order with a clean, unforced
suspend/resume before each. The shape held both ways.

Your v4 resets it inside the loop, so this does not arise - but the two
changes are coupled and nothing says so.

3. The patch's D0 branch is an incomplete substitute for the handler.

The handler's submit_rx block does three things, not two: reset_ia,
start_rx, and the mbox<->alive handshake at 2038-2041. Mine does the
first two and claims in its comment to do what the branch would have.
If MBOX_PARSE_PENDING is set, the waiter at 1615 then sits out its
timeout for nothing.

4. start_rx() can fail and its return is discarded.

The handler is void and has no choice; set_dxstate() returns a status to
the resume path and does. Reporting 0 with the rings unarmed is the exact
condition this patch exists to prevent.

Rolled up, with the comment rewritten (it needs an int err; beside the
existing retry and status):

  		if (dxstate == BTINTEL_PCIE_STATE_D0) {
  			if (btintel_pcie_in_d0(data)) {
 +				/* Do what the handler's D3 -> D0 branch
 +				 * would have done, unless it already has.
 +				 */
  				if (data->alive_intr_ctxt == BTINTEL_PCIE_D0)
  					return 0;
  				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
  				btintel_pcie_reset_ia(data);
 -				btintel_pcie_start_rx(data);
 +				err = btintel_pcie_start_rx(data);
 +				if (err)
 +					return err;
 +
 +				/* Complete the mbox<->alive handshake */
 +				if (test_and_clear_bit(BTINTEL_PCIE_MBOX_PARSE_PENDING,
 +						       &data->flags)) {
 +					set_bit(BTINTEL_PCIE_MBOX_PARSE_READY,
 +						&data->flags);
 +					wake_up(&data->mbox_parse_wait_q);
 +				}
  				return 0;
  			}

and the comment above the register read:

  /* gp0_received is set at the top of the handler, before the switch on
   * alive_intr_ctxt. Error and lockdown are filtered out above it, but a
   * matched case can still complete without doing anything - D3 breaks
   * unchanged while the controller has not reached D0 - and a hardware
   * bug may drop the interrupt outright. Either way the flag says a gp0
   * was handled, not that the transition completed, and only the register
   * knows. Refresh the cache here and retry only if the state check still
   * fails.
   */

The D3 branch needs no such guard: setting alive_intr_ctxt to D3 twice
has no second effect, where reset_ia and start_rx do.

On your 4/4, for what it is worth from here: clr_reg_bits() is a
read-modify-write, so on a W1C register it writes 0 to GP0 - not clearing
it - and 1 to every other cause that happened to be pending, acking
HWEXP, GP1 or FWTRIG before the ISR sees them. The ISR's own idiom is the
proof: 2712 reads the HW causes and 2716 clears them by writing the same
value back, under the comment that says so.

The one thing I need from you is question 1's other half.

Please keep Vladimir V. Kondratyev's From: and Signed-off-by on 1/2. His
one condition for letting me carry his patch was authorship credit, and I
relayed that to the list with the assurance that it was already the case.
As posted, 1/2 carries his From: at the top, his Signed-off-by first and
his two Link: tags, with my Tested-by and Signed-off-by beneath:

  https://lore.kernel.org/linux-bluetooth/20260909123416.71919-2-lsa.uz@pm.me/

Your answer explains the ambiguous diff as a local squash, which I read as
1/2 staying its own commit - but you did not say so, and it is his name on
it rather than mine, so I would rather ask than infer.

Same rule for the third patch, whichever form you prefer: Co-developed-by:
with the Signed-off-by, or From:. A bare sign-off records the chain but
not the author.

On question 2, your answer is enough whichever way that window falls. If a
handler is in flight while set_dxstate() reads the context, the guard
simply does not fire and we are back to the behaviour without it. It can
help and cannot hurt, so it never needed the window to be impossible.

The three patches apply to 671d566d3c3b with no fuzz. So does the rolled-up
version above, which is not just quoted at you: applied on top of them it
builds W=1 with sparse, zero warnings, and checkpatch --strict gives
0 errors, 0 warnings, 0 checks over 195 lines.

The bench for the measurements: kernel 7.2.0-rc6-btnext-norework built
from e40edfa04, where set_dxstate() and the gp0 handler are byte-identical
to 671d566d3c3b - only their line numbers moved, 4133 against 4200 - on a
BE201 8086:a876 rev 10.

Send v4 when it is ready and I will put it through the same bench.

Sergey

RE: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Ravindra]

Hi Sergey,

Thanks for the careful re-read and for the measurements. I’ve updated the v4
series to address the four points in your review.

Patch 3 now treats `gp0_received` as evidence that the handler ran, not that
the transition completed. It refreshes the hardware state before accepting
success, keeps the flag reset inside the retry loop, completes the
mailbox/alive handshake when restoring the D0 path, propagates
`btintel_pcie_start_rx()` errors, and restores the D3 context where needed. I
also used your revised comments.

To answer your authorship question explicitly: patches 1 and 2 remain
separate commits. Patch 1 retains Vladimir’s `From:` and `Signed-off-by:`,
along with the existing `Link:` tags and your `Tested-by:` and
`Signed-off-by:`. I also updated its `Fixes:` tag to `88c6216a52ea`
(“Bluetooth: btintel_pcie: Suspend/Resume: Controller doorbell interrupt
handling”). Patch 3 has you as `From:` author, with both `Signed-off-by:`
trailers. Patch 4 remains authored and signed off by me.

Patch 4 uses a direct W1C write for GP0. The complete four-patch series
applies cleanly, and the local build and strict checkpatch validation pass.
I haven’t independently run your forced-path bench, so I appreciate your
offer to test this v4 against it.

I am sending the v4 series which includes all 4 patches as part of it.

Best Regards,
Ravindra

> -----Original Message-----
> From: Sergey Lebedev <lsa.uz@pm.me>
> Sent: Friday, September 25, 2026 12:43 AM
> To: Ravindra <ravindra@intel.com>; Luiz Augusto von Dentz
> <luiz.dentz@gmail.com>; Marcel Holtmann <marcel@holtmann.org>
> Cc: Vladimir V . Kondratyev <vladimirkondratyev2@gmail.com>; Ferenc
> Lengyel <dev@lengyelf.eu>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; Paul Menzel
> <pmenzel@molgen.mpg.de>; K, Kiran <kiran.k@intel.com>; Devegowda,
> Chandrashekar <chandrashekar.devegowda@intel.com>; Chambarakatta,
> Mahalingeshwara <mahalingeshwara.chambarakatta@intel.com>; Arnd
> Bergmann <arnd@arndb.de>; linux-bluetooth@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and
> S4
> 
> Ravindra,
> 
> Thank you - that settles it. I am not sending a v3; your v4 is the right vehicle,
> and I will not put a second copy of one fix in flight.
> 
> But do not take the third patch as posted. Re-reading it against your answer
> turned up four things, three of them mine. Line numbers are from bluetooth-
> next master at 671d566d3c3b.
> 
> 1. The comment is wrong, and your answer is what makes it wrong.
> 
> You are right that error and lockdown are filtered at 1958 and 1965, before
> the flag is set. My comment says the flag is raised for three causes and the
> switch may match none of them. Carried as written, v4 would fix the old
> misguiding comment and import the same fault in a new one.
> 
> The patch is still needed, for a reason the comment does not give:
> 
>   1971  data->gp0_received = true;
>   2014  case BTINTEL_PCIE_D3:
>   2015          if (btintel_pcie_in_d0(data)) {
>    ..              ctxt = D0, submit_rx, signal_waitq
>   2020          }
>   2021          break;          <- matched, changed nothing
> 
> A matched case can complete having done nothing, with the flag already true
> and signal_waitq clear. So it is not about which cause raised the interrupt. That
> also means "the only scenario is a missed interrupt" is too narrow: here the
> interrupt arrives and is handled, to no effect.
> 
> And it bites without any missed interrupt. Measured on the part rather than
> argued, with a debug copy of the module: the handler's D3 case is made to
> read in_d0() as false so it takes its own 2021 break, a 2 ms sleep before the
> wait widens a window that is already there, and the early return is a module
> parameter so stock and patched are one build.
> The SP11 lines below are that instrumentation; everything else is the driver's
> own.
> 
>   stock (early return kept):
>     SP11: handler forced down the do-nothing break
>     SP11: early return, dx=0 ctxt=6 after 204902 us
>     Bluetooth: hci0: Received hw exception interrupt
>     btintel_pcie 0000:00:14.7: resetting
>     Bluetooth: hci0: command 0x0c01 tx timeout
>     Bluetooth: hci0: Opcode 0x0c01 failed: -110
>     Bluetooth: hci0: Opcode 0x0c1a failed: -110
> 
>   with the patch:
>     SP11: handler forced down the do-nothing break
>     SP11: verified path re-armed RX
>     SP11: ok dx=0 ctxt=5 waits=1 nosleep=1 2133 us
> 
> ctxt 6 is D3 and 5 is D0, so 4213 returned success with the context still saying
> D3 and the rings unarmed. Note the 204902 us: signal_waitq is clear on that
> path, so nothing woke the queue - the wait ran its full 200 ms, re-checked the
> condition at expiry, found the flag true and returned non-zero. The success
> was believed on a flag set by a handler run that had done nothing and had not
> even woken anyone.
> 
> One caveat on that pair, since it is a race and I would rather you heard it from
> me. An earlier attempt suppressed the handler once instead, and the stock
> side came back clean - a later handler run had repaired the state before
> anything used it. So the window is real and reachable but not hit every time.
> The older fixture, which injected a return between
> 1971 and the switch rather than taking the 2021 path, failed 3 of 3 and was
> clean 3 of 3 with the fix; that is where the reproducibility comes from, and this
> run is what ties it to the real path.
> 
> 2. Keeping gp0_received inside the loop is not optional once this patch is in,
> which is a second reason for the decision you already made.
> 
> With the flag hoisted out of the loop and "if (status) return 0" gone, once any
> handler run has set it - the 2021 case is one - the later retries find it still true,
> wait_event_timeout returns at once, and each retry becomes a bare register
> read. Three 200 ms attempts collapse into microseconds and the function
> returns -EBUSY without ever having waited.
> A genuinely missed interrupt leaves the flag false and still waits, so this only
> shows up in exactly the case the patch is for.
> 
> Measured the same way, on the suspend direction so a failure only aborts the
> suspend, with the state check forced to fail all three times:
> 
>     hoisted:      waits=3  nosleep=2  total    1465 us   (and 1143 us)
>     in the loop:  waits=3  nosleep=0  total  622445 us   (and 411499 us)
> 
> with the three "Timeout (200 ms)" lines present only in the second. The first
> pair ran back to back, which left the second configuration facing a controller
> the first had put in D3, so it was not a control; the figures in brackets are a
> rerun in the opposite order with a clean, unforced suspend/resume before
> each. The shape held both ways.
> 
> Your v4 resets it inside the loop, so this does not arise - but the two changes
> are coupled and nothing says so.
> 
> 3. The patch's D0 branch is an incomplete substitute for the handler.
> 
> The handler's submit_rx block does three things, not two: reset_ia, start_rx,
> and the mbox<->alive handshake at 2038-2041. Mine does the first two and
> claims in its comment to do what the branch would have.
> If MBOX_PARSE_PENDING is set, the waiter at 1615 then sits out its timeout
> for nothing.
> 
> 4. start_rx() can fail and its return is discarded.
> 
> The handler is void and has no choice; set_dxstate() returns a status to the
> resume path and does. Reporting 0 with the rings unarmed is the exact
> condition this patch exists to prevent.
> 
> Rolled up, with the comment rewritten (it needs an int err; beside the existing
> retry and status):
> 
>   		if (dxstate == BTINTEL_PCIE_STATE_D0) {
>   			if (btintel_pcie_in_d0(data)) {
>  +				/* Do what the handler's D3 -> D0 branch
>  +				 * would have done, unless it already has.
>  +				 */
>   				if (data->alive_intr_ctxt == BTINTEL_PCIE_D0)
>   					return 0;
>   				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
>   				btintel_pcie_reset_ia(data);
>  -				btintel_pcie_start_rx(data);
>  +				err = btintel_pcie_start_rx(data);
>  +				if (err)
>  +					return err;
>  +
>  +				/* Complete the mbox<->alive handshake */
>  +				if
> (test_and_clear_bit(BTINTEL_PCIE_MBOX_PARSE_PENDING,
>  +						       &data->flags)) {
>  +
> 	set_bit(BTINTEL_PCIE_MBOX_PARSE_READY,
>  +						&data->flags);
>  +					wake_up(&data-
> >mbox_parse_wait_q);
>  +				}
>   				return 0;
>   			}
> 
> and the comment above the register read:
> 
>   /* gp0_received is set at the top of the handler, before the switch on
>    * alive_intr_ctxt. Error and lockdown are filtered out above it, but a
>    * matched case can still complete without doing anything - D3 breaks
>    * unchanged while the controller has not reached D0 - and a hardware
>    * bug may drop the interrupt outright. Either way the flag says a gp0
>    * was handled, not that the transition completed, and only the register
>    * knows. Refresh the cache here and retry only if the state check still
>    * fails.
>    */
> 
> The D3 branch needs no such guard: setting alive_intr_ctxt to D3 twice has no
> second effect, where reset_ia and start_rx do.
> 
> On your 4/4, for what it is worth from here: clr_reg_bits() is a read-modify-
> write, so on a W1C register it writes 0 to GP0 - not clearing it - and 1 to every
> other cause that happened to be pending, acking HWEXP, GP1 or FWTRIG
> before the ISR sees them. The ISR's own idiom is the
> proof: 2712 reads the HW causes and 2716 clears them by writing the same
> value back, under the comment that says so.
> 
> The one thing I need from you is question 1's other half.
> 
> Please keep Vladimir V. Kondratyev's From: and Signed-off-by on 1/2. His one
> condition for letting me carry his patch was authorship credit, and I relayed
> that to the list with the assurance that it was already the case.
> As posted, 1/2 carries his From: at the top, his Signed-off-by first and his two
> Link: tags, with my Tested-by and Signed-off-by beneath:
> 
>   https://lore.kernel.org/linux-bluetooth/20260909123416.71919-2-
> lsa.uz@pm.me/
> 
> Your answer explains the ambiguous diff as a local squash, which I read as
> 1/2 staying its own commit - but you did not say so, and it is his name on it
> rather than mine, so I would rather ask than infer.
> 
> Same rule for the third patch, whichever form you prefer: Co-developed-by:
> with the Signed-off-by, or From:. A bare sign-off records the chain but not the
> author.
> 
> On question 2, your answer is enough whichever way that window falls. If a
> handler is in flight while set_dxstate() reads the context, the guard simply does
> not fire and we are back to the behaviour without it. It can help and cannot
> hurt, so it never needed the window to be impossible.
> 
> The three patches apply to 671d566d3c3b with no fuzz. So does the rolled-up
> version above, which is not just quoted at you: applied on top of them it
> builds W=1 with sparse, zero warnings, and checkpatch --strict gives
> 0 errors, 0 warnings, 0 checks over 195 lines.
> 
> The bench for the measurements: kernel 7.2.0-rc6-btnext-norework built
> from e40edfa04, where set_dxstate() and the gp0 handler are byte-identical
> to 671d566d3c3b - only their line numbers moved, 4133 against 4200 - on a
> BE201 8086:a876 rev 10.
> 
> Send v4 when it is ready and I will put it through the same bench.
> 
> Sergey

Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Sergey Lebedev]

Ravindra,

Thank you - all four are in. I checked them in the code, with v4 applied
from lore onto 671d566d3c3b and onto e40edfa04, where it lands identically.

Your D3 branch also closes a hole in what I proposed: if the handler's D0
case runs before the controller reaches D3, it breaks without recording
D3, the context stays D0, and the guard in the D0 branch then skips the
re-arm on resume. The bench shows it with and without your branch.

Bench: Surface Pro 11, BE201 8086:a876, the driver at bluetooth-next
e40edfa04 against the same plus v4, one instrumented build each. "HCI ok"
is Read Local Version returning status 0 after the cycle.

  first gp0 on D3 entry dropped, as a missed interrupt (1/4's case)
    stock  timeouts at retries 0, 1 and 2, -EBUSY, suspend aborted,
           HCI fails
    v4     one timeout, D3 recorded from the register, HCI ok (2 of 2)

  handler's D0 case forced to break on suspend
    v4 without the D3 branch   resume skips the re-arm, HCI fails
    v4                         D3 recorded, handler re-arms on resume,
                               HCI ok (2 of 2)

  handler's D3 case forced to break on resume
    stock  success reported with ctxt 6 (D3), then hw exception, FLR,
           0x0c01 tx timeout
    v4     re-armed to ctxt 5 (D0) either way the race falls: handler
           before the wait, 2.2 ms; during it, 208 ms; HCI ok (2 of 2)

  state check forced to fail three times on suspend
    v4     waits=3, none skipped, 413 ms, -EBUSY as forced

  plain s2idle
    v4     D3 in 1.5-1.6 ms, D0 in 1.5-1.7 ms, HCI ok (3 of 3)

  S4, in the kernel's suspend and test_resume hibernation modes
    stock  .thaw goes through FLR
    v4     .freeze: D3_COLD in 1.5-1.8 ms
           .thaw: D0 in 1.6 ms, HCI ok after (2 of 2)
           .restore: FLR, firmware reloaded, HCI ok (2 of 2)

S4 ran on the bench kernel, since the distribution kernel refuses it
under Secure Boot lockdown, and without cutting power, so .poweroff is
not covered. The firmware offers no S3.

After the FLR in .restore the log says "BT reprobe failed", on stock
too; the device probes again about a second later and works.

Tested-by for 4/4 follows in its own thread.

Sergey

RE: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Ravindra]

Hi Sergey,

Thank you for running the full bench and for confirming the results.

The additional D3 branch was intended to cover exactly the resume hole you
identified: when the handler's D0 case runs before the controller reaches D3,
the context can remain D0 and the resume guard can incorrectly skip re-arming
RX. Recording D3 from the verified hardware state ensures the next D0
transition performs the required re-arm.

Your results also confirm the other parts of patch 3: missed GP0 handling,
the D3-to-D0 fallback, retry-flag reset, and preservation of the normal S4
freeze/thaw and restore flows. It is good to see HCI recover successfully in
all of those cases.

Thanks again for the detailed testing. I'll include your `Tested-by:` and
`Reviewed-by:` tags for patch 4.

Best Regards,
Ravindra

> Subject: Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and
> S4
> 
> Ravindra,
> 
> Thank you - all four are in. I checked them in the code, with v4 applied from
> lore onto 671d566d3c3b and onto e40edfa04, where it lands identically.
> 
> Your D3 branch also closes a hole in what I proposed: if the handler's D0 case
> runs before the controller reaches D3, it breaks without recording D3, the
> context stays D0, and the guard in the D0 branch then skips the re-arm on
> resume. The bench shows it with and without your branch.
> 
> Bench: Surface Pro 11, BE201 8086:a876, the driver at bluetooth-next
> e40edfa04 against the same plus v4, one instrumented build each. "HCI ok"
> is Read Local Version returning status 0 after the cycle.
> 
>   first gp0 on D3 entry dropped, as a missed interrupt (1/4's case)
>     stock  timeouts at retries 0, 1 and 2, -EBUSY, suspend aborted,
>            HCI fails
>     v4     one timeout, D3 recorded from the register, HCI ok (2 of 2)
> 
>   handler's D0 case forced to break on suspend
>     v4 without the D3 branch   resume skips the re-arm, HCI fails
>     v4                         D3 recorded, handler re-arms on resume,
>                                HCI ok (2 of 2)
> 
>   handler's D3 case forced to break on resume
>     stock  success reported with ctxt 6 (D3), then hw exception, FLR,
>            0x0c01 tx timeout
>     v4     re-armed to ctxt 5 (D0) either way the race falls: handler
>            before the wait, 2.2 ms; during it, 208 ms; HCI ok (2 of 2)
> 
>   state check forced to fail three times on suspend
>     v4     waits=3, none skipped, 413 ms, -EBUSY as forced
> 
>   plain s2idle
>     v4     D3 in 1.5-1.6 ms, D0 in 1.5-1.7 ms, HCI ok (3 of 3)
> 
>   S4, in the kernel's suspend and test_resume hibernation modes
>     stock  .thaw goes through FLR
>     v4     .freeze: D3_COLD in 1.5-1.8 ms
>            .thaw: D0 in 1.6 ms, HCI ok after (2 of 2)
>            .restore: FLR, firmware reloaded, HCI ok (2 of 2)
> 
> S4 ran on the bench kernel, since the distribution kernel refuses it under
> Secure Boot lockdown, and without cutting power, so .poweroff is not
> covered. The firmware offers no S3.
> 
> After the FLR in .restore the log says "BT reprobe failed", on stock too; the
> device probes again about a second later and works.
> 
> Tested-by for 4/4 follows in its own thread.
> 
> Sergey

Re: [PATCH 0/2] Bluetooth: btintel_pcie: two PM fixes, assembled as one series

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed, 09 Sep 2026 12:34:25 +0000 you wrote:
> Two patches already on this list fix different halves of the same fault, and
> each leaves a real failure behind when applied alone. This assembles them into
> one series. I am the submitter only - authorship, Fixes: tags and existing
> trailers are unchanged.
> 
>   1/2  Vladimir V. Kondratyev  - re-read BOOT_STAGE_REG before the fallback
>                                  check, so a missed alive interrupt is
>                                  survivable
>   2/2  Ravindra (Intel)        - fix the PM flow for S0ix, S3 and S4, which
>                                  among other things stops .thaw running an FLR
> 
> [...]

Here is the summary with links:
  - [1/2] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check
    https://git.kernel.org/bluetooth/bluetooth-next/c/20dfccf8d854
  - [2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html