Re: [PATCH] Prevent AER driver from being loaded on PCIE devices which are not root ports.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Kenji Kaneshige wrote:
Malcolm Crossley wrote:
The PCI Express port driver is registering the AER service for any device
reporting AER capability. The AER driver uses registers which are only
available on PCI Express device root port devices which advertise AER
capabilities.

This patch adds a check that the PCI Express device is a root port before
enabling the AER service driver on that port.

Signed-off-by: Malcolm Crossley <malcolm.crossley2@xxxxxxxxxxx>
---
A bug was seen on boards using a PLX 8518 switch device which advertises AER on each of it's transparent bridges. The AER driver was loaded for each bridge and this driver tried to access the AER source ID register whenever an interrupt occured on the shared PCI INTX lines. The source ID register does not exist on non root port PCIE device's which advertise AER and trying to access this register causes a unsupported request error on the bridge. Thus, when the next interrupt occurs, another error is found and the non existent source ID register is accessed again, and so it goes on. The result is a spammed dmesg with unsupported request PCI express errors on the bridge device that the AER driver is loaded against.


 drivers/pci/pcie/portdrv_core.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 52f84fc..2f0cfd5 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -225,7 +225,9 @@ static int get_port_device_capability(struct pci_dev *dev)
     int services = 0, pos;
     u16 reg16;
     u32 reg32;
+    struct pcie_port_data *port_data;
+ port_data = pci_get_drvdata(dev);
     pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
     pci_read_config_word(dev, pos + PCIE_CAPABILITIES_REG, &reg16);
     /* Hot-Plug Capable */
@@ -236,7 +238,8 @@ static int get_port_device_capability(struct pci_dev *dev)
             services |= PCIE_PORT_SERVICE_HP;
     }
     /* AER capable */
-    if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR))
+    if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR) &&
+        port_data->port_type == PCIE_RC_PORT)
         services |= PCIE_PORT_SERVICE_AER;
     /* VC support */
     if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VC))


Hi,

I'm sorry for the delayed comment.

I think some of current AER driver's code seems to have an assumption
that struct pcie_device are allocated for switch upstream/downstream
ports. And it also seems to assume that any other AER driver would be
registered to switch upstream/downstream ports. See find_aer_service_iter()
in aerdrv_core.c, for example.

How about changing .port_type of struct pcie_port_service_driver for
AER driver like below?

Thanks,
Kenji Kaneshige


---
drivers/pci/pcie/aer/aerdrv.c |    2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: 20091005/drivers/pci/pcie/aer/aerdrv.c
===================================================================
--- 20091005.orig/drivers/pci/pcie/aer/aerdrv.c
+++ 20091005/drivers/pci/pcie/aer/aerdrv.c
@@ -52,7 +52,7 @@ static struct pci_error_handlers aer_err

static struct pcie_port_service_driver aerdriver = {
    .name        = "aer",
-    .port_type    = PCIE_ANY_PORT,
+    .port_type    = PCIE_RC_PORT,
    .service    = PCIE_PORT_SERVICE_AER,

    .probe        = aer_probe,

Hi Kenji,

Your patch is a much cleaner way of achieving the same goal. Thanks for letting pointing it out. I should have looked at the PCIE service driver registeration code more closely.

Jesse, could you drop my patch and apply Kenji's patch instead?

I tested that Kenji's patch achieves the same result as my patch, so you add tested by me as well.

Thanks,
Malcolm Crossley

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux