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