RE: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

[Ravindra]

Hi Sergey,

Thanks for the detailed investigation and write-up.

I agree with the issue arising from not updating the alive interrupt
context, as well as the incorrect (read-modify-write) clear of the GP0
cause bit. I've folded both fixes into the PM-flow patch as a single
commit and tested 50 suspend/resume (S4) cycles on an NVL Linux platform -
no issues seen across all 50 runs.

I'm fine either way on how this lands: I can push the combined patch
myself, or if you'd prefer to carry it and push it upstream directly,
that works for me too.

Diff:

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 1816c81c4721..9d4f0bdf992b 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -5193,21 +5193,35 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
                "Timeout (%u ms) on alive interrupt for D%d entry, retry count %d",
                dx_intr_timeout_ms, dxstate, retry);
 
-		/* clear gp0 cause */
-		btintel_pcie_clr_reg_bits(data,
-					  BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
-					  BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
+		/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so write only
+		 * this bit to avoid acking other pending causes
+		 */
+		btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES,
+				      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.
+		/* A hardware bug may cause the alive interrupt to be missed. Refresh
+		 * boot_stage_cache from hardware, since only the interrupt handler
+		 * updates it. Finally retry only if the state check still fails.
          */
+		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))
+			if (btintel_pcie_in_d0(data)) {
+				/* GP0 handler never ran to do this: keep the
+				 * state tracker in sync and resubmit RX.
+				 */
+				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)) {
+				/* GP0 handler never ran to do this. */
+				data->alive_intr_ctxt = BTINTEL_PCIE_D3;
                 return 0;
+			}
         }
 
     } while (++retry < BTINTEL_PCIE_DX_TRANSITION_MAX_RETRIES);

Per Paul's comments, I've also updated the commit message:

Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4

Fix two issues in the PM suspend/resume path:

1. S3 was handled the same as S0ix, keeping the controller on the
   D3hot-style path. That caused resume instability because S3 can
   remove power from the controller, unlike s2idle/S0ix. Use
   pm_suspend_target_state to distinguish them: D3_HOT for S0ix and
   D3_COLD for S3/S4. Add .restore to force FLR-based firmware recovery
   after S4 and S3 (PM_SUSPEND_MEM), as power is lost. S0ix resumes via
   a normal D0 transition.

2. During hibernation, .freeze() puts the controller into the D3cold
   state without any loss of power, and the flow normally continues to
   .poweroff(). If hibernation instead fails, .thaw() is called to bring
   the controller back up, and the old code routed it through
   btintel_pcie_resume(), which forced FLR-based firmware recovery
   whenever data->pm_sx_event was PM_EVENT_FREEZE. That check was
   incorrect: since the controller's power was never actually removed on
   this failed-hibernation path, FLR-based recovery is unnecessary. Remove
   pm_sx_event and route .thaw through a normal D0 transition instead;
   FLR-based recovery is retained only in .restore, where genuine S4
   power loss requires it.

The Sx debug logging (debug_mask & BTINTEL_PCIE_LOG_SX) is kept and
adapted to use pm_message_t.event instead of the removed pm_sx_event
field.

Tested with:
  S0ix: sudo sh -c 'echo "+40" > /sys/class/rtc/rtc0/wakealarm' && \
        echo freeze | sudo tee /sys/power/state
  S3:   sudo rtcwake -m mem  -s 60
  S4:   sudo rtcwake -m disk -s 60

Fixes: e57362f4911b ("Bluetooth: btintel_pcie: Add support for _suspend() / _resume()")
Signed-off-by: Ravindra <ravindra@xxxxxxxxx>

Thanks,
Ravindra
> 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