On Wed, 07 Oct 2009 13:44:48 +0900
Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx> 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,
Applied, thanks.
--
Jesse Barnes, Intel Open Source Technology Center
--
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
