* [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
2026-10-16 20:17 ` David Matlack
0 siblings, 2 replies; 5+ 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] 5+ 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
2026-10-16 20:17 ` David Matlack
1 sibling, 1 reply; 5+ 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] 5+ 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; 5+ 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] 5+ 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-16 20:17 ` David Matlack
2026-10-16 20:45 ` Michał Cłapiński
1 sibling, 1 reply; 5+ messages in thread
From: David Matlack @ 2026-10-16 20:17 UTC (permalink / raw)
To: Michal Clapinski
Cc: Bjorn Helgaas, linux-pci, Lukas Wunner, linux-kernel,
Pasha Tatashin
On 2026-10-12 01:42 PM, Michal Clapinski wrote:
> 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.
Does any of this remaining time come from pci_doe_abort()? I see that is
also called during mailbox creation and has the same call to
pci_doe_wait(). If so, maybe the poll logic can be shared.
>
> 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)
It would be nice to also fix the HZ=100 issue, in a separate patch
within the same series, since you are updating this code.
> +#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 tens of microsecond is the norm then is polling for up to 2ms
overkill?
> + 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 [flat|nested] 5+ messages in thread* Re: [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping
2026-10-16 20:17 ` David Matlack
@ 2026-10-16 20:45 ` Michał Cłapiński
0 siblings, 0 replies; 5+ messages in thread
From: Michał Cłapiński @ 2026-10-16 20:45 UTC (permalink / raw)
To: David Matlack
Cc: Bjorn Helgaas, linux-pci, Lukas Wunner, linux-kernel,
Pasha Tatashin
On Fri, Oct 16, 2026 at 10:17 PM David Matlack <dmatlack@google.com> wrote:
>
> On 2026-10-12 01:42 PM, Michal Clapinski wrote:
> > 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.
>
> Does any of this remaining time come from pci_doe_abort()? I see that is
> also called during mailbox creation and has the same call to
> pci_doe_wait(). If so, maybe the poll logic can be shared.
Yes, but I'm not sure if the poll logic can be shared without making
the code worse rather than better. I'll look into that.
Also, I'm planning on skipping pci_doe_abort for preserved devices so
it was less of a priority for me.
> >
> > 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)
>
> It would be nice to also fix the HZ=100 issue, in a separate patch
> within the same series, since you are updating this code.
Okay, will do in v2.
> > +#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 tens of microsecond is the norm then is polling for up to 2ms
> overkill?
I guess. I can do more precise measurements and tweak this value;
however, I can only speak for the device I have on hand, so generally
I prefer overkilling 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 [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-10-16 20:45 UTC | newest]
Thread overview: 5+ 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
2026-10-16 20:17 ` David Matlack
2026-10-16 20:45 ` Michał Cłapiński
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.