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

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

Thank you - and for taking the third finding, which I had written off as
not this series' business.

Where this stands, and what I need from you
===========================================

A v3 is assembled and ready: 1/2 and 2/2 unchanged, plus a third patch for
the one path the fold does not close. It applies to today's bluetooth-next
with no adjustment, checkpatch --strict gives 0 errors and 0 checks, and the
third patch is measured rather than argued - 3 of 3 injected cycles failed
without it, 3 of 3 were clean with it, plus four uninjected controls.

I am not sending it. Four things are not mine to settle, and I would rather
ask than guess:

  1. Is 1/2 still a separate commit in your tree? The diff you sent reads
     either way, and the answer decides whether v3 is three patches or two.

  2. Can btintel_pcie_set_dxstate() race the gp0 handler? The new patch
     guards on alive_intr_ctxt so that RX is not armed twice, and that guard
     is only sound if it cannot. You know that path; I have only read it.

  3. Which tree is your hunk against? It sits at 5193, while bluetooth-next
     has set_dxstate() at 4169 and nothing inside it has moved since the
     series went out.

  4. Where do you want the W1C clear? It is your fix and pre-existing, and
     neither patch touched that line, so it reads better as its own commit
     with its own Fixes: than inside a PM patch. Say the word and v3 carries
     it as a fourth patch with your Signed-off-by; say nothing and I leave
     it out for you to send.

An answer to any of them - or "hold, I will carry it myself" - and I will
act on it the same day. Everything below is the evidence behind them.

Whose commit 1/2 is
===================

The diff applies to a tree without 1/2: its minus side carries the old
comment, its plus side carries Vladimir's re-read word for word. So either
the two commits are now one, or it was taken against the pre-series base.
I cannot tell from here.

If they are one, please keep his From: and Signed-off-by, or add
Co-developed-by: and Signed-off-by: if it has to stay a single commit. His
one condition for letting me carry his patch was authorship, and I answered
him on-list that it was already safe - so this part is mine to ask about
rather than his. His two Link: tags are worth keeping either way.

Ferenc Lengyel's Tested-by is a different matter: it was given for the patch
as posted, so dropping it from a modified one is fair. He is on Cc, so I am
saying it here rather than letting it go quietly.

The flag means less than it says
================================

I argued the opposite to you on 9 September - that moving gp0_received out
of the retry loop rescues a late interrupt, and that your comment stated the
invariant well. That was wrong, and I would rather say so than switch sides
quietly.

data->gp0_received = true is set at 1968 unconditionally, before a switch
that may match no case at all. The comment at 1940 says the same thing from
the other side: gp0 is raised for three causes and it is not easy to know
which one fired. So the flag means a gp0 arrived, not that the controller
moved.

"if (status) return 0;" at 4182 has no register read behind it. Together
those two let set_dxstate() report success while alive_intr_ctxt is stale
and the RX rings were never re-armed.

That needs no late interrupt and no second attempt: it happens inside one
pass, and it is in bluetooth-next today. Moving the flag out of the loop
widens it to a later attempt as well. The fold closes neither, because it
sits on the timeout path and this returns above it.

Measured
========

Fixture: the gp0 handler returns after data->gp0_received = true and before
the switch, once, while alive_intr_ctxt is D3. That is not a delayed
interrupt - it is what a gp0 that matches no case does on its own. s2idle,
rtcwake -m freeze -s 45, SP11, BE201 8086:a876 rev 10, kernel 7.0.0-30.

  build                                     injected    result
  ---------------------------------------------------------------
  set_dxstate() byte-identical to bt-next    3 of 3     failed
  1/2 + 2/2 + your fold                      3 of 3     failed
  the same plus the diff below               3 of 3     clean
  the same, no injection                     4 of 4     clean

Failure is the same every time:

    Bluetooth: hci0: Received hw exception interrupt
    Bluetooth: hci0: Controller in error state
    Bluetooth: hci0: command 0x0c01 tx timeout
    Bluetooth: hci0: Opcode 0x0c1a failed: -110
    btintel_pcie 0000:00:14.7: resetting

and the controller returns under a new hci index, so every paired device is
gone. In one of the six the FLR did not recover it either: it came back at
00:00:00:00:00:00 and needed btintel_pcie and btintel both unloaded.

The part I did not expect is how quiet it is. "Timeout (200 ms) on alive
interrupt" appears in none of the six, because the function returns above
it. Nothing in the log says the resume went wrong - the first sign is the
firmware exception a second later.

The diff
========

This is v3's third patch, shown against your folded version so the change is
visible rather than the whole function. It moves the verification above the
success return rather than adding anything, and does the handler's work only
when the handler has not.

  @@ -1,31 +1,36 @@
   		status = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
   			msecs_to_jiffies(dx_intr_timeout_ms));

  -		if (status)
  -			return 0;
  +		if (!status) {
  +			bt_dev_warn(data->hdev,
  +				   "Timeout (%u ms) on alive interrupt for D%d entry, retry count %d",
  +				   dx_intr_timeout_ms, dxstate, retry);

  -		bt_dev_warn(data->hdev,
  -			   "Timeout (%u ms) on alive interrupt for D%d entry, retry count %d",
  -			   dx_intr_timeout_ms, dxstate, retry);
  +			/* 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);
  +		}

  -		/* clear gp0 cause; MSIX_HW_INT_CAUSES is W1C, so write only
  -		 * this bit to avoid acking other pending causes
  +		/* gp0_received only says a gp0 arrived. It is raised for three
  +		 * causes and the handler sets the flag before a switch that may
  +		 * match none of them, so it does not mean the target state was
  +		 * reached; a hardware bug may also drop the interrupt outright.
  +		 * Either way only the register knows, and only the handler
  +		 * refreshes the cache. Refresh it here and retry only if the
  +		 * state check still fails.
   		 */
  -		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. 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)) {
  -				/* GP0 handler never ran to do this: keep the
  -				 * state tracker in sync and resubmit RX.
  +				/* Do what the handler's D3 -> D0 branch would
  +				 * have done, unless it already did it.
   				 */
  +				if (data->alive_intr_ctxt == BTINTEL_PCIE_D0)
  +					return 0;
   				data->alive_intr_ctxt = BTINTEL_PCIE_D0;
   				btintel_pcie_reset_ia(data);
   				btintel_pcie_start_rx(data);

That guard - "if alive_intr_ctxt is already D0, the handler did it" - is
question 2 above. The handler is threaded (2779) and the gp0 dispatch at
2736 runs after irq_lock is dropped at 2697, so it can set gp0_received on
one CPU and reach submit_rx at 2030 while set_dxstate() is reading the
context on another. If that window is real the guard is wrong, and the rest
of the patch stands without it. It bears on your fold the same way.

Fifty clean S4 cycles cannot reach any of this - the path runs only when the
alive interrupt is missing or unactionable, so a healthy controller never
enters it. That makes your run a no-regression result on the normal path,
which is worth having. Send me whatever you settle on and I will put it
through the same fixture.

Whichever way this goes, please post it to the list rather than pushing it
straight - then Vladimir, Ferenc and Luiz all see the trailers before it
lands.

Thanks,
Sergey