1. Patchew
  2. linux
  3. View series

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

Vladimir V. Kondratyev posted 1 patch 3 weeks, 3 days ago
There is a newer version of this series
drivers/bluetooth/btintel_pcie.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
[PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check
Posted by Vladimir V. Kondratyev 3 weeks, 3 days ago 
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
Posted by Paul Menzel 3 weeks, 3 days ago 
[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
Posted by Sergey Lebedev 3 weeks, 3 days ago 
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
Posted by Paul Menzel 3 weeks, 3 days ago 
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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.