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

[Sergey Lebedev <lsa.uz@pm.me>]

I said in my earlier mail that these two patches fix different halves of
one failure, and offered to test them. Here is the measurement rather
than the argument.

Hardware: Surface Pro 11 (Intel, Lunar Lake), Intel BE201, 8086:a876
rev 10, kernel 7.0.0-30, s2idle. Four builds of btintel_pcie from the
same source, differing only in which hunk is present, each carrying one
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 is a missed alive
interrupt: gp0_received never set, cache never refreshed, controller
still reaching D3. Suspend triggered with rtcwake -m freeze -s 45.

  build                          result
  ---------------------------------------------------------------
  neither hunk                   3 timeouts, -EBUSY, suspend aborted
  Ravindra's hunk only           3 timeouts, -EBUSY, suspend aborted
  Vladimir's hunk only           1 timeout, suspended and resumed
  both hunks                     1 timeout, suspended and resumed

Unpatched and with your hunk alone, identical:

  PM: suspend entry (s2idle)
  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: failed to suspend async: error -16
  PM: Some devices failed to suspend, or early wake event detected
  PM: suspend exit          <- 1.7 s after entry; it never slept

With the re-read present, alone or alongside your hunk:

  PM: suspend entry (s2idle)
  Bluetooth: hci0: Timeout (200 ms) on alive interrupt for D2 entry, retry count 0
  PM: suspend exit          <- 48.8 s after entry; it slept the full period

This is not a criticism of your patch: the fixture removes the interrupt
entirely, so there is no late interrupt for your hunk to rescue, and it
cannot help by construction. It measures the half it does not cover, and
confirms the two do not interfere when applied together.

The single-run table above had one oddity - the both-hunks run slept 8.6 s
instead of the full 45 - so I repeated it rather than leave a guess in the
archive. Eight further cycles, no failures anywhere:

  both hunks        48.0  48.6  50.8  48.6  50.7 s
  re-read only      48.6  46.6  50.8 s

The 8.6 s did not recur. It was a stray wake, not a property of the
combination.

Those runs turned up something better than the caveat they removed. The
retry count varies between 1 and 3 from run to run, and the outcome does
not: even when all three retries time out, the re-read then observes D3
and set_dxstate() returns 0. One of the re-read-only runs did exactly
that - three timeouts, no -EBUSY, slept 46.6 s - which is the fallback
doing the whole job with the retry loop having contributed nothing.

One incidental confirmation, since Paul asked in the other thread whether
a register called *boot stage* really changes after boot. Tracing the
fallback path prints what it actually reads at D3 entry:

  SP11TRACE: retry=0 dxstate=2 gp0_received=0 boot_stage=0x61710007

Bit 24 - BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY - is set, on hardware,
long after boot, which is why the re-read is able to answer the question
the cached value cannot.

One caveat I cannot remove. I have no fixture for the late-interrupt case
your hunk addresses, so I have measured only that it does no harm here,
not that it works. That needs a different fixture - delaying the interrupt
rather than dropping it - and I have not built one.

Raw log and the four builds are available if anyone wants them.

While instrumenting this I caught the failure happening on its own, with the
injection disabled, which I had not managed before. One run in six:

  PM: suspend entry (s2idle)
  Timeout (200 ms) on alive interrupt for D2 entry, retry count 0
  SP11TRACE: retry=0 dxstate=2 gp0_received=0 boot_stage=0x61511007
  Timeout (200 ms) on alive interrupt for D0 entry, retry count 0
  SP11TRACE: retry=0 dxstate=0 gp0_received=0 boot_stage=0xa0db1007
  Timeout (3000 ms) on alive interrupt, alive context: intel_reset1

The interrupt went missing in both directions in the same cycle, and the
re-read rescued both: bit 24 set at D3 entry, clear at D0 entry, each time
matching the state the controller had actually reached. No -EBUSY. So this
is not only a fixture artefact - the hardware does drop it, and the register
is right when the interrupt is not.

That leads to a question rather than a patch, for whoever owns this code.

Both patches keep the interrupt as the primary signal and the register as a
fallback. Given that the driver's own comment calls the missed interrupt a
hardware bug, and given the register answers correctly in both directions,
would it not be sounder to invert that - poll BOOT_STAGE_REG for the target
bit, and treat the interrupt as an early-exit optimisation rather than the
thing being waited on?

The file already does exactly this shape for the reset path:

  do {
      reg = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
      if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_STS)
          break;
      usleep_range(10000, 12000);
  } while (--retry > 0);

and btintel_pcie.c declares POLL_INTERVAL_US, which nothing uses - so the
intent seems to have existed at some point.

The practical difference is not only correctness. Today a missed interrupt
costs up to 600 ms of waiting per transition before the fallback is even
consulted; a poll would return as soon as the bit flips. And the stale-cache
class of bug disappears rather than being caught.

I am not proposing this as a patch over yours - you two own this code and I
have one machine. But if either of you thinks the shape is right, I have the
hardware, the fixture and now a spontaneous reproduction to test it against.

Vladimir has said he is content either way on the series question, so it
rests with you and the maintainers. My offer to do the assembly stands.

Thanks,
Sergey