All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sergey Lebedev <lsa.uz@pm.me>
To: Ravindra <ravindra@intel.com>
Cc: "Vladimir V . Kondratyev" <vladimirkondratyev2@gmail.com>,
	Paul Menzel <pmenzel@molgen.mpg.de>, Kiran K <kiran.k@intel.com>,
	Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>,
	Ravishankar Srivatsa <ravishankar.srivatsa@intel.com>,
	Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>,
	Marcel Holtmann <marcel@holtmann.org>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4
Date: Wed, 02 Sep 2026 09:10:29 +0000	[thread overview]
Message-ID: <20260902091021.20160-1-lsa.uz@pm.me> (raw)

Ravindra,

Paul Menzel pointed me at this patch from the thread on Vladimir
Kondratyev's stale-cache fix, since both change
btintel_pcie_set_dxstate(). They overlap textually and, more usefully,
they fix different halves of the same failure. I have the hardware and a
fixture for one of those halves, so here is what I can add.

Your change moves the flag out of the retry loop:

  +    data->gp0_received = false;
  +
       do {
  -        data->gp0_received = false;

That recovers the case where the alive interrupt is merely *late* — it
arrives while a later retry is waiting, and previously the per-iteration
reset threw it away. The comment you added states the invariant well.

What it does not cover is the interrupt being missed outright. When that
happens gp0_received never becomes true, the loop falls through to

      if (dxstate == BTINTEL_PCIE_STATE_D0) {
          if (btintel_pcie_in_d0(data))

and those helpers read data->boot_stage_cache, which only the interrupt
handler ever writes. No interrupt, no refresh: the fallback is asked to
decide using the value from before the transition, so it reports the old
state, every retry is exhausted, and -EBUSY aborts the suspend. Vladimir's
patch re-reads BTINTEL_PCIE_CSR_BOOT_STAGE_REG immediately before that
check, which is what btintel_pcie_resume() already does.

So the two are complementary rather than competing: yours makes a late
interrupt count, his makes a missing one survivable.

I can be concrete about the second half because I reproduced it
deliberately. Surface Pro 11 (Intel, Lunar Lake), Intel BE201, 8086:a876
rev 10. The driver was built out of tree with one debug-only parameter
that returns from btintel_pcie_msix_gp0_handler() before the
boot_stage_cache refresh, and only while alive_intr_ctxt is
BTINTEL_PCIE_D0 — the exact state a genuinely missed alive interrupt
leaves behind, with the controller still reaching D3.

Under that fixture, moving the flag out of the loop does not help: there
is no late interrupt to catch, and the cache stays stale. Three timeouts,
-EBUSY, suspend aborted. With the re-read, one timeout and the system
suspends. Method and logs are in
Message-ID <20260830151550.44687-1-lsa.uz@pm.me>.

The failure is not theoretical here. Before any workaround it aborted 2 of
the 3 suspends I attempted, and one device returning -EBUSY stops the
whole system from sleeping.

Whichever of the two lands first, the other will need a rebase — the
hunks touch adjacent lines. If it helps, I am happy to test them applied
together against my fixture and on ordinary suspend cycles, and to report
per-patch rather than as a combined result.

If you and Vladimir would prefer these as one series rather than two
independent patches, I am happy to do the assembly work — rebased, with
both of you as the authors of your respective halves and the Fixes: tags
kept separate, since they are separate bugs with separate reverts. I did
not want to send that uninvited over two patches that are already on the
list; say the word and it is a short job.

One question on your patch, from someone who does not have S4 on this
machine to check: .thaw now takes the normal D0 path, while .restore
forces FLR. After PM_EVENT_FREEZE the controller was put into D3_COLD by
btintel_pcie_suspend_late(). Is D3_COLD guaranteed to preserve firmware
across a freeze/thaw on this part, or does thaw also need the FLR path?

Thanks,
Sergey


             reply	other threads:[~2026-09-02  9:10 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-02  9:10 Sergey Lebedev [this message]
2026-09-02 13:38 ` [PATCH v2] Bluetooth: btintel_pcie: fix PM flow for S0ix, S3 and S4 Sergey Lebedev
2026-09-08  9:32 ` Ravindra
2026-09-08 10:28   ` Sergey Lebedev
  -- strict thread matches above, loose matches on Subject: below --
2026-09-09 12:36 Sergey Lebedev
2026-09-02  4:28 Ravindra
2026-09-02  7:01 ` Paul Menzel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260902091021.20160-1-lsa.uz@pm.me \
    --to=lsa.uz@pm.me \
    --cc=chandrashekar.devegowda@intel.com \
    --cc=chethan.tumkur.narayan@intel.com \
    --cc=kiran.k@intel.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=marcel@holtmann.org \
    --cc=pmenzel@molgen.mpg.de \
    --cc=ravindra@intel.com \
    --cc=ravishankar.srivatsa@intel.com \
    --cc=vladimirkondratyev2@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.