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

[Ravindra]

Hi Sergey,

Thanks for the careful re-read and for the measurements. I’ve updated the v4
series to address the four points in your review.

Patch 3 now treats `gp0_received` as evidence that the handler ran, not that
the transition completed. It refreshes the hardware state before accepting
success, keeps the flag reset inside the retry loop, completes the
mailbox/alive handshake when restoring the D0 path, propagates
`btintel_pcie_start_rx()` errors, and restores the D3 context where needed. I
also used your revised comments.

To answer your authorship question explicitly: patches 1 and 2 remain
separate commits. Patch 1 retains Vladimir’s `From:` and `Signed-off-by:`,
along with the existing `Link:` tags and your `Tested-by:` and
`Signed-off-by:`. I also updated its `Fixes:` tag to `88c6216a52ea`
(“Bluetooth: btintel_pcie: Suspend/Resume: Controller doorbell interrupt
handling”). Patch 3 has you as `From:` author, with both `Signed-off-by:`
trailers. Patch 4 remains authored and signed off by me.

Patch 4 uses a direct W1C write for GP0. The complete four-patch series
applies cleanly, and the local build and strict checkpatch validation pass.
I haven’t independently run your forced-path bench, so I appreciate your
offer to test this v4 against it.

I am sending the v4 series which includes all 4 patches as part of it.

Best Regards,
Ravindra

> -----Original Message-----
> From: Sergey Lebedev <lsa.uz@xxxxx>
> Sent: Friday, September 25, 2026 12:43 AM
> To: Ravindra <ravindra@xxxxxxxxx>; Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx>; Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Cc: Vladimir V . Kondratyev <vladimirkondratyev2@xxxxxxxxx>; Ferenc
> Lengyel <dev@xxxxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; Paul Menzel
> <pmenzel@xxxxxxxxxxxxx>; K, Kiran <kiran.k@xxxxxxxxx>; Devegowda,
> Chandrashekar <chandrashekar.devegowda@xxxxxxxxx>; Chambarakatta,
> Mahalingeshwara <mahalingeshwara.chambarakatta@xxxxxxxxx>; Arnd
> Bergmann <arnd@xxxxxxxx>; linux-bluetooth@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and
> S4
> 
> Ravindra,
> 
> Thank you - that settles it. I am not sending a v3; your v4 is the right vehicle,
> and I will not put a second copy of one fix in flight.
> 
> But do not take the third patch as posted. Re-reading it against your answer
> turned up four things, three of them mine. Line numbers are from bluetooth-
> next master at 671d566d3c3b.
> 
> 1. The comment is wrong, and your answer is what makes it wrong.
> 
> You are right that error and lockdown are filtered at 1958 and 1965, before
> the flag is set. My comment says the flag is raised for three causes and the
> switch may match none of them. Carried as written, v4 would fix the old
> misguiding comment and import the same fault in a new one.
> 
> The patch is still needed, for a reason the comment does not give:
> 
>   1971  data->gp0_received = true;
>   2014  case BTINTEL_PCIE_D3:
>   2015          if (btintel_pcie_in_d0(data)) {
>    ..              ctxt = D0, submit_rx, signal_waitq
>   2020          }
>   2021          break;          <- matched, changed nothing
> 
> A matched case can complete having done nothing, with the flag already true
> and signal_waitq clear. So it is not about which cause raised the interrupt. That
> also means "the only scenario is a missed interrupt" is too narrow: here the
> interrupt arrives and is handled, to no effect.
> 
> And it bites without any missed interrupt. Measured on the part rather than
> argued, with a debug copy of the module: the handler's D3 case is made to
> read in_d0() as false so it takes its own 2021 break, a 2 ms sleep before the
> wait widens a window that is already there, and the early return is a module
> parameter so stock and patched are one build.
> The SP11 lines below are that instrumentation; everything else is the driver's
> own.
> 
>   stock (early return kept):
>     SP11: handler forced down the do-nothing break
>     SP11: early return, dx=0 ctxt=6 after 204902 us
>     Bluetooth: hci0: Received hw exception interrupt
>     btintel_pcie 0000:00:14.7: resetting
>     Bluetooth: hci0: command 0x0c01 tx timeout
>     Bluetooth: hci0: Opcode 0x0c01 failed: -110
>     Bluetooth: hci0: Opcode 0x0c1a failed: -110
> 
>   with the patch:
>     SP11: handler forced down the do-nothing break
>     SP11: verified path re-armed RX
>     SP11: ok dx=0 ctxt=5 waits=1 nosleep=1 2133 us
> 
> ctxt 6 is D3 and 5 is D0, so 4213 returned success with the context still saying
> D3 and the rings unarmed. Note the 204902 us: signal_waitq is clear on that
> path, so nothing woke the queue - the wait ran its full 200 ms, re-checked the
> condition at expiry, found the flag true and returned non-zero. The success
> was believed on a flag set by a handler run that had done nothing and had not
> even woken anyone.
> 
> One caveat on that pair, since it is a race and I would rather you heard it from
> me. An earlier attempt suppressed the handler once instead, and the stock
> side came back clean - a later handler run had repaired the state before
> anything used it. So the window is real and reachable but not hit every time.
> The older fixture, which injected a return between
> 1971 and the switch rather than taking the 2021 path, failed 3 of 3 and was
> clean 3 of 3 with the fix; that is where the reproducibility comes from, and this
> run is what ties it to the real path.
> 
> 2. Keeping gp0_received inside the loop is not optional once this patch is in,
> which is a second reason for the decision you already made.
> 
> With the flag hoisted out of the loop and "if (status) return 0" gone, once any
> handler run has set it - the 2021 case is one - the later retries find it still true,
> wait_event_timeout returns at once, and each retry becomes a bare register
> read. Three 200 ms attempts collapse into microseconds and the function
> returns -EBUSY without ever having waited.
> A genuinely missed interrupt leaves the flag false and still waits, so this only
> shows up in exactly the case the patch is for.
> 
> Measured the same way, on the suspend direction so a failure only aborts the
> suspend, with the state check forced to fail all three times:
> 
>     hoisted:      waits=3  nosleep=2  total    1465 us   (and 1143 us)
>     in the loop:  waits=3  nosleep=0  total  622445 us   (and 411499 us)
> 
> with the three "Timeout (200 ms)" lines present only in the second. The first
> pair ran back to back, which left the second configuration facing a controller
> the first had put in D3, so it was not a control; the figures in brackets are a
> rerun in the opposite order with a clean, unforced suspend/resume before
> each. The shape held both ways.
> 
> Your v4 resets it inside the loop, so this does not arise - but the two changes
> are coupled and nothing says so.
> 
> 3. The patch's D0 branch is an incomplete substitute for the handler.
> 
> The handler's submit_rx block does three things, not two: reset_ia, start_rx,
> and the mbox<->alive handshake at 2038-2041. Mine does the first two and
> claims in its comment to do what the branch would have.
> If MBOX_PARSE_PENDING is set, the waiter at 1615 then sits out its timeout
> for nothing.
> 
> 4. start_rx() can fail and its return is discarded.
> 
> The handler is void and has no choice; set_dxstate() returns a status to the
> resume path and does. Reporting 0 with the rings unarmed is the exact
> condition this patch exists to prevent.
> 
> Rolled up, with the comment rewritten (it needs an int err; beside the existing
> retry and status):
> 
>   		if (dxstate == BTINTEL_PCIE_STATE_D0) {
>   			if (btintel_pcie_in_d0(data)) {
>  +				/* Do what the handler's D3 -> D0 branch
>  +				 * would have done, unless it already has.
>  +				 */
>   				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);
>  +				err = btintel_pcie_start_rx(data);
>  +				if (err)
>  +					return err;
>  +
>  +				/* Complete the mbox<->alive handshake */
>  +				if
> (test_and_clear_bit(BTINTEL_PCIE_MBOX_PARSE_PENDING,
>  +						       &data->flags)) {
>  +
> 	set_bit(BTINTEL_PCIE_MBOX_PARSE_READY,
>  +						&data->flags);
>  +					wake_up(&data-
> >mbox_parse_wait_q);
>  +				}
>   				return 0;
>   			}
> 
> and the comment above the register read:
> 
>   /* gp0_received is set at the top of the handler, before the switch on
>    * alive_intr_ctxt. Error and lockdown are filtered out above it, but a
>    * matched case can still complete without doing anything - D3 breaks
>    * unchanged while the controller has not reached D0 - and a hardware
>    * bug may drop the interrupt outright. Either way the flag says a gp0
>    * was handled, not that the transition completed, and only the register
>    * knows. Refresh the cache here and retry only if the state check still
>    * fails.
>    */
> 
> The D3 branch needs no such guard: setting alive_intr_ctxt to D3 twice has no
> second effect, where reset_ia and start_rx do.
> 
> On your 4/4, for what it is worth from here: clr_reg_bits() is a read-modify-
> write, so on a W1C register it writes 0 to GP0 - not clearing it - and 1 to every
> other cause that happened to be pending, acking HWEXP, GP1 or FWTRIG
> before the ISR sees them. The ISR's own idiom is the
> proof: 2712 reads the HW causes and 2716 clears them by writing the same
> value back, under the comment that says so.
> 
> The one thing I need from you is question 1's other half.
> 
> Please keep Vladimir V. Kondratyev's From: and Signed-off-by on 1/2. His one
> condition for letting me carry his patch was authorship credit, and I relayed
> that to the list with the assurance that it was already the case.
> As posted, 1/2 carries his From: at the top, his Signed-off-by first and his two
> Link: tags, with my Tested-by and Signed-off-by beneath:
> 
>   https://lore.kernel.org/linux-bluetooth/20260909123416.71919-2-
> lsa.uz@xxxxx/
> 
> Your answer explains the ambiguous diff as a local squash, which I read as
> 1/2 staying its own commit - but you did not say so, and it is his name on it
> rather than mine, so I would rather ask than infer.
> 
> Same rule for the third patch, whichever form you prefer: Co-developed-by:
> with the Signed-off-by, or From:. A bare sign-off records the chain but not the
> author.
> 
> On question 2, your answer is enough whichever way that window falls. If a
> handler is in flight while set_dxstate() reads the context, the guard simply does
> not fire and we are back to the behaviour without it. It can help and cannot
> hurt, so it never needed the window to be impossible.
> 
> The three patches apply to 671d566d3c3b with no fuzz. So does the rolled-up
> version above, which is not just quoted at you: applied on top of them it
> builds W=1 with sparse, zero warnings, and checkpatch --strict gives
> 0 errors, 0 warnings, 0 checks over 195 lines.
> 
> The bench for the measurements: kernel 7.2.0-rc6-btnext-norework built
> from e40edfa04, where set_dxstate() and the gp0 handler are byte-identical
> to 671d566d3c3b - only their line numbers moved, 4133 against 4200 - on a
> BE201 8086:a876 rev 10.
> 
> Send v4 when it is ready and I will put it through the same bench.
> 
> Sergey