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

[Ravindra]

[Removed ravishankar.srivatsa@xxxxxxxxx]

Hi Sergey,

> Subject: Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and
> S4
> 
> 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.
   One clarification on the diff you saw: for my own testing I
   temporarily squashed everything (1/2 + 2/2 + your fold) into a
   single working commit. That is almost certainly why the diff
   read ambiguously. The squash was local-only and never intended
   for posting.

>   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.
   Not in the way the fold's guard needs to worry about. GP0 is
   raised for a specific, known set of causes, and a D0<->D3
   transition is one of them. set_dxstate() waits 200 ms for that
   interrupt. If it has not arrived within that window, a missed
   interrupt is far more likely than a merely delayed one, since a
   healthy D0<->D3 transition completes well inside that budget.

   So when the timeout fires and the boot stage register already
   shows the target state, that is read as ground truth: the
   handler almost certainly never ran for this transition, so the
   driver re-arms the RX rings itself. There is no race with
   gp0_handler() here, because the register check and the re-arm
   only happen after the wait has already timed out; nothing about
   this path invokes or waits on a second, concurrent handler
   execution.

>   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.
   That hunk came from an internal kernel tree carrying some
   additional, unrelated changes, which is why the line numbers
   do not line up with bluetooth-next. Sorry for the inconvenience
   caused. v4 will be rebased and diffed directly against today's
   bluetooth-next/master so the line numbers match what you see
   there.

>   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.
   I will include it as 4/4 in v4, as its own commit with its own
   Fixes: tag and my Signed-off-by.

> 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.

On moving data->gp0_received outside the retry loop:

That change assumed a late interrupt could still arrive after a
timed-out attempt, so keeping the flag armed across retries would
let it be caught. In practice that case does not occur: the only
scenario is a missed interrupt (never sent, not late), which the
existing register readback already handles on each retry. So there
is no benefit to relocating the flag, and I will keep it exactly as
it is in bluetooth-next today, reset inside the loop before each
sleep_cntrl write.

> data->gp0_received = true is set at 1968 unconditionally, before a
> data->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.

That comment looks misguiding. Will fix it in the next v4 series.
alive_intr_ctxt records the reason the driver is waiting for the
alive interrupt, not an unconstrained guess. Before the switch even
runs, the top of the gp0 handler already checks the error and
lockdown states and returns early on either, so gp0 arriving for a
genuinely unexpected/error reason is filtered out before
gp0_received is ever set. By the time gp0_received = true is
reached, the interrupt has already been qualified as a normal
boot-stage transition, not an arbitrary "any cause" signal as the
comment implies.

> "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.

You are correct. In the interrupt-miss case, both
alive_intr_ctxt and the RX rings need to be updated. Otherwise,
set_dxstate() may report success while operating on stale state.

I will include the fix you provided, along with your Signed-off-by,
in the next revision.

> 
> 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 for the suggestion. I will post the mailing list.

> Thanks,
> Sergey

Thanks,
Ravindra