Re: [PATCH 1/2] PCI: Don't treat ~0 as ready in pci_dev_wait() with RRS SV

[Lukas Wunner]

On Sat, Oct 10, 2026 at 06:05:28PM +0200, Alexander Gruhlke wrote:
> With RRS Software Visibility, pci_dev_wait() considers a device ready as
> soon as reading the Vendor ID doesn't return the RRS value.  Some devices
> don't respond with RRS while they aren't ready, so the read returns ~0
> and pci_dev_wait() stops waiting too early.  This was seen with Intel
> [8086:0a54] and Samsung [144d:a80c] NVMe SSDs.

If I understand the spec correctly (PCIe r7.1 sec 2.3.2), enabling
RRS Software Visibility at the Root Port doesn't mean that every
failed device access has to return 0x0001.

Rather, that value is only returned if the device sent an RRS Completion
to the Root Complex.

It seems support for RRS Completions in Endpoints is optional because the
"Implementation Note: Request Retry Status for Configuration Requests"
at the end of PCIe r7.1 sec 2.3.1 says devices are "permitted" to send
RRS Completions.  That's a spec term used if something is optional.
The device may send such Completions, but it doesn't have to.

Also, the device may not be accessible at all, in which case it's sending
no Completion to the Root Complex.

> @@ -1215,7 +1215,8 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  	 * If the device is below a Root Port with Configuration RRS
>  	 * Software Visibility enabled, reading the Vendor ID returns a
>  	 * special data value if the device responded with RRS.  Read the
> -	 * Vendor ID until we get non-RRS status.
> +	 * Vendor ID until we get non-RRS status, then the Command register
> +	 * as below.
>  	 *

The code comment should be rephrased such that "returns" is replaced
with "may return" because of the optionality of RRS Completions.

> @@ -1235,8 +1236,11 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  
>  		if (root && root->config_rrs_sv) {
>  			pci_read_config_dword(dev, PCI_VENDOR_ID, &id);
> -			if (!pci_bus_rrs_vendor_id(id))
> -				break;
> +			if (!pci_bus_rrs_vendor_id(id)) {
> +				pci_read_config_dword(dev, PCI_COMMAND, &id);
> +				if (!PCI_POSSIBLE_ERROR(id))
> +					break;
> +			}

I think it's sufficient if you just change the if-condition like this:

-			if (!pci_bus_rrs_vendor_id(id))
+			if (!pci_bus_rrs_vendor_id(id) &&
+			    !PCI_POSSIBLE_ERROR(id)) {

I don't see the need for the extra read of the Command register
you're performing.

Thanks,

Lukas