Re: [PATCH V2 1/1] Intel Sky Lake-E host root ports check.

[Jason Gunthorpe <jgg@nvidia.com>]

On Sun, Mar 27, 2022 at 02:20:11PM +0300, Shlomo Pongratz wrote:
> On commit 7b94b53db34f ("PCI/P2PDMA: Add Intel Sky Lake-E Root Ports B, C, D to the whitelist")
> Andrew Maier added the Sky Lake-E additional devices
> 2031, 2032 and 2033 root ports to the already existing 2030 device.
> 
> The Intel devices 2030, 2031, 2032 and 2033 which are ports A, B, C and D,
> and if all exist they will occupy slots 0 till 3 in that order.
> 
> Now if for example device 2030 is missing then there will no device on slot 0, but
> other devices can reside on other slots according to there port.
> For this reason the test that insisted that the bridge should be on slot 0 was modified
> to support bridges that are not on slot 0.

This helped our systems here! Thanks

Though to be clear the BIOS/ACPI modeling seems to be wrong in a way
which prevents linux from finding the true root port which is the main
cause of this problem.

2030-2033 are *root ports* not host bridges. So when we are in
pci_host_bridge_dev() the code is not looking at the system's host
bridge device at all, but a root port off the host bridge.

Which explains why the non-zero slot is happening.

So this might be better to add a flag 'IS_ROOT_PORT' instead of 'port'
and then just ignore the slot number entirely for root ports.

Though maybe someone has a better idea how the host bridge stuff is
supposed to work on these skylake-e systems.

> + * The method above will work in most cases but not for all.
> + * Note that the Intel devices 2030, 2031, 2032 and 2033 are ports A, B, C and D.
> + * Consider on a bus X only port C is connected downstream so in the PCI scan only
> + * device 8086:2032 on 0000:X:02.0 will be found as birdges with no children are ignored

'bridges' mispelled

> + *
>   * This function is equivalent to pci_get_slot(host->bus, 0), however it does
>   * not take the pci_bus_sem lock seeing __host_bridge_whitelist() must not
>   * sleep.
> @@ -350,7 +356,10 @@ static struct pci_dev *pci_host_bridge_dev(struct pci_host_bridge *host)
>  
>  	if (!root)
>  		return NULL;
> -	if (root->devfn != PCI_DEVFN(0, 0))
> +	/* Here just check that the function is 0
> +	 * The slot number will be checked later
> +	 */
> +	if (PCI_FUNC(root->devfn) != 0)
>  		return NULL;
>  
>  	return root;
> @@ -372,6 +381,13 @@ static bool __host_bridge_whitelist(struct pci_host_bridge *host,
>  	for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
>  		if (vendor != entry->vendor || device != entry->device)
>  			continue;
> +		/* For devices which are bounded to a specific slot
> +		 * (e.g. Intel Sky Lake-E host root ports) check the port is
> +		 * Identical to the slot number.
> +		 * For other devices continue to inssist on slot 0

"insist" mispelled.

Jason