Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

[Sergey Lebedev]

Hi Vladimir, Marcel, Luiz,

This patch is four months old and has had no reply other than the CI
bot. It fixes a real failure. Here is a report from the field and a
test of the patch itself.

Hardware: Microsoft Surface Pro 11 (Intel, Lunar Lake), Intel BE201,
8086:a876 rev 10, firmware timestamp 2026.8 buildtype 1 build 113003,
SHA1 0x08f159a0.

The failure, as it happened here on Ubuntu 7.0.0-28 before any
workaround existed:

  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

One device returning -EBUSY aborts the whole system suspend, so the
machine simply does not sleep. The workaround in use here since then
unloads btintel_pcie from a systemd sleep hook.

On the frequency I have to be honest rather than useful: it failed on
2 of the 3 suspends attempted before the workaround was installed, and
this week, on 7.0.0-30 with the workaround disabled, it succeeded 7
times out of 7 (3 of those with an LE scan running). Same firmware,
and no btintel_pcie change between the two kernels. So I cannot give
you a reproduction rate, and I am not going to invent one.

What I could do instead is exercise the exact path your patch touches,
deterministically. Method, so it can be judged:

  - built v7.0 drivers/bluetooth/btintel_pcie.c out of tree against the
    running kernel's headers, unmodified apart from the debug knob
    below, and confirmed it loads and works;

  - added a debug-only module parameter that returns from
    btintel_pcie_msix_gp0_handler() before the boot_stage_cache
    refresh, and only while alive_intr_ctxt is BTINTEL_PCIE_D0. That
    leaves boot_stage_cache stale and gp0_received false, which is
    precisely the state a genuinely missed alive interrupt leaves
    behind, while the controller itself still reaches D3.

Without your patch, the emulated missed interrupt reproduces the
production failure exactly:

  Timeout (200 ms) on alive interrupt for D2 entry, retry count 0
  Timeout (200 ms) on alive interrupt for D2 entry, retry count 1
  Timeout (200 ms) on alive interrupt for D2 entry, retry count 2
  btintel_pcie 0000:00:14.7: PM: failed to suspend async: error -16
  PM: Some devices failed to suspend, or early wake event detected

With your patch applied to that same build, and nothing else changed:

  PM: suspend entry (s2idle)
  Bluetooth: hci0: Timeout (200 ms) on alive interrupt for D2 entry, retry count 0
  PM: suspend exit

One timeout, then the re-read observes D3, set_dxstate() returns 0, and
the system suspends and resumes normally. That is the difference the
two added lines make.

The debug knob is a test fixture, not a proposal; I am happy to send it
to anyone who wants to reproduce this.

Worth noting that btintel_pcie_set_dxstate() is byte-identical in v7.0
and v7.1-rc7, so this is current, not historical.

Tested-by: Sergey Lebedev <lsa.uz@pm.me>

I have the hardware and can re-test any revision of this patch.

Thanks,
Sergey

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

[Vladimir V. Kondratyev]

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()")

Signed-off-by: Vladimir V. Kondratyev <vladimirkondratyev2@gmail.com>
---
 drivers/bluetooth/btintel_pcie.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index fda474406003..7fca8eea6e01 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -2778,9 +2778,13 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
 					  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.
+		 * Refresh boot_stage_cache from hardware since it is only
+		 * updated by the interrupt handler, then check if the
+		 * controller reached the expected state and retry the
+		 * operation only if it hasn't.
 		 */
+		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;
-- 
2.54.0

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

[Vladimir V. Kondratyev]

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()")

Signed-off-by: Vladimir V. Kondratyev <vladimirkondratyev2@gmail.com>
Tested-by: Sergey Lebedev <lsa.uz@pm.me>
---
 drivers/bluetooth/btintel_pcie.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index fda474406003..7fca8eea6e01 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -2778,9 +2778,13 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
 					  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.
+		 * Refresh boot_stage_cache from hardware since it is only
+		 * updated by the interrupt handler, then check if the
+		 * controller reached the expected state and retry the
+		 * operation only if it hasn't.
 		 */
+		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;
-- 
2.55.0

Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

[Paul Menzel]

[Cc: +Chandrashekar, +Kiran]

Dear Vladimir,


Thank you for your patch.

Am 01.09.26 um 22:37 schrieb Vladimir V. Kondratyev:
> 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.

Can you reproduce this on real hardware?

> 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()")
> 
> Signed-off-by: Vladimir V. Kondratyev <vladimirkondratyev2@gmail.com>
> Tested-by: Sergey Lebedev <lsa.uz@pm.me>
> ---
>   drivers/bluetooth/btintel_pcie.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fda474406003..7fca8eea6e01 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -2778,9 +2778,13 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
>   					  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.
> +		 * Refresh boot_stage_cache from hardware since it is only
> +		 * updated by the interrupt handler, then check if the
> +		 * controller reached the expected state and retry the
> +		 * operation only if it hasn't.

Re-flow for less lines?

>   		 */
> +		data->boot_stage_cache = btintel_pcie_rd_reg32(data,
> +							       BTINTEL_PCIE_CSR_BOOT_STAGE_REG);

To me it sounds strange, that a register name *boot stage* should change 
after the boot. ;-)

>   		if (dxstate == BTINTEL_PCIE_STATE_D0) {
>   			if (btintel_pcie_in_d0(data))
>   				return 0;


Kind regards,

Paul

Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

[Sergey Lebedev]

Paul,

Two of your three points I can answer from the hardware, since the
Tested-by on this patch is mine.

> Can you reproduce this on real hardware?

Yes. Surface Pro 11 (Intel, Lunar Lake), Intel BE201, 8086:a876 rev 10.
It first appeared in ordinary use, not in a test:

  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 returns -16
  PM: Some devices failed to suspend, or early wake event detected

One device returning -EBUSY aborts the whole system suspend, so the
machine does not sleep at all.

I have to be honest about the rate rather than useful: it failed on 2 of
the 3 suspends I attempted before installing a workaround, and later, on
a newer kernel with the workaround removed, it succeeded 7 times out of 7
with no btintel_pcie change in between. So the spontaneous failure is not
something I can produce on demand.

What I could do was exercise the exact path deterministically. I built
the driver out of tree with one debug-only parameter that returns from
btintel_pcie_msix_gp0_handler() before the boot_stage_cache refresh, and
only while alive_intr_ctxt is BTINTEL_PCIE_D0. That leaves the cache
stale and gp0_received false — precisely the state a genuinely missed
alive interrupt leaves behind — while the controller still reaches D3.

Without the patch, that reproduces the production failure exactly: three
timeouts, -EBUSY, suspend aborted. With the patch and nothing else
changed: one timeout, the re-read observes D3, set_dxstate() returns 0,
and the system suspends and resumes normally.

Full method and logs are in my earlier message on this thread,
Message-ID <20260830151550.44687-1-lsa.uz@pm.me>.

> To me it sounds strange, that a register name *boot stage* should
> change after the boot. ;-)

It is a fair reaction to the name, but the register genuinely carries
live power state. From the driver's own header:

  #define BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY  (BIT(24))

and both helpers test that one bit and nothing else:

  static bool btintel_pcie_in_d3(struct btintel_pcie_data *data)
  {
      return data->boot_stage_cache & BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY;
  }

  static bool btintel_pcie_in_d0(struct btintel_pcie_data *data)
  {
      return !(data->boot_stage_cache & BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY);
  }

So bit 24 is updated by the controller on every D-state transition, long
after boot. The name describes the register's original purpose rather
than everything Intel later put in it. btintel_pcie_resume() already
re-reads it for exactly this reason, which is what makes the patch's
change consistent rather than novel.

The re-flow is Vladimir's call and I have no opinion on it. If a v3 comes
out of this, I still have the hardware and the test fixture and will
re-run both halves against it.

Thanks,
Sergey

Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

[Paul Menzel]

Dear Sergey,


Thank you for your immediate response.

Am 02.09.26 um 09:24 schrieb Sergey Lebedev:

> Two of your three points I can answer from the hardware, since the
> Tested-by on this patch is mine.
> 
>> Can you reproduce this on real hardware?
> 
> Yes. Surface Pro 11 (Intel, Lunar Lake), Intel BE201, 8086:a876 rev 10.
> It first appeared in ordinary use, not in a test:
> 
>    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 returns -16
>    PM: Some devices failed to suspend, or early wake event detected
> 
> One device returning -EBUSY aborts the whole system suspend, so the
> machine does not sleep at all.
> 
> I have to be honest about the rate rather than useful: it failed on 2 of
> the 3 suspends I attempted before installing a workaround, and later, on
> a newer kernel with the workaround removed, it succeeded 7 times out of 7
> with no btintel_pcie change in between. So the spontaneous failure is not
> something I can produce on demand.
> 
> What I could do was exercise the exact path deterministically. I built
> the driver out of tree with one debug-only parameter that returns from
> btintel_pcie_msix_gp0_handler() before the boot_stage_cache refresh, and
> only while alive_intr_ctxt is BTINTEL_PCIE_D0. That leaves the cache
> stale and gp0_received false — precisely the state a genuinely missed
> alive interrupt leaves behind — while the controller still reaches D3.
> 
> Without the patch, that reproduces the production failure exactly: three
> timeouts, -EBUSY, suspend aborted. With the patch and nothing else
> changed: one timeout, the re-read observes D3, set_dxstate() returns 0,
> and the system suspends and resumes normally.
> 
> Full method and logs are in my earlier message on this thread,
> Message-ID <20260830151550.44687-1-lsa.uz@pm.me>.

It’d be great if the thread could be referenced via Link: tag, and also 
the problem summarized in the commit message including the log messages.

>> To me it sounds strange, that a register name *boot stage* should
>> change after the boot. ;-)
> 
> It is a fair reaction to the name, but the register genuinely carries
> live power state. From the driver's own header:
> 
>    #define BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY  (BIT(24))
> 
> and both helpers test that one bit and nothing else:
> 
>    static bool btintel_pcie_in_d3(struct btintel_pcie_data *data)
>    {
>        return data->boot_stage_cache & BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY;
>    }
> 
>    static bool btintel_pcie_in_d0(struct btintel_pcie_data *data)
>    {
>        return !(data->boot_stage_cache & BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY);
>    }
> 
> So bit 24 is updated by the controller on every D-state transition, long
> after boot. The name describes the register's original purpose rather
> than everything Intel later put in it. btintel_pcie_resume() already
> re-reads it for exactly this reason, which is what makes the patch's
> change consistent rather than novel.

Maybe a comment could be added above the macro definition.

> The re-flow is Vladimir's call and I have no opinion on it. If a v3 comes
> out of this, I still have the hardware and the test fixture and will
> re-run both halves against it.

Awesome.

Also note, gemini/gemini-3.1-pro-preview found the same issue in a 
different review [1].


Kind regards,

Paul


[1]: 
https://sashiko.dev/#/patchset/20260902042840.2432862-1-ravindra%40intel.com

Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

[Sergey Lebedev]

Paul,

Thank you for the pointer — it was the useful part of your mail.

Ravindra's patch changes the same function, and the two turn out to fix
different halves of one failure: his makes a *late* alive interrupt count
by moving the flag out of the retry loop, while Vladimir's makes a
*missing* one survivable, because the fallback check still reads the
stale boot_stage_cache. The hunks also sit on adjacent lines, so whichever
lands second will need a rebase.

I have followed that up in Ravindra's thread rather than here, since it
concerns his patch:

  Message-ID <20260902091021.20160-1-lsa.uz@pm.me>

There I report what our test fixture does under each patch, and offer to
assemble the two as a series if both authors would rather have it that way.

Your other two points — the Link: tag with a fuller commit message, and a
comment above the macro definition — are Vladimir's to act on, and I have
no opinion to add beyond agreeing they would help.

Thanks,
Sergey