* [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping
@ 2026-10-12 11:42 Michal Clapinski
2026-10-12 11:51 ` sashiko-bot
0 siblings, 1 reply; 3+ messages in thread
From: Michal Clapinski @ 2026-10-12 11:42 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci
Cc: Lukas Wunner, linux-kernel, David Matlack, Pasha Tatashin,
Michal Clapinski
doe_statemachine_work() waits for a response by sleeping
PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128) between status reads,
which is 7 ms at HZ=1000 and 4 ms at HZ=250. Exchanges are served from
the device's local mailbox logic and typically complete within tens of
microseconds, so nearly all of that sleep is wasted. Every DOE-capable
endpoint pays it once per supported feature during enumeration.
Poll with usleep_range(20, 50) for the first ~2 ms before falling back
to the jiffies-based wait. On an Intel Emerald Rapids system, time spent
in pci_device_add() for a DOE-capable endpoint drops from 41.8 ms to
14.0 ms.
Signed-off-by: Michal Clapinski <mclapinski@google.com>
---
I was asked if I can do this with interrupts instead. I don't see a way
to do that during device enumeration that won't collide with the
drivers.
---
drivers/pci/doe.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 6b5d9ba8e969..5e93d78a1a97 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -27,6 +27,9 @@
/* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
#define PCI_DOE_TIMEOUT HZ
#define PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128)
+#define PCI_DOE_POLL_FAST_MIN_US 20
+#define PCI_DOE_POLL_FAST_MAX_US 50
+#define PCI_DOE_POLL_FAST_TRIES 40
#define PCI_DOE_FLAG_CANCEL 0
#define PCI_DOE_FLAG_DEAD 1
@@ -487,7 +490,7 @@ static void doe_statemachine_work(struct work_struct *work)
int offset = doe_mb->cap_offset;
unsigned long timeout_jiffies;
u32 val;
- int rc;
+ int rc, tries = 0;
if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) {
signal_task_complete(task, -EIO);
@@ -525,10 +528,25 @@ static void doe_statemachine_work(struct work_struct *work)
signal_task_abort(task, -EIO);
return;
}
- rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
- if (rc) {
- signal_task_abort(task, rc);
- return;
+ /*
+ * Exchanges are served from the device's local mailbox
+ * logic and usually complete in tens of microseconds, far
+ * below the jiffies granularity of PCI_DOE_POLL_INTERVAL.
+ * Poll for up to ~2 ms before falling back to it.
+ */
+ if (tries++ < PCI_DOE_POLL_FAST_TRIES) {
+ if (test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags)) {
+ signal_task_abort(task, -EIO);
+ return;
+ }
+ usleep_range(PCI_DOE_POLL_FAST_MIN_US,
+ PCI_DOE_POLL_FAST_MAX_US);
+ } else {
+ rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
+ if (rc) {
+ signal_task_abort(task, rc);
+ return;
+ }
}
goto retry_resp;
}
--
2.56.0.385.gd3acb90ef8-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping
2026-10-12 11:42 [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping Michal Clapinski
@ 2026-10-12 11:51 ` sashiko-bot
2026-10-12 11:53 ` Michał Cłapiński
0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-10-12 11:51 UTC (permalink / raw)
To: Michal Clapinski; +Cc: linux-pci
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci/doe: CPU busy loop when HZ=100 due to PCI_DOE_POLL_INTERVAL=0
--
commit 16e5277536bbca37386848082d18257e2ffa71dd
Author: Michal Clapinski <mclapinski@google.com>
PCI/DOE: Poll briefly for a response before sleeping
doe_statemachine_work() waits for a response by sleeping
PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128) between status reads,
which is 7 ms at HZ=1000 and 4 ms at HZ=250. Exchanges are served from
the device's local mailbox logic and typically complete within tens of
microseconds, so nearly all of that sleep is wasted. Every DOE-capable
endpoint pays it once per supported feature during enumeration.
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 6b5d9ba8e9697..5e93d78a1a97e 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -27,6 +27,9 @@
> /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
> #define PCI_DOE_TIMEOUT HZ
> #define PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128)
[Severity: Medium]
When HZ=100, does this integer division truncate to 0?
If PCI_DOE_POLL_INTERVAL evaluates to 0, it permanently breaks the fallback
sleep mechanism.
> +#define PCI_DOE_POLL_FAST_MIN_US 20
> +#define PCI_DOE_POLL_FAST_MAX_US 50
> +#define PCI_DOE_POLL_FAST_TRIES 40
[ ... ]
> @@ -525,10 +528,25 @@ static void doe_statemachine_work(struct work_struct *work)
> signal_task_abort(task, -EIO);
> return;
> }
> - rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> - if (rc) {
> - signal_task_abort(task, rc);
> - return;
> + /*
> + * Exchanges are served from the device's local mailbox
> + * logic and usually complete in tens of microseconds, far
> + * below the jiffies granularity of PCI_DOE_POLL_INTERVAL.
> + * Poll for up to ~2 ms before falling back to it.
> + */
> + if (tries++ < PCI_DOE_POLL_FAST_TRIES) {
> + if (test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags)) {
> + signal_task_abort(task, -EIO);
> + return;
> + }
> + usleep_range(PCI_DOE_POLL_FAST_MIN_US,
> + PCI_DOE_POLL_FAST_MAX_US);
> + } else {
> + rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
[Severity: Medium]
Does passing 0 to this wait function when HZ=100 trigger a tight spin bug
instead of yielding the CPU?
Since wait_event_timeout() immediately returns without sleeping when the
timeout is 0, this regression results in a tight CPU busy loop spinning over
pci_read_config_dword() continuously for up to 1 second during enumeration
for affected hardware.
> + if (rc) {
> + signal_task_abort(task, rc);
> + return;
> + }
> }
> goto retry_resp;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114213.1757207-1-mclapinski@google.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping
2026-10-12 11:51 ` sashiko-bot
@ 2026-10-12 11:53 ` Michał Cłapiński
0 siblings, 0 replies; 3+ messages in thread
From: Michał Cłapiński @ 2026-10-12 11:53 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-pci
On Mon, Oct 12, 2026 at 1:51 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] pci/doe: CPU busy loop when HZ=100 due to PCI_DOE_POLL_INTERVAL=0
> --
>
> commit 16e5277536bbca37386848082d18257e2ffa71dd
> Author: Michal Clapinski <mclapinski@google.com>
>
> PCI/DOE: Poll briefly for a response before sleeping
>
> doe_statemachine_work() waits for a response by sleeping
> PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128) between status reads,
> which is 7 ms at HZ=1000 and 4 ms at HZ=250. Exchanges are served from
> the device's local mailbox logic and typically complete within tens of
> microseconds, so nearly all of that sleep is wasted. Every DOE-capable
> endpoint pays it once per supported feature during enumeration.
>
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 6b5d9ba8e9697..5e93d78a1a97e 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -27,6 +27,9 @@
> > /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
> > #define PCI_DOE_TIMEOUT HZ
> > #define PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128)
>
> [Severity: Medium]
> When HZ=100, does this integer division truncate to 0?
>
> If PCI_DOE_POLL_INTERVAL evaluates to 0, it permanently breaks the fallback
> sleep mechanism.
>
> > +#define PCI_DOE_POLL_FAST_MIN_US 20
> > +#define PCI_DOE_POLL_FAST_MAX_US 50
> > +#define PCI_DOE_POLL_FAST_TRIES 40
>
> [ ... ]
>
> > @@ -525,10 +528,25 @@ static void doe_statemachine_work(struct work_struct *work)
> > signal_task_abort(task, -EIO);
> > return;
> > }
> > - rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> > - if (rc) {
> > - signal_task_abort(task, rc);
> > - return;
> > + /*
> > + * Exchanges are served from the device's local mailbox
> > + * logic and usually complete in tens of microseconds, far
> > + * below the jiffies granularity of PCI_DOE_POLL_INTERVAL.
> > + * Poll for up to ~2 ms before falling back to it.
> > + */
> > + if (tries++ < PCI_DOE_POLL_FAST_TRIES) {
> > + if (test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags)) {
> > + signal_task_abort(task, -EIO);
> > + return;
> > + }
> > + usleep_range(PCI_DOE_POLL_FAST_MIN_US,
> > + PCI_DOE_POLL_FAST_MAX_US);
> > + } else {
> > + rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
>
> [Severity: Medium]
> Does passing 0 to this wait function when HZ=100 trigger a tight spin bug
> instead of yielding the CPU?
>
> Since wait_event_timeout() immediately returns without sleeping when the
> timeout is 0, this regression results in a tight CPU busy loop spinning over
> pci_read_config_dword() continuously for up to 1 second during enumeration
> for affected hardware.
Yes, but it's a preexisting issue.
> > + if (rc) {
> > + signal_task_abort(task, rc);
> > + return;
> > + }
> > }
> > goto retry_resp;
> > }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012114213.1757207-1-mclapinski@google.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-12 11:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-12 11:42 [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping Michal Clapinski
2026-10-12 11:51 ` sashiko-bot
2026-10-12 11:53 ` Michał Cłapiński
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox