Changes in v3:
Use Jason Gunthorpe suggestion, that is add a flag 'IS_ROOT_PORT'
instead of 'port' and then just ignore the slot number entirely for root ports.
Changes in v2:
Change comment and description based on Logan Gunthorpe comments.
Shlomo Pongratz (1):
Intel Sky Lake-E host root ports check.
drivers/pci/p2pdma.c | 42 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 37 insertions(+), 5 deletions(-)
--
2.17.1
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.
Signed-off-by: Shlomo Pongratz <[email protected]>
---
drivers/pci/p2pdma.c | 42 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 37 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 30b1df3c9d2f..c088d4ab64f4 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -307,6 +307,7 @@ static const struct pci_p2pdma_whitelist_entry {
unsigned short device;
enum {
REQ_SAME_HOST_BRIDGE = 1 << 0,
+ IS_ROOT_PORT = 1 << 1,
} flags;
} pci_p2pdma_whitelist[] = {
/* Intel Xeon E5/Core i7 */
@@ -316,15 +317,38 @@ static const struct pci_p2pdma_whitelist_entry {
{PCI_VENDOR_ID_INTEL, 0x2f00, REQ_SAME_HOST_BRIDGE},
{PCI_VENDOR_ID_INTEL, 0x2f01, 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, 0x2030, IS_ROOT_PORT},
+ {PCI_VENDOR_ID_INTEL, 0x2031, IS_ROOT_PORT},
+ {PCI_VENDOR_ID_INTEL, 0x2032, IS_ROOT_PORT},
+ {PCI_VENDOR_ID_INTEL, 0x2033, IS_ROOT_PORT},
{PCI_VENDOR_ID_INTEL, 0x2020, 0},
{PCI_VENDOR_ID_INTEL, 0x09a2, 0},
{}
};
+/*
+ * The functionality of thisunction can be integrated into
+ * __host_bridge_whitelist function but this will make the code
+ * less readable
+ */
+static bool pci_is_root_port(struct pci_dev *root)
+{
+ const struct pci_p2pdma_whitelist_entry *entry;
+ unsigned short vendor, device;
+
+ vendor = root->vendor;
+ device = root->device;
+
+ for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
+ if (vendor != entry->vendor || device != entry->device)
+ continue;
+
+ if (entry->flags & IS_ROOT_PORT)
+ return true;
+ }
+ return false;
+}
+
/*
* This lookup function tries to find the PCI device corresponding to a given
* host bridge.
@@ -333,6 +357,11 @@ static const struct pci_p2pdma_whitelist_entry {
* bus->devices list and that the devfn is 00.0. These assumptions should hold
* for all the devices in the whitelist above.
*
+ * 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 has devices connected to it so in the PCI scan only
+ * device 8086:2032 on 0000:X:02.0 will be found as bridges with no children are ignored
+ *
* 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 +379,9 @@ static struct pci_dev *pci_host_bridge_dev(struct pci_host_bridge *host)
if (!root)
return NULL;
- if (root->devfn != PCI_DEVFN(0, 0))
+
+ /* Intel Sky Lake-E host root ports can be on no zero slot */
+ if (root->devfn != PCI_DEVFN(0, 0) && !pci_is_root_port(root))
return NULL;
return root;
@@ -372,6 +403,7 @@ 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;
+
if (entry->flags & REQ_SAME_HOST_BRIDGE && !same_host_bridge)
return false;
--
2.17.1
Please update the subject line to match the style of previous ones.
On Tue, Mar 29, 2022 at 01:43:21PM +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.
I can't really parse the above, but "there port" is surely incorrect.
Wrap the commit log to fit in 80 columns like previous commits.
> Signed-off-by: Shlomo Pongratz <[email protected]>
> ---
> drivers/pci/p2pdma.c | 42 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 30b1df3c9d2f..c088d4ab64f4 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -307,6 +307,7 @@ static const struct pci_p2pdma_whitelist_entry {
> unsigned short device;
> enum {
> REQ_SAME_HOST_BRIDGE = 1 << 0,
> + IS_ROOT_PORT = 1 << 1,
> } flags;
> } pci_p2pdma_whitelist[] = {
> /* Intel Xeon E5/Core i7 */
> @@ -316,15 +317,38 @@ static const struct pci_p2pdma_whitelist_entry {
> {PCI_VENDOR_ID_INTEL, 0x2f00, REQ_SAME_HOST_BRIDGE},
> {PCI_VENDOR_ID_INTEL, 0x2f01, 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, 0x2030, IS_ROOT_PORT},
> + {PCI_VENDOR_ID_INTEL, 0x2031, IS_ROOT_PORT},
> + {PCI_VENDOR_ID_INTEL, 0x2032, IS_ROOT_PORT},
> + {PCI_VENDOR_ID_INTEL, 0x2033, IS_ROOT_PORT},
> {PCI_VENDOR_ID_INTEL, 0x2020, 0},
> {PCI_VENDOR_ID_INTEL, 0x09a2, 0},
> {}
> };
>
> +/*
> + * The functionality of thisunction can be integrated into
> + * __host_bridge_whitelist function but this will make the code
> + * less readable
> + */
Comment seems probably unnecessary, but "thisunction" is wrong.
> +static bool pci_is_root_port(struct pci_dev *root)
> +{
> + const struct pci_p2pdma_whitelist_entry *entry;
> + unsigned short vendor, device;
> +
> + vendor = root->vendor;
> + device = root->device;
> +
> + for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
> + if (vendor != entry->vendor || device != entry->device)
> + continue;
> +
> + if (entry->flags & IS_ROOT_PORT)
> + return true;
> + }
> + return false;
> +}
> +
> /*
> * This lookup function tries to find the PCI device corresponding to a given
> * host bridge.
> @@ -333,6 +357,11 @@ static const struct pci_p2pdma_whitelist_entry {
> * bus->devices list and that the devfn is 00.0. These assumptions should hold
> * for all the devices in the whitelist above.
> *
> + * 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 has devices connected to it so in the PCI scan only
> + * device 8086:2032 on 0000:X:02.0 will be found as bridges with no children are ignored
Wrap to 80 columns.
Apparently there's something super special about Sky Lake? Whatever
it is that's special needs to be explained somewhere.
> + *
> * 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 +379,9 @@ static struct pci_dev *pci_host_bridge_dev(struct pci_host_bridge *host)
>
> if (!root)
> return NULL;
> - if (root->devfn != PCI_DEVFN(0, 0))
> +
> + /* Intel Sky Lake-E host root ports can be on no zero slot */
> + if (root->devfn != PCI_DEVFN(0, 0) && !pci_is_root_port(root))
> return NULL;
>
> return root;
> @@ -372,6 +403,7 @@ 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;
> +
> if (entry->flags & REQ_SAME_HOST_BRIDGE && !same_host_bridge)
> return false;
>
> --
> 2.17.1
>
On Tue, Mar 29, 2022 at 01:43:21PM +0300, Shlomo Pongratz wrote:
> On commit 7b94b53db34f ("PCI/P2PDMA: Add Intel Sky Lake-E Root Ports B, C, D to the whitelist")
'in commit' and it is expected to word wrap the description
> 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 needs updating to the new test
>
> Signed-off-by: Shlomo Pongratz <[email protected]>
> drivers/pci/p2pdma.c | 42 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 30b1df3c9d2f..c088d4ab64f4 100644
> +++ b/drivers/pci/p2pdma.c
> @@ -307,6 +307,7 @@ static const struct pci_p2pdma_whitelist_entry {
> unsigned short device;
> enum {
> REQ_SAME_HOST_BRIDGE = 1 << 0,
> + IS_ROOT_PORT = 1 << 1,
> } flags;
> } pci_p2pdma_whitelist[] = {
> /* Intel Xeon E5/Core i7 */
> @@ -316,15 +317,38 @@ static const struct pci_p2pdma_whitelist_entry {
> {PCI_VENDOR_ID_INTEL, 0x2f00, REQ_SAME_HOST_BRIDGE},
> {PCI_VENDOR_ID_INTEL, 0x2f01, 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, 0x2030, IS_ROOT_PORT},
> + {PCI_VENDOR_ID_INTEL, 0x2031, IS_ROOT_PORT},
> + {PCI_VENDOR_ID_INTEL, 0x2032, IS_ROOT_PORT},
> + {PCI_VENDOR_ID_INTEL, 0x2033, IS_ROOT_PORT},
> {PCI_VENDOR_ID_INTEL, 0x2020, 0},
> {PCI_VENDOR_ID_INTEL, 0x09a2, 0},
> {}
> };
>
> +/*
> + * The functionality of thisunction can be integrated into
> + * __host_bridge_whitelist function but this will make the code
> + * less readable
This comment is not useful, explain what the function does or delete
it
> + */
> +static bool pci_is_root_port(struct pci_dev *root)
> +{
> + const struct pci_p2pdma_whitelist_entry *entry;
> + unsigned short vendor, device;
> +
> + vendor = root->vendor;
> + device = root->device;
> +
> + for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
> + if (vendor != entry->vendor || device != entry->device)
> + continue;
> +
> + if (entry->flags & IS_ROOT_PORT)
> + return true;
> + }
> + return false;
> +}
> +
> /*
> * This lookup function tries to find the PCI device corresponding to a given
> * host bridge.
> @@ -333,6 +357,11 @@ static const struct pci_p2pdma_whitelist_entry {
> * bus->devices list and that the devfn is 00.0. These assumptions should hold
> * for all the devices in the whitelist above.
> *
> + * 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 has devices connected to it so in the PCI scan only
> + * device 8086:2032 on 0000:X:02.0 will be found as bridges with no children are ignored
> + *
> * 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 +379,9 @@ static struct pci_dev *pci_host_bridge_dev(struct pci_host_bridge *host)
>
> if (!root)
> return NULL;
> - if (root->devfn != PCI_DEVFN(0, 0))
> +
> + /* Intel Sky Lake-E host root ports can be on no zero slot */
> + if (root->devfn != PCI_DEVFN(0, 0) && !pci_is_root_port(root))
> return NULL;
The name pci_host_bridge_dev needs some attention if it is now
returning a root port.
> @@ -372,6 +403,7 @@ 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;
> +
> if (entry->flags & REQ_SAME_HOST_BRIDGE && !same_host_bridge)
> return false;
Unnecessary hunk
Jason