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, ®16);
/* 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
