On Wed, 07 Oct 2009 10:22:47 +0100
Malcolm Crossley <malcolm.crossley2@xxxxxxxxxxx> wrote:
> 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.
Sure, thanks a lot Kenji-san.
--
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
