Re: [PATCH RESEND] PCI/DOE: Poll briefly for a response before sleeping

[sashiko-bot@kernel.org]

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