Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <981016a7-f994-f0dd-422e-66ac909371c7@deltatee.com>
Date:   Mon, 21 Mar 2022 09:46:04 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Content-Language: en-CA
To:     Shlomo Pongratz <shlomopongratz@gmail.com>,
        linux-pci@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, andrew.maier@eideticom.com,
        bhelgaas@google.com, Shlomo Pongratz <shlomop@pliops.com>
References: <20220321143120.12191-1-shlomop@pliops.com>
From:   Logan Gunthorpe <logang@deltatee.com>
In-Reply-To: <20220321143120.12191-1-shlomop@pliops.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v1] Intel Sky Lake-E host root ports check.
Precedence: bulk


On 2022-03-21 08:31, 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.
> 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 bridges that have no children are ignored.
> As a result the routine pci_host_bridge_dev will return NULL for devices under slot C.
> In the proposed patch port field is added to the whitelist which is 0 for 2030, 1 for 2031,
> 2 for 2032 3 for 2033 and 0 for all other devices.

The patch looks largely ok, but I'm not sure I follow this description.

It sounds like in practice the host bridges B, C and D are not addressed
at function 0 as was assumed. But what does it mean that only C is
connected downstream? How can a bridge not be connected downstream?


> Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
> ---
>  drivers/pci/p2pdma.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 1015274bd2fe..86f6594a0b8a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -305,22 +305,23 @@ static bool cpu_supports_p2pdma(void)
>  static const struct pci_p2pdma_whitelist_entry {
>  	unsigned short vendor;
>  	unsigned short device;
> +	unsigned short port;
>  	enum {
>  		REQ_SAME_HOST_BRIDGE	= 1 << 0,
>  	} flags;

If port is placed after flags then we only need to add the port for the
three devices that care about it. Designated initializers will set it to
0 if it is omitted.

>  } pci_p2pdma_whitelist[] = {
>  	/* Intel Xeon E5/Core i7 */
> -	{PCI_VENDOR_ID_INTEL,	0x3c00, REQ_SAME_HOST_BRIDGE},
> -	{PCI_VENDOR_ID_INTEL,	0x3c01, REQ_SAME_HOST_BRIDGE},
> +	{PCI_VENDOR_ID_INTEL,	0x3c00, 0, REQ_SAME_HOST_BRIDGE},
> +	{PCI_VENDOR_ID_INTEL,	0x3c01, 0, REQ_SAME_HOST_BRIDGE},
>  	/* Intel Xeon E7 v3/Xeon E5 v3/Core i7 */
> -	{PCI_VENDOR_ID_INTEL,	0x2f00, REQ_SAME_HOST_BRIDGE},
> -	{PCI_VENDOR_ID_INTEL,	0x2f01, REQ_SAME_HOST_BRIDGE},
> +	{PCI_VENDOR_ID_INTEL,	0x2f00, 0, REQ_SAME_HOST_BRIDGE},
> +	{PCI_VENDOR_ID_INTEL,	0x2f01, 0, REQ_SAME_HOST_BRIDGE},
>  	/* Intel SkyLake-E */
> -	{PCI_VENDOR_ID_INTEL,	0x2030, 0},
> -	{PCI_VENDOR_ID_INTEL,	0x2031, 0},
> -	{PCI_VENDOR_ID_INTEL,	0x2032, 0},
> -	{PCI_VENDOR_ID_INTEL,	0x2033, 0},
> -	{PCI_VENDOR_ID_INTEL,	0x2020, 0},
> +	{PCI_VENDOR_ID_INTEL,	0x2030, 0, 0},
> +	{PCI_VENDOR_ID_INTEL,	0x2031, 1, 0},
> +	{PCI_VENDOR_ID_INTEL,	0x2032, 2, 0},
> +	{PCI_VENDOR_ID_INTEL,	0x2033, 3, 0},
> +	{PCI_VENDOR_ID_INTEL,	0x2020, 0, 0},
>  	{}
>  };
>  
>

Logan