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

[Paul Menzel <pmenzel@molgen.mpg.de>]

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